[FFmpeg-devel] [PATCH] flac demuxer

[Justin Ruggles]

Michael Niedermayer wrote:
> 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:
> [...]
>>>> 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?

Yes, they will. Silly me.

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

ok.

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

Because it's not the best choice in the end.  Currently, the decoder
init function sets up a bitstream reader when it really doesn't need to.
 The metadata parsing function uses the decoder context bitstream
reader, but that function will be moving to the demuxer, where it will
not need to use a GetBitContext at all.  Initializing 2 bitstream
readers in 2 separate places for the sole purpose of passing them to a
single function is code duplication.  The only downside is that there is
temporarily 1 ugly call which accesses the internal buffer of the
bitstream reader...

> 
>> +
>>      /* 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

Yes, it could.

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

It won't always be zero once I add some checks for header validity.
...one patch at a time. :)  but I guess I could leave it void until then.

> 
>>  }
>>  
>>  /**
>> @@ -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?

Yes. Since the bitstream reader isn't actually used, its internal buffer
needs to be moved forward to skip the streaminfo header bytes.


I'll make the appropriate changes and submit new patch(es).

Thanks,
Justin