[FFmpeg-devel] [PATCH] new function get_sbits_long()

[Måns Rullgård]

Justin Ruggles <justin.ruggles at gmail.com> writes:

> Michael Niedermayer wrote:
>> On Mon, Mar 02, 2009 at 07:38:09PM -0500, Justin Ruggles wrote:
>>> M?ns Rullg?rd wrote:
>>>> Justin Ruggles <justin.ruggles at gmail.com> writes:
>>>>
>>>>> Reimar D?ffinger wrote:
>>>>>> On Mon, Mar 02, 2009 at 01:35:45PM -0500, Justin Ruggles wrote:
>>>>>>> Reimar D?ffinger wrote:
>>>>>>>> On Mon, Mar 02, 2009 at 06:58:49PM +0100, Michael Niedermayer wrote:
>>>>>>>>> On Mon, Mar 02, 2009 at 12:39:05PM -0500, Justin Ruggles wrote:
>>>>>>>>>> I need a get_sbits_long() function to support FLAC files that are more
>>>>>>>>>> than 24-bit.  There might be a better way to do it, but this works.
>>>>>>>>> only where int is 32bit which isnt guranteed even if likely
>>>>>>>> There is already the NEG_SSR32 macro to take care of that (and it
>>>>>>>> also optimizes gcc's brain-deadness away on x86).
>>>>>>> yes, this works well.
>>>>>>>
>>>>>>> -Justin
>>>>>>>
>>>>>>> Index: libavcodec/bitstream.h
>>>>>>> ===================================================================
>>>>>>> --- libavcodec/bitstream.h	(revision 17735)
>>>>>>> +++ libavcodec/bitstream.h	(working copy)
>>>>>>> @@ -707,6 +707,14 @@
>>>>>>>  }
>>>>>>>  
>>>>>>>  /**
>>>>>>> + * reads 0-32 bits as a signed integer.
>>>>>>> + */
>>>>>>> +static inline int get_sbits_long(GetBitContext *s, int n) {
>>>>>>> +    int val = get_bits_long(s, n);
>>>>>>> +    return NEG_SSR32(val, n);
>>>>>> Can't imagine that, you must use it like in the SHOW_SBITS macro:
>>>>>> NEG_SSR32(val<<(32-n), n)
>>>>> You're right. The FLAC file I tested just happened not to trigger any
>>>>> problem.  I tried another one.
>>>>>
>>>>> -Justin
>>>>>
>>>>>
>>>>> Index: libavcodec/bitstream.h
>>>>> ===================================================================
>>>>> --- libavcodec/bitstream.h	(revision 17735)
>>>>> +++ libavcodec/bitstream.h	(working copy)
>>>>> @@ -707,6 +707,14 @@
>>>>>  }
>>>>>  
>>>>>  /**
>>>>> + * reads 0-32 bits as a signed integer.
>>>>> + */
>>>>> +static inline int get_sbits_long(GetBitContext *s, int n) {
>>>>> +    int val = get_bits_long(s, n);
>>>>> +    return NEG_SSR32(val<<(32-n), n);
>>>>> +}
>>>> This is obfuscation IMO.  I'll prepare a patch adding a sign extend
>>>> function if nobody beats me to it.  What is the preferred return type,
>>>> int or int32_t?  I'd say int, but maybe there's a good reason for
>>>> something else.
>>> Thanks M?ns. Here is a patch using the new function in mathops.h.
>> 
>> was there another file that needs get_sbits_long() ? if not this code
>> belongs in flac*
>
> It could be used in 1 place in alac.c:547:
> audiobits = get_bits_long(&alac->gb, alac->setinfo_sample_size);
> audiobits = extend_sign32(audiobits, alac->setinfo_sample_size);
>
> additionally, that extend_sign32() function, which is also used in other
> places in alac.c, could be replaced by calls to sign_extend() in mathops.h.

I posted a patch for that a while ago.

-- 
M?ns Rullg?rd
mans at mansr.com