[FFmpeg-devel] [PATCH] FLAC parser

[Justin Ruggles]

Michael Chinen wrote:

> Hi,
> 
> Here is a new patch with the discussed changes.
> Also style and log cleanups (although I'm sure there's more that I'm
> not catching.)

A few things:

In the parser, you cannot assume that the AVCodecContext private data
will be a FLACContext.  It seems you're only using it to get the maximum
block size from the header.  I don't think having that check really adds
much to the parser.  There are enough other checks in there that will
catch a fake frame.  Also, the decoder can technically (although not
implemented yet) work without the STREAMINFO header in many cases.

I know the ff_flac_decode_frame_header() changes were originally part of
my patch, but I think it might be a better idea to return different
error codes instead of passing an AVCodecContext or NULL to indicate
printing of error messages.  That way the parser can just check if there
is an error, and the decoder can optionally print out error messages in
the calling function depending on the error value.  See the AC3 parser
for a similar example.

Regarding the debug messages for penalties, maybe there could be a way
to only print those out if the frame with the penalties is actually
selected for output.  Possibly a penalty mask in addition to the score?
 Then the messages could be changed to warnings instead of debug messages.

With the addition of the parser, the check for the frame sync code and
resync in flac_decode_frame() is unnecessary.

I assume the under_last variable in flacdec.c is a leftover from debugging?

It looks like at least one of the goto markers in flac_parse() can be
cleanly eliminated, maybe more.

Double-check for trailing white space.

Besides that, it is mostly good.  Once these items are addressed I can
do a more detailed code review.

Cheers,
Justin