[FFmpeg-devel] [PATCH] WMA: use type punning and unroll loops in decode_exp_vlc()

[Måns Rullgård]

Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:

> On Tue, Sep 29, 2009 at 01:51:52PM +0100, M?ns Rullg?rd wrote:
>> Reimar D?ffinger <Reimar.Doeffinger at gmx.de> writes:
>> 
>> > On Tue, Sep 29, 2009 at 11:40:36AM +0100, M?ns Rullg?rd wrote:
>> >> Michael Niedermayer <michaelni at gmx.at> writes:
>> >> 
>> >> > On Tue, Sep 29, 2009 at 09:10:49AM +0100, M?ns Rullg?rd wrote:
>> >> >> Mans Rullgard <mans at mansr.com> writes:
>> >> >> 
>> >> >> > GCC does stupid things if these assignments are done using floats
>> >> >> > directly, so fill the runs using integer operations instead.  Also
>> >> >> > unroll the loops since the length is always a multiple of 4.
>> >> >> > ---
>> >> >> >  libavcodec/wmadec.c |   22 ++++++++++++++++------
>> >> >> >  1 files changed, 16 insertions(+), 6 deletions(-)
>> >> >> >
>> >> >> > +        iv = AV_RN32(ptab + last_exp);
>> >> >> 
>> >> >> I don't know what I was thinking when I wrote that.  I've changed it
>> >> >> to use a uint32_t pointer instead.
>> >> >
>> >> > float->uint32_t is ok
>> >> 
>> >> Those patches applied.
>> >
>> > I'd appreciate if you'd run make test before such extensive changes.
>> > Then you'd have noticed that n can be 0 in the loops you unrolled, which
>> > causes a crash.
>> 
>> Fixed.  The value can not be zero, or the old code was broken already.
>> However, it could apparently be a non-multiple of 4.  I must have
>> missed some place where those values are computed.
>
> Well, maybe that was a side-effect of whatever the bug was.
>
>> Always more fun complaining after the fact than reviewing patches,
>> isn't it?
>
> No, it's always particular fun when you want to prepare a patch, see
> make test failing and after debugging find out that someone else just
> broke it, then you either have to wait for it to be fixed, fix it
> yourself, or disable the test and update the regression ref.
> _If_ (what I don't know) that is because someone couldn't be bothered to
> run make test I sure think I have every right to complain about wasting
> my time.

I do share your sentiment, and I take the blame for this one.  I'm
just annoyed at how hard it is to get anyone to so much as look at a
patch, yet when you commit something, *then* all kinds of people are
more than happy to tell you why it's wrong on cvslog.

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