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

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> 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*

Even if there isn't one now, I think this function belongs in
bitstream.h.  It is the logical place for it to be, and it should be
easy to find should someone need it in the future, which it not a
far-fetched prospect.

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