[FFmpeg-devel] [PATCH] flac demuxer

Justin Ruggles justinruggles
Tue Apr 29 05:34:01 CEST 2008


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

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

ok. I'll try that then.

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

Yeah, that does make sense.

> 
>> 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(-)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: flac_demux_01.patch
Type: text/x-diff
Size: 7022 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080428/7b56362a/attachment.patch>



More information about the ffmpeg-devel mailing list