[FFmpeg-devel] Patch - Allow disabling of bit reservoir when encoding MP3 audio
Michael Niedermayer
michaelni
Sun Feb 10 21:11:00 CET 2008
On Sun, Feb 10, 2008 at 07:16:41PM +0000, Paul Kelly wrote:
> On Sat, 9 Feb 2008, Paul Kelly wrote:
>
>> On Sat, 9 Feb 2008, Michael Niedermayer wrote:
>>
>>> On Sat, Feb 09, 2008 at 03:28:16PM +0000, Paul Kelly wrote:
>>>> On Fri, 8 Feb 2008, Michael Niedermayer wrote:
>>>>
>>>>> On Fri, Feb 08, 2008 at 09:57:47PM +0000, Paul Kelly wrote:
>>>>>
>>>> [...]
>>>>> 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
>>>>>
>>>> [...]
>>>>>> 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 ...
>>>>
>>>> OK, I've attached two patches for consideration. The first,
>>>> flags2.patch,
>>>> re-arranges the code for setting WMA flags in wmaenc.c to make it
>>>> clearer.
>>>> It seemed to be a copy and paste of the code in wmadec.c, otherwise it
>>>> doesn't make much sense. It should be clear from the patch my
>>>> understanding
>>>> of what the code is supposed to do, and I hope my changes are
>>>> acceptable.
>>>> This patch also adds the warning if the bit reservoir flag is set, and
>>>> unsets it.
>>>
>>> Rearanging is a cosmetical change and must be in a seperate patch from
>>> functional changes.
>>
>> But it is - are you saying the change to add the error message should go
>> in the second patch? I thought, since the value of use_bit_reservoir is
>> hard-coded to 0 - the warning will never be printed, so there is no change
>> in functionality.
>> But yes you have a point. I'll send another version tomorrow!
>
> Here it is now. The first patch only rearranges the flag-setting code in
> wmaenc.c and does not add anything.
> The second patch adds the new flag,
> sets it as the default but also adds the warning in wmaenc.c that it will
> not be used. Is this OK?
The second patch is ok with the exception of the wmaenc change
the first as well as the wmaenc change of the second is a bloated mess
Code must always be as simple as possible for what it does, that is
if(flag set) print warning. To do that one needs at a maximum 2 lines of
code. You add 11:
> Index: libavcodec/wmaenc.c
> ===================================================================
> --- libavcodec/wmaenc.c (revision 11901)
> +++ libavcodec/wmaenc.c (working copy)
> @@ -41,7 +41,16 @@
>
> /* extract flag infos */
> flags1 = 0;
> - flags2 = 1;
> + flags2 = 0;
> + s->use_exp_vlc = 1;
> + s->use_bit_reservoir = 0;
> + s->use_variable_block_len = 0;
> + if(s->use_exp_vlc)
> + flags2 |= 0x0001;
> + if(s->use_bit_reservoir)
> + flags2 |= 0x0002;
> + if(s->use_variable_block_len)
> + flags2 |= 0x0004;
> if (avctx->codec->id == CODEC_ID_WMAV1) {
> extradata= av_malloc(4);
> avctx->extradata_size= 4;
> @@ -55,9 +64,6 @@
> }else
> assert(0);
> avctx->extradata= extradata;
> - s->use_exp_vlc = flags2 & 0x0001;
> - s->use_bit_reservoir = flags2 & 0x0002;
> - s->use_variable_block_len = flags2 & 0x0004;
>
> ff_wma_init(avctx, flags2);
>
[...]
> Index: libavcodec/wmaenc.c
> ===================================================================
> --- libavcodec/wmaenc.c~ 2008-02-10 18:38:12.000000000 +0100
> +++ libavcodec/wmaenc.c 2008-02-10 18:37:58.000000000 +0100
> @@ -43,8 +43,13 @@
> flags1 = 0;
> flags2 = 0;
> s->use_exp_vlc = 1;
> - s->use_bit_reservoir = 0;
> + s->use_bit_reservoir = avctx->flags2 & CODEC_FLAG2_BIT_RESERVOIR;
> s->use_variable_block_len = 0;
> + if(s->use_bit_reservoir) {
> + /* Remove this when bit reservoir usage is implemented */
> + av_log(avctx, AV_LOG_INFO, "Bit reservoir not yet implemented for WMA encoder; disabling.\n");
> + s->use_bit_reservoir = 0;
> + }
> if(s->use_exp_vlc)
> flags2 |= 0x0001;
> if(s->use_bit_reservoir)
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/20080210/b19cc2e7/attachment.pgp>
More information about the ffmpeg-devel
mailing list