[FFmpeg-devel] [PATCH] flac demuxer

[Justin Ruggles]

Hi,

Thanks for the quick review. :)

Michael Niedermayer wrote:
> On Mon, Apr 28, 2008 at 12:51:10AM -0400, Justin Ruggles wrote:
>> Justin Ruggles wrote:
>> diff --git a/libavcodec/flac.c b/libavcodec/flac.c
>> index bebed03..b849e7f 100644
>> --- a/libavcodec/flac.c
>> +++ b/libavcodec/flac.c
>> @@ -60,7 +60,7 @@ typedef struct FLACContext {
>>      GetBitContext gb;
>>  
>>      int min_blocksize, max_blocksize;
>> -    int min_framesize, max_framesize;
>> +    int max_framesize;
>>      int samplerate, channels;
>>      int blocksize/*, last_blocksize*/;
>>      int bps, curr_bps;
>> @@ -120,7 +120,7 @@ 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, "  Framesize: %d .. %d\n", s->min_framesize, s->max_framesize);
>> +    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);
>> @@ -149,7 +149,7 @@ static void metadata_streaminfo(FLACContext *s)
>>      s->min_blocksize = get_bits(&s->gb, 16);
>>      s->max_blocksize = get_bits(&s->gb, 16);
>>  
>> -    s->min_framesize = get_bits_long(&s->gb, 24);
>> +    skip_bits(&s->gb, 24); /* skip min frame size */
>>      s->max_framesize = get_bits_long(&s->gb, 24);
>>  
>>      s->samplerate = get_bits_long(&s->gb, 20);
>> -- 
>> 1.5.3.5
>>
> 
> this patch is ok

ok. i'll go ahead and apply that.

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

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

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


Thanks,
Justin