[FFmpeg-devel] [PATCH] flac demuxer

[Michael Niedermayer]

On Mon, Apr 28, 2008 at 11:34:01PM -0400, Justin Ruggles wrote:
> Michael Niedermayer wrote:
> > On Mon, Apr 28, 2008 at 05:31:07PM -0400, Justin Ruggles wrote:
> > [...]
> >>>
> >>>> diff --git a/libavcodec/flac.c b/libavcodec/flac.c
> >>>> index b849e7f..ed5b2d2 100644
> >>>> --- a/libavcodec/flac.c
> >>>> +++ b/libavcodec/flac.c
> >>>> @@ -40,13 +40,13 @@
> >>>>  #include "bitstream.h"
> >>>>  #include "golomb.h"
> >>>>  #include "crc.h"
> >>>> +#include "flac.h"
> >>>>  
> >>>>  #undef NDEBUG
> >>>>  #include <assert.h>
> >>>>  
> >>>>  #define MAX_CHANNELS 8
> >>>>  #define MAX_BLOCKSIZE 65535
> >>>> -#define FLAC_STREAMINFO_SIZE 34
> >>>>  
> >>>>  enum decorrelation_type {
> >>>>      INDEPENDENT,
> >>>> @@ -98,6 +98,36 @@ static void metadata_streaminfo(FLACContext *s);
> >>>>  static void allocate_buffers(FLACContext *s);
> >>>>  static int metadata_parse(FLACContext *s);
> >>>>  
> >>>> +int ff_flac_parse_streaminfo(FLACStreaminfo *s, const uint8_t *buffer)
> >>>> +{
> >>>> +    GetBitContext gbc;
> >>>> +    init_get_bits(&gbc, buffer, FLAC_STREAMINFO_SIZE*8);
> >>>> +
> >>>> +    s->min_block_size = get_bits(&gbc, 16);
> >>>> +    s->max_block_size = get_bits(&gbc, 16);
> >>>> +    if(s->max_block_size < 16)
> >>>> +        return -1;
> >>>> +
> >>>> +    skip_bits_long(&gbc, 24); // skip min frame size
> >>>> +    s->max_frame_size = get_bits_long(&gbc, 24);
> >>>> +
> >>>> +    s->sample_rate = get_bits_long(&gbc, 20);
> >>>> +    if(s->sample_rate < 1 || s->sample_rate > 655350)
> >>>> +        return -1;
> >>>> +
> >>>> +    s->channels = get_bits(&gbc, 3) + 1;
> >>>> +
> >>>> +    s->bps = get_bits(&gbc, 5) + 1;
> >>>> +    if(s->bps < 8 || s->bps & 0x3)
> >>>> +        return -1;
> >>>> +
> >>>> +    s->total_samples = get_bits_long(&gbc, 36);
> >>>> +
> >>>> +    // don't need to parse MD5 checksum
> >>>> +
> >>>> +    return 0;
> >>>> +}
> >>> code duplication
> >> How is that code duplication if I'm removing the similar code in the
> >> same commit?  I could try to modify the existing function, maybe in
> >> several steps, if that would make it more obvious what is happening.
> > 
> > Well maybe "duplication" is the wrong word. Senseless bloat is maybe better
> > 
> > diffstat says:
> >  flac.c |   56 +++++++++++++++++++++++++++++++++++++++++++-------------
> >  flac.h |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 93 insertions(+), 14 deletions(-)
> > 
> > This hardly can count as moving code around, if id assume all the removed
> > lines are moved then 14 turn into 93 thats more than 6 times.
> > Also even if one completely ignores flac.h its still huge bloat.
> > 
> > The naive solution of litterally duplicating the code would be less
> > bloated.
> 
> That diffstat makes it look worse than it really is, but still, I was
> too focused on the final result and not on making each step as small and
> clean as possible.  The final code has added checks for header validity
> and more whitespace.  But for now, I'll just take it one step at a time,
> so you can reject the "bloat" later if you wish. :)

:)


[...]
> > 
> >> i was trying to avoid large
> >> patches.  does this sound any better?
> >>
> >> - create and use a common struct for streaminfo data
> >>
> >> - modify streaminfo header parsing in flac.c to be sharable
> >>
> >> - split out the flac demuxer into a new file with same functionality as
> >> it has currently
> >>
> >> - move metadata parsing from the decoder to demuxer (in a single, kinda
> >> big, commit)
> > 
> > I really do not know what is best without seeing the patches. Basically
> > they should be clean, small and definitly not add more code than they
> > remove unless really needed.
> 
> ok. I'll try again with a focus on making the changes as small and clean
> as possible rather than on the final result.
> 
> Here is the first patch which allows for the streaminfo parsing function
> to eventually be shared with the demuxer.
> 
> Thanks,
> Justin
> 
> 
>  libavcodec/flac.c |   77
> +++++++++++++++++++++++++++++++++-------------------
>  libavcodec/flac.h |   43 +++++++++++++++++++++++++++++
>  2 files changed, 92 insertions(+), 28 deletions(-)
> 

