[FFmpeg-devel] [PATCH] avcodec/wmalosslessdec: real 24bit support

[Christophe Gisquet]

2016-04-14 7:56 GMT+02:00 Christophe Gisquet <christophe.gisquet at gmail.com>:
> As Hendrik said, please move it to LLAudDSPContext.

So this, and some of my other comments, are already addressed by the
final patch which was pushed before my review, making it somewhat
moot.

Could you please send a message next time indicating it was (already) pushed?
I suppose the OK was made through another medium that this mailing
list, so can the OK'er and/or the pusher mentions the OK'ing next
time?

> On a side note, is this through RE or guess (I'm asking because the
> guess was not difficult) ?
>
> Because having an accumulator on 32 bits only may lead to overflows.

I saw a 30+M value at some point. I haven't seen anything else than 8
coeffs in use, so maybe 32 bits is enough and wrap-around is minimal.

But I'm still wondering what the reason is for this 32bits accumulator.

>> -               sizeof(int16_t) * order * num_channels);
>> +               sizeof(int) * order * num_channels);
>
> The format has the bitdepth stored and put into s->bits_per_sample.
> The decoder actually uses it to select how to store the samples later
> on.
> In any such case, this should be dynamic. Either you use int size =
> s->bits_per_sample>16 ? 4 : 2 (because sizeof(int16_t isn't going to
> change much...)
>
> Or FFALIGN(s->bits_per_sample>>3, 2)? Whatever floats your boat.

So it's a lot more involved than this, and the final code is not the
nicest looking.

>> FFALIGN(s->cdlms[ch][ilms].order,
>> -
>> WMALL_COEFF_PAD_SIZE),

Just noticed that a lower value is likely needed - the reason for this
is to allow treating even 1 additional coeff with another loop,
thereby requiring some padding. But the padding is more linked to
overread size than number of coefficient.

I'll post a patch series soon on this topic.

-- 
Christophe