[FFmpeg-devel] [PATCH] flac demuxer

Michael Niedermayer michaelni
Tue Apr 29 00:01:22 CEST 2008


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.


> 
> > 
> > [...]
> >> diff --git a/libavcodec/flac.c b/libavcodec/flac.c
> >> index ed5b2d2..4b51946 100644
> >> --- a/libavcodec/flac.c
> >> +++ b/libavcodec/flac.c
> >> @@ -59,11 +59,9 @@ typedef struct FLACContext {
> >>      AVCodecContext *avctx;
> >>      GetBitContext gb;
> >>  
> >> -    int min_blocksize, max_blocksize;
> >> -    int max_framesize;
> >> -    int samplerate, channels;
> >> +    FLACStreaminfo info;
> >>      int blocksize/*, last_blocksize*/;
> >> -    int bps, curr_bps;
> >> +    int curr_bps;
> >>      enum decorrelation_type decorrelation;
> >>  
> >>      int32_t *decoded[MAX_CHANNELS];
> > 
> >> @@ -149,50 +147,43 @@ static av_cold int flac_decode_init(AVCodecContext * avctx)
> >>  
> >>  static void dump_headers(FLACContext *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(s->avctx, AV_LOG_DEBUG, "  Blocksize: %d .. %d (%d)\n", s->info.min_block_size, s->info.max_block_size, s->blocksize);
> >> +    av_log(s->avctx, AV_LOG_DEBUG, "  Max Framesize: %d\n", s->info.max_frame_size);
> >> +    av_log(s->avctx, AV_LOG_DEBUG, "  Samplerate: %d\n", s->info.sample_rate);
> >> +    av_log(s->avctx, AV_LOG_DEBUG, "  Channels: %d\n", s->info.channels);
> >> +    av_log(s->avctx, AV_LOG_DEBUG, "  Bits: %d\n", s->info.bps);
> >>  }
> >>  
> >>  static void allocate_buffers(FLACContext *s){
> >>      int i;
> >>  
> >> -    assert(s->max_blocksize);
> >> +    assert(s->info.max_block_size);
> >>  
> >> -    if(s->max_framesize == 0 && s->max_blocksize){
> >> -        s->max_framesize= (s->channels * s->bps * s->max_blocksize + 7)/ 8; //FIXME header overhead
> >> +    if(s->info.max_frame_size == 0 && s->info.max_block_size){
> >> +        s->info.max_frame_size= (s->info.channels * s->info.bps * s->info.max_block_size + 7)/ 8; //FIXME header overhead
> >>      }
> >>  
> >> -    for (i = 0; i < s->channels; i++)
> >> +    for (i = 0; i < s->info.channels; i++)
> >>      {
> >> -        s->decoded[i] = av_realloc(s->decoded[i], sizeof(int32_t)*s->max_blocksize);
> >> +        s->decoded[i] = av_realloc(s->decoded[i], sizeof(int32_t)*s->info.max_block_size);
> >>      }
> >>  
> >> -    s->bitstream= av_fast_realloc(s->bitstream, &s->allocated_bitstream_size, s->max_framesize);
> >> +    s->bitstream= av_fast_realloc(s->bitstream, &s->allocated_bitstream_size, s->info.max_frame_size);
> >>  }
> > 
> > i do not like having "info." added to half of all lines of code
> 
> ok. well, several things could be done. what do you think would be the
> optimal solution?
> 
> 1. copy all fields to local context
>     - i did this for ac3, but iirc you did not like it
> 2. copy most common fields to local context
>     - max_frame_size and channels are the 2 most commonly used
> 3. get rid of the struct and pass pointers to all 7.
>     - kinda ugly
> 4. something else i'm not thinking of...

definitly 4 :)

#define FLACSTREAMINFO \
    int min_block_size;     /**< minimum block size, in samples          */\
    int max_block_size;     /**< maximum block size, in samples          */\
    int max_frame_size;     /**< maximum frame size, in bytes            */\
    int sample_rate;        /**< 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;

typedef struct FLACContext {
    FLACSTREAMINFO
...

ff_flac_parse_streaminfo((FLACStreaminfo*)FLACContext);

Would be the obvious solution, there might very well be nicer ones ...


> 
> > 
> > [...]
> > moving code cannot be split in a patch duplicating it and one removing the old
> > this is unacceptable. Its like using cvs add and remove instead of svn mv.
> 
> yeah, i was a little worried about that.  

It also confuses gits ancestor finding IIRC. Code moves should be in one
commit so that the disapearing and newly appearing code can be associated
automatically.


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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I count him braver who overcomes his desires than him who conquers his
enemies for the hardest victory is over self. -- Aristotle
-------------- 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/d58c61d5/attachment.pgp>



More information about the ffmpeg-devel mailing list