[FFmpeg-devel] [PATCH] WMA Voice decoder

[Reimar Döffinger]

On Tue, Feb 09, 2010 at 03:01:18PM -0500, Ronald S. Bultje wrote:
> On Mon, Feb 1, 2010 at 2:35 PM, Reimar D?ffinger
> <Reimar.Doeffinger at gmx.de> wrote:
> > On Sun, Jan 31, 2010 at 09:46:43PM -0500, Ronald S. Bultje wrote:
> >> On Sat, Jan 30, 2010 at 6:48 PM, Reimar D?ffinger
> >> <Reimar.Doeffinger at gmx.de> wrote:
> >> > "short"? Also specifying the type only once is ugly.
> >> > Also if you use frame_size with a shift they all can be uint8_t
> >> > (though probably it's better to just "waste" those 34 bytes and
> >> > leave frame_size 16 bits, but the others don't need to be.
> >>
> >> OK, so I changed this to be holdable in 32bits per frame type, maybe
> >> that's a bit too much just let me know how far I should go.
> >
> > I'm generally in favour of the most readable, at least as long as we
> > are talking about saving a few _bytes_, if it was a kB that'd be different.
> 
> (Dis)assembly-readable or source-readable? I mean, readability is not
> affected by me adding a :3 to the field.

Source readability.
You're right it's no real issue with bitfields assuming you don't mind the other
issues like
1) gcc possibly generating code for accessing it that is even more inefficient
than you can possibly imagine
2) incompatibility between compilers (should be no issue here)
3) I don't trust gcc to have sufficient test cases for bitstream handling.
Anyway this is way beyond bikeshed already, just using 4 bytes for such
small values is maybe going a bit too far.

> > Um, the thing is that lsps and base_q is the same type, if you don't
> > add a restrict keyword the compiler usually simply _can't_ know that
> > lsps[m] is not the same as base_q[n] and thus _has_ to reload
> > base_q[n] each time.
> > I agree though that possibly the better solution is to add restrict (even
> > though I consider gcc stupid enough that I expect at least some version
> > to still get it wrong).
> 
> Aha, I just learned what restrict is. So I only have to add restrict
> to the three arguments here (also since everything else is const),
> right?
> 
> static void dequant_lsps(double *restrict lsps, int num,
>                          const uint16_t *values,
>                          const uint16_t *sizes,
>                          int n_stages, const uint8_t *table,
>                          const double *restrict mul_q,
>                          const double *restrict base_q)

I think so, yes. Only way to really know is looking at the generated assembler
code though...