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

[Michael Niedermayer]

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>