[...]
> +/**
> + * Data needed from the Streaminfo header for use by the raw FLAC demuxer
> + * and/or the FLAC decoder.
> + */
> +#define FLACSTREAMINFO \
> +    int min_blocksize;      /**< minimum block size, in samples          */\
> +    int max_blocksize;      /**< maximum block size, in samples          */\
> +    int max_framesize;      /**< maximum frame size, in bytes            */\
> +    int samplerate;         /**< sample rate                             */\
> +    int channels;           /**< number of channels                      */\
> +    int bps;                /**< bits-per-sample                         */\
> +    uint64_t total_samples; /**< total number of samples, or 0 if unknown*/\
> +
> +typedef struct FLACStreaminfo {
> +    FLACSTREAMINFO
> +} FLACStreaminfo;

Wont these 2 be needed in a header later?


[...]
> @@ -117,13 +133,13 @@ static av_cold int flac_decode_init(AVCodecContext * avctx)
>      return 0;
>  }
>  
> -static void dump_headers(FLACContext *s)
> +static void dump_headers(AVCodecContext *avctx, FLACStreaminfo *s)
>  {
> -    av_log(s->avctx, AV_LOG_DEBUG, "  Blocksize: %d .. %d (%d)\n", s->min_blocksize, s->max_blocksize, s->blocksize);
> -    av_log(s->avctx, AV_LOG_DEBUG, "  Max Framesize: %d\n", s->max_framesize);
> -    av_log(s->avctx, AV_LOG_DEBUG, "  Samplerate: %d\n", s->samplerate);
> -    av_log(s->avctx, AV_LOG_DEBUG, "  Channels: %d\n", s->channels);
> -    av_log(s->avctx, AV_LOG_DEBUG, "  Bits: %d\n", s->bps);
> +    av_log(avctx, AV_LOG_DEBUG, "  Blocksize: %d .. %d\n", s->min_blocksize, s->max_blocksize);
> +    av_log(avctx, AV_LOG_DEBUG, "  Max Framesize: %d\n", s->max_framesize);
> +    av_log(avctx, AV_LOG_DEBUG, "  Samplerate: %d\n", s->samplerate);
> +    av_log(avctx, AV_LOG_DEBUG, "  Channels: %d\n", s->channels);
> +    av_log(avctx, AV_LOG_DEBUG, "  Bits: %d\n", s->bps);
>  }

This could be in a seperate patch


>  
>  static void allocate_buffers(FLACContext *s){
> @@ -143,28 +159,33 @@ static void allocate_buffers(FLACContext *s){
>      s->bitstream= av_fast_realloc(s->bitstream, &s->allocated_bitstream_size, s->max_framesize);
>  }
>  

> -static void metadata_streaminfo(FLACContext *s)
> +int ff_flac_parse_streaminfo(AVCodecContext *avctx, FLACStreaminfo *s,
> +                             const uint8_t *buffer)
>  {
> +    GetBitContext gb;
> +    init_get_bits(&gb, buffer, FLAC_STREAMINFO_SIZE*8);

Why is the change from GetBitContext to uint8_t * done? It does not seem
needed? All callers have a GetBitContext already.


> +
>      /* mandatory streaminfo */
> -    s->min_blocksize = get_bits(&s->gb, 16);
> -    s->max_blocksize = get_bits(&s->gb, 16);
> +    s->min_blocksize = get_bits(&gb, 16);
> +    s->max_blocksize = get_bits(&gb, 16);

This as well could be in a seperate patch


>  
> -    skip_bits(&s->gb, 24); /* skip min frame size */
> -    s->max_framesize = get_bits_long(&s->gb, 24);
> +    skip_bits(&gb, 24); /* skip min frame size */
> +    s->max_framesize = get_bits_long(&gb, 24);
>  
> -    s->samplerate = get_bits_long(&s->gb, 20);
> -    s->channels = get_bits(&s->gb, 3) + 1;
> -    s->bps = get_bits(&s->gb, 5) + 1;
> +    s->samplerate = get_bits_long(&gb, 20);
> +    s->channels = get_bits(&gb, 3) + 1;
> +    s->bps = get_bits(&gb, 5) + 1;
>  
> -    s->avctx->channels = s->channels;
> -    s->avctx->sample_rate = s->samplerate;
> +    avctx->channels = s->channels;
> +    avctx->sample_rate = s->samplerate;
>  
> -    skip_bits(&s->gb, 36); /* total num of samples */
> +    skip_bits(&gb, 36); /* total num of samples */
>  
> -    skip_bits(&s->gb, 64); /* md5 sum */
> -    skip_bits(&s->gb, 64); /* md5 sum */
> +    skip_bits(&gb, 64); /* md5 sum */
> +    skip_bits(&gb, 64); /* md5 sum */
>  
> -    dump_headers(s);
> +    dump_headers(avctx, s);

> +    return 0;

if its always return 0 than it could as well stay void


>  }
>  
>  /**
> @@ -193,9 +214,9 @@ static int metadata_parse(FLACContext *s)
>              if (metadata_size) {
>                  switch (metadata_type) {
>                  case METADATA_TYPE_STREAMINFO:
> -                    metadata_streaminfo(s);
> +                    ff_flac_parse_streaminfo(s->avctx, (FLACStreaminfo *)s,
> +                            s->gb.buffer+get_bits_count(&s->gb)/8);
>                      streaminfo_updated = 1;
> -                    break;
>  
>                  default:
>                      for (i=0; i<metadata_size; i++)


Is the removed break; intended?


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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- 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/20080429/0cade07a/attachment.pgp>