[FFmpeg-devel] [PATCH] FLAC parser

[Justin Ruggles]

Hi,

Michael Chinen wrote:

> Here is an updated patch that combined with the previous index_entries
> pos patch will result in cleaner seeking for FLAC (seek directly to
> frame start instead of junk.)
> Also please don't forget the 0002 patch from the first email.

> +/**
> + * @file flac_parser.c
> + * @brief A FLAC parser that tracks chains of headers to minimize false positives


Don't include the file name in the @file statement.
Also, @brief is not necessary.  @brief is assumed for the 1st line (well
technically up to the first period followed by a space or new line).
Just "FLAC parser" is sufficient for the brief description since your
detailed description is so in-depth.

> [...]
> +typedef struct FLACHeaderMarker {
> +    int offset;       /**< byte offset from start of FLACParseContext->buffer */
> +
> +    int crc_valid;    /**< each bit of crc_valid is a flag which is set if the
> +                         16 bit CRC is valid from this header to the header at
> +                         a distance equal to the bit position                 */
> +
> +    int max_score;    /**< maximum score found after checking each child that
> +                          has a valid CRC                                     */
> +
> +    FLACFrameInfo fi; /**< decoded frame header info                          */
> +
> +    struct FLACHeaderMarker *next;       /**< next CRC-8 verified header that
> +                                            immediately follows this one in
> +                                            the bytestream                    */
> +
> +    struct FLACHeaderMarker *best_child; /**< following frame header with
> +                                            which this frame has the best
> +                                            score with                        */
> +} FLACHeaderMarker;
> +
> +typedef struct FLACParseContext {
> +    uint8_t *buffer;               /**< buffer to store all data until headers
> +                                      can be verified                         */
> +
> +    int buffer_size;               /**< number of valid bytes in the buffer   */
> +
> +    int buffer_allocated_size;     /**< actual allocated size of the buffer   */
> +
> +    FLACHeaderMarker *headers;     /**< linked-list that starts at the first
> +                                      CRC-8 verified header within buffer     */
> +
> +    FLACHeaderMarker *best_header; /**< highest scoring header within buffer  */
> +
> +    int nb_headers_found;          /**< number of headers found in the last
> +                                      flac_parse() call                       */
> +
> +    int best_header_valid;         /**< flag set when the parser returns junk;
> +                                      if set return the best_header next time */
> +} FLACParseContext;


I would suggest aligning 2nd line text with 1st line text.  Also, the
blank lines are not necessary, but I guess it's not really a big deal.

> +
> +static int frame_header_is_valid(const uint8_t *buf, FLACFrameInfo *fi)
> +{
> +    GetBitContext gb;
> +    init_get_bits(&gb, buf, MAX_FRAME_HEADER_SIZE*8);
> +    if (ff_flac_decode_frame_header(&gb, fi))
> +        return 0;
> +    return 1;


return !ff_flac_decode_frame_header(&gb, fi)


> [...]
> +        av_log(NULL, AV_LOG_DEBUG,
> +               "update_sequence CRC failed at dist %i from %i to %i\n",
> +               distance, mid->offset, end->offset);

here and many other places, you can split the string mid-line by doing:
av_log(avctx, AV_LOG_WHATEVER, "start some text here but need to "
       "break the line at 80 chars to keep things nice and neat")

oh, and you should pass a context to av_log() whenever possible.  there
are 4 places in flac_parser.c where you pass NULL.  you could store a
pointer to the AVCodecContext in the FLACParseContext.

> [...]
> +            *end_handle = av_mallocz(sizeof(FLACHeaderMarker));
> +            if (!*end_handle)
> +                return AVERROR(ENOMEM);


I would suggest also logging an error message here since the error code
is not passed down to the user level.

> [...]
> +
> +/**
> + * @brief Score a header.


Explicitly using @brief is not necessary.

> [...]
> +            /* Check sample and frame numbers. */
> +            if ((child->fi.frame_or_sample_num - header->fi.frame_or_sample_num
> +                 != child->fi.blocksize) &&
> +                (child->fi.frame_or_sample_num
> +                 != header->fi.frame_or_sample_num + 1)) {
> +                    child_score -= FLAC_HEADER_CHANGED_PENALTY;
> +            }


This is not correct.  Should be:
child sample num - header sample num = header blocksize

Try testing with a variable blocksize sample.

> [...]
> +    if (fpc->best_header) {
> +        if (fpc->best_header->offset > 0) {
> +            /* Output a junk frame. */
> +            av_log(NULL, AV_LOG_DEBUG, "flac parser:junk frame till offset %i\n",
> +                   fpc->best_header->offset);
> +
> +            /* frame_size = 0 makes compute_pkt_fields do the correct thing */


You shouldn't mention compute_pkt_fields here.  The parser is in
libavcodec, but compute_pkt_fields() is a local function in libavformat.
Just mention that you're setting frame_size to 0 because the junk frame
is invalid, therefore the number of samples is either unknown or not
applicable.

The rest of the patch looks fine.

-Justin