Re: [FFmpeg-devel] [PATCH 02/10] checkasm: Add vc1dsp inverse transform tests

["Martin Storsjö" <martin@martin.st>]

On Fri, 25 Mar 2022, Ben Avison wrote:

> This test deliberately doesn't exercise the full range of inputs described in
> the committee draft VC-1 standard. It says:
>
> input coefficients in frequency domain, D, satisfy   -2048 <= D < 2047
> intermediate coefficients, E, satisfy                -4096 <= E < 4095
> fully inverse-transformed coefficients, R, satisfy    -512 <= R <  511
>
> For one thing, the inequalities look odd. Did they mean them to go the
> other way round? That would make more sense because the equations generally
> both add and subtract coefficients multiplied by constants, including powers
> of 2. Requiring the most-negative values to be valid extends the number of
> bits to represent the intermediate values just for the sake of that one case!
>
> For another thing, the extreme values don't look to occur in real streams -
> both in my experience and supported by the following comment in the AArch32
> decoder:
>
>    tNhalf is half of the value of tN (as described in vc1_inv_trans_8x8_c).
>    This is done because sometimes files have input that causes tN + tM to
>    overflow. To avoid this overflow, we compute tNhalf, then compute
>    tNhalf + tM (which doesn't overflow), and then we use vhadd to compute
>    (tNhalf + (tNhalf + tM)) >> 1 which does not overflow because it is
>    one instruction.
>
> My AArch64 decoder goes further than this. It calculates tNhalf and tM
> then does an SRA (essentially a fused halve and add) to compute
> (tN + tM) >> 1 without ever having to hold (tNhalf + tM) in a 16-bit element
> without overflowing. It only encounters difficulties if either tNhalf or
> tM overflow in isolation.
>
> I haven't had sight of the final standard, so it's possible that these
> issues were dealt with during finalisation, which could explain the lack
> of usage of extreme inputs in real streams. Or a preponderance of decoders
> that only support 16-bit intermediate values in their inverse transforms
> might have caused encoders to steer clear of such cases.
>
> I have effectively followed this approach in the test, and limited the
> scale of the coefficients sufficient that both the existing AArch32 decoder
> and my new AArch64 decoder both pass.
>
> Signed-off-by: Ben Avison <bavison@riscosopen.org>
> ---
> tests/checkasm/vc1dsp.c | 258 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 258 insertions(+)

The reasoning sounds sensible to me.

I didn't try to follow the exact logic for how the input data is produced, 
but it seems reasonable.

It'd be nice to unmacro the function and wrap it in a separate standalone 
test function like check_idct() in vp8dsp, check_itxfm in vp9dsp or 
check_idct in hevc_idct.c. You may want to have a deeply nested loop to 
check e.g.

     for (int w = 4; w <= 8; w += 4) {
         for (int h = 4; w <= 8; w += 4) {
             for (int dc = 0; dc <= 1; dc++) {
                 if (w == 8 && h == 8 && dc == 0)
                     continue; // Tested separately
                 [... actual test ...]
                 (or call a separate check_idct_func(w,h,dc) function
                 to avoid unnecessarily deep indentation of a lot of code)
             }
         }
     }

// Martin

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".