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

[Michael Niedermayer]

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)

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

[...]

> @@ -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


[...]
> +    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

and min_framesize is not used


[...]
> -    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()


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071002/a034d431/attachment.pgp>