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

[Paul Kelly]

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.

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?

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. Or is 
that not needed?

Paul