[FFmpeg-devel] [PATCH] use MUL64 in ac3dec.c

[Justin Ruggles]

Reimar D?ffinger wrote:

> On Mon, Jan 11, 2010 at 11:11:43PM +0000, M?ns Rullg?rd wrote:
>> Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
>>> Even faster (though possibly wrong, even though I hear no issues with
>>> my samples) should be the variant with MULH, though I could not really
>>> measure a difference:
>> Make sure the output is actually identical.
>>
>>> Index: libavcodec/ac3dec.c
>>> ===================================================================
>>> --- libavcodec/ac3dec.c	(revision 21153)
>>> +++ libavcodec/ac3dec.c	(working copy)
>>> @@ -420,10 +420,9 @@
>>>          int band_end = bin + s->cpl_band_sizes[band];
>>>          for (ch = 1; ch <= s->fbw_channels; ch++) {
>>>              if (s->channel_in_cpl[ch]) {
>>> -                int64_t cpl_coord = s->cpl_coords[ch][band];
>>> +                int cpl_coord = s->cpl_coords[ch][band] << 9;
>> Is this certain not to overflow?
> 
> No idea, I just wanted to mention it. For me it is not any faster so I don't
> intend to investigate that variant further - it was just in case someone here
> knows the limits for these variables right out of their head.


The shift can overflow.  The maximum value for cpl_coords is 31<<21.

> 
>>>                  for (bin = band_start; bin < band_end; bin++) {
>>> -                    s->fixed_coeffs[ch][bin] = ((int64_t)s->fixed_coeffs[CPL_CH][bin] *
>>> -                                                cpl_coord) >> 23;
>>> +                    s->fixed_coeffs[ch][bin] = MULH(s->fixed_coeffs[CPL_CH][bin], cpl_coord);
>>>                  }
>> Provided the shift above is safe, that had better work, since the
>> destination is 32-bit, or the original would be suffering some serious
>> truncation problems.
> 
> And if fixed_coeffs can end up being any 32 bit value, that in turn would
> limit cpl_coord enough to make above shift safe. However it is all speculation.

fixed_coeffs original value is 24-bit signed, but the final value can be
as large as cpl_coords.  I had honestly never looked at the maximum
value of this before, and it may have other implications in the code
that I will need to investigate.

Your first patch looks fine though.

-Justin