[FFmpeg-devel] [PATCH] read metadata in FLAC demuxer
Justin Ruggles
justinruggles
Wed Oct 3 02:20:08 CEST 2007
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
More information about the ffmpeg-devel
mailing list