[FFmpeg-devel] [PATCH] FLAC parser

[Justin Ruggles]

Hi,

Michael Chinen wrote:

> I've included everything from yours and Diego's review, but I'll
> respond to the questions below.  I feel more confident about it now,
> thanks again to both of you.
> 
> On Sun, Aug 1, 2010 at 10:29 PM, Justin Ruggles
> <justin.ruggles at gmail.com> wrote:
> [...]
>>> +/* the maximum number of adjacent headers that will compare crcs against each other */
>>> +#define FLAC_MAX_SEQUENTIAL_HEADERS 4
>>> +/* the minimum number of headers that need to be buffered and checked before returning frames */
>>> +#define FLAC_MIN_HEADERS 20
>>
>> Just out of curiosity, have you done testing with fewer headers, both
>> buffered and sequential?
> I tried before with 3 and 10 respectively, and things still seemed to
> work okay, but arbitrarily went with 4 and 20.  Maybe it's overkill?

If 3 and 10 works well, then I think it should be used.  I'll test the
FLAC files in my lossless music collection (about 450 songs).

Your newest patch is broken.  It took me a while to figure out what you
had changed, so I don't have time this evening to write up a new review.
 But you need to take a look at flac_parser.c line 238.

I'll send a new review tomorrow evening.  Maybe Diego will do another
style/doc review before then?  That's where most of the remaining issues
are.

Cheers,
Justin