[FFmpeg-devel] [PATCH] read metadata in FLAC demuxer

[Justin Ruggles]

Michael Niedermayer wrote:
> Hi
> 
> On Mon, Oct 01, 2007 at 11:30:28PM -0400, Justin Ruggles wrote:
>> Hi,
>>
>> Here is a new patch.  There is now very little redundancy.  The FLAC 
>> demuxer, Ogg-FLAC demuxer, and FLAC decoder all share a single function to 
>> parse the streaminfo header.  The rest of the metadata is handled according 
>> to the format and requirements of each.  This simplifies the decoder 
>> metadata parsing quite a bit and fixes related bugs.
>>
>> The changes to the Ogg demuxer are shown here for demonstration purposes 
>> only.  The changes would be done separately, in 2 commits.  One to utilize 
>> the shared function and another to simplify the code.  Due to the 
>> dependency on the flac decoder, this maybe should wait until if/when a FLAC 
>> parser is implemented.
>>
>> Ideally, I would like to rename flac.c to flacdec.c and put the shared 
>> function into a new flac.c.  As far as the build system is concerned, this 
>> would only be useful if there was a parser to use a dependency reference 
>> which is tied to the shared code, similar to how it's done with AC3.
> 
> iam ok with spliting decoder only funtions from flac.c into flacdec.c
> (with svn cp of course)

Great.  I'll submit that as the last step.

> partal review below either iam too tired or the patch is a mess ...
> and please split the patch into small selfcontained changes!

Maybe both.  And ok, I'll split it.

> [...]
> 
>> @@ -143,28 +144,62 @@
>>      s->bitstream= av_fast_realloc(s->bitstream, &s->allocated_bitstream_size, s->max_framesize);
>>  }
>>  
>> -static void metadata_streaminfo(FLACContext *s)
>> +int flac_decode_streaminfo(FlacStreaminfo *s, uint8_t data[FLAC_STREAMINFO_SIZE])
> 
> this needs a ff_ prefix

You're right. I'll fix it.

> 
> [...]
>> +    s->min_blocksize = si.min_blocksize;
>> +    s->max_blocksize = si.max_blocksize;
>>  
>> +    s->min_framesize = si.min_framesize;
>> +    s->max_framesize = si.max_framesize;
>> +
>> +    s->samplerate = si.samplerate;
>> +    s->channels = si.channels;
>> +    s->bps = si.bps;
> 
> no, not a third struct to hold the same values
> please correct me if iam wrong but they could just be stored in
> AVCodecContext and FlacStreaminfo could even be part of FLACContext

Using the AVCodecContext in this case seems like a bad idea since the 
values could be too easily changed by the user at any point.  I will 
make FlacStreaminfo part of FLACContext to reduce the copying though.

> and min_framesize is not used

True.  I never really thought about it, but it is kind of a pointless 
piece of data.

> 
> [...]
>> -    if (show_bits_long(&s->gb, 32) == MKBETAG('f','L','a','C')) {
>> -        skip_bits(&s->gb, 32);
>> +    if(buf[0] != 'f' || buf[1] != 'L' || buf[2] != 'a' || buf[3] != 'C') {
>> +        av_log(s->avctx, AV_LOG_ERROR, "fLaC marker not found\n");
>> +        return -1;
>> +    }
> 
> no, theres AV_RL/B32() or memcmp()

I've never used the AV_RL/B stuff before.  Thanks for pointing it out. 
I'll use it now.

And thanks for the review.  It was helpful even if you were sleepy. :)

-Justin