[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