[FFmpeg-devel] Patch - Allow disabling of bit reservoir when encoding MP3 audio

Michael Niedermayer michaelni
Fri Feb 8 23:48:23 CET 2008

On Fri, Feb 08, 2008 at 09:57:47PM +0000, Paul Kelly wrote:
> On Tue, 5 Feb 2008, Michael Niedermayer wrote:
> > On Tue, Feb 05, 2008 at 04:55:51PM +0000, Paul Kelly wrote:
> >> On Tue, 5 Feb 2008, Michael Niedermayer wrote:
> >>
> >>> On Tue, Feb 05, 2008 at 12:40:18AM +0000, Paul Kelly wrote:
> >> [...]
> >>>> Index: libavcodec/utils.c
> >>>> ===================================================================
> >>>> --- libavcodec/utils.c	(revision 11865)
> >>>> +++ libavcodec/utils.c	(working copy)
> >>>> @@ -762,6 +762,7 @@
> >>>>  {"non_linear_q", "use non linear quantizer", 0, FF_OPT_TYPE_CONST, CODEC_FLAG2_NON_LINEAR_QUANT, INT_MIN, INT_MAX, V|E, "flags2"},
> >>>>  {"request_channels", "set desired number of audio channels", OFFSET(request_channels), FF_OPT_TYPE_INT, DEFAULT, 0, INT_MAX, A|D},
> >>>>  {"drc_scale", "percentage of dynamic range compression to apply", OFFSET(drc_scale), FF_OPT_TYPE_FLOAT, 1.0, 0.0, 1.0, A|D},
> >>>> +{"disable_reservoir", "disable bit reservoir (libmp3lame)", 0, FF_OPT_TYPE_CONST, CODEC_FLAG2_DISABLE_RESERVOIR, INT_MIN, INT_MAX, A|E, "flags2"},
> >>>
> >>> The mentioning of libmp3lame is not appropriate. The flag is not specific to
> >>> lame.
> >>
> >> If the mention was removed from there and from the comment against the
> >> list of flags in avcodec.h, would the patch be acceptable? I take on board
> >
> > Iam not sure if a disable, or enable flag or maybe even a int bit_reservoir
> > size would be best. Besdides this, yes i would like to have such a thing.
> Well, as using a bit reservoir is common practice (for MP3 anyway) and 
> produces higher quality, it seems to me that it should stay the default. 
> Hence a flag to disable it would be more appropriate than one to enable 
> it.

Everything else is disabled by default and has enable flags.
I definitly want the flag to be an enable flag. The default is a seperate
issue, we might choose to leave it enabled.
"-flags2 -reservoir" would disable it
"-flags2 +reservoir" would enable it
this is much cleaner than a +disable_reservoir

> As for a bit reservoir size, it is beyond my understanding to know whether 
> that would be useful. As I understand it, the size of the bit reservoir 
> continually varies (presumably that's why it's called a reservoir) and 
> there is a set limit (part of the codec spec?) as to how big it is allowed 
> to grow (i.e. how many bits of the current frame are allowed to be encoded 
> in previous frames). The use case for why it's useful to turn it off 
> completely is to be able to cleanly split the audio between frames - I 
> can't see how being able to vary the maximum size would be useful in a 
> practical context, apart from theoretical experiments to measure the 
> difference in quality. We don't need to support those, right?

If the codec would allow large reservoirs (which mp3 doesnt) then the
size would be a tradeoff between quality and general problems. If there
would be an hour of data in the reservoir then seeking would be really
funny as you first would need to read 1 hour of mp3 before you had
a header with matching data to decode.

> So I am still in favour of a flag to disable it. This could be used by MP3 
> and WMA. As far as I can see the WMA encoder is currently hard-coded not 
> to attempt to use the bit reservoir - it could be changed to use the flag 
> in AVCodecContext but then the problem would be that there is an assert() 
> that will fail, because bit reservoir usage is not implemented in the 
> encoder.

> So I'm not sure what to do about the WMA encoder. Would it be acceptable 
> to put in some code that prints a warning that the bit reservoir will not 
> be used even though it is specified? I mean just so there is some 
> reference to this AVCodecContext disable bit reservoir flag there so 
> someone hacking the code in the future will be reminded to use it. 

If you want to add that ...

Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- 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/20080208/9a611d1a/attachment.pgp>

More information about the ffmpeg-devel mailing list