[FFmpeg-devel] [PATCH 04/10] avcodec/vc1: Introduce fast path for unescaping bitstream buffer

[Martin Storsjö]

On Fri, 25 Mar 2022, Ben Avison wrote:

> void ff_vc1dsp_init(VC1DSPContext* c);
> diff --git a/tests/checkasm/vc1dsp.c b/tests/checkasm/vc1dsp.c
> index 0823ccad31..0ab5892403 100644
> --- a/tests/checkasm/vc1dsp.c
> +++ b/tests/checkasm/vc1dsp.c
> @@ -286,6 +286,20 @@ static matrix *generate_inverse_quantized_transform_coefficients(size_t width, s
>         }                                                                   \
>     } while (0)
>
> +#define TEST_UNESCAPE                                                                                   \
> +    do {                                                                                            \
> +        for (int count = 100; count > 0; --count) {                                                 \
> +            escaped_offset = rnd() & 7;                                                             \
> +            unescaped_offset = rnd() & 7;                                                           \
> +            escaped_len = (1u << (rnd() % 8) + 3) - (rnd() & 7);                                    \
> +            RANDOMIZE_BUFFER8(unescaped, UNESCAPE_BUF_SIZE);                                        \

The output buffer will be overwritten in the end, but I guess this 
initialization is useful for making sure that the test doesn't 
accidentally rely on the output from the previous iteration, right?

> +            len0 = call_ref(escaped0 + escaped_offset, escaped_len, unescaped0 + unescaped_offset); \
> +            len1 = call_new(escaped1 + escaped_offset, escaped_len, unescaped1 + unescaped_offset); \
> +            if (len0 != len1 || memcmp(unescaped0, unescaped1, len0))                               \

Don't you need to include unescaped_offset here too? Otherwise you're just 
checking areas of the buffer that wasn't necessarily written.


> +                fail();                                                                             \
> +        }                                                                                           \
> +    } while (0)
> +

As with the rest of the checkasm tests - please unmacro most things where 
possible (except for the RANDOMIZE_* macros, those are ok to keep macroed 
if you want to). And sorry for leading you down a path with a bad example 
in that respect.

> void checkasm_check_vc1dsp(void)
> {
>     /* Inverse transform input coefficients are stored in a 16-bit buffer
> @@ -309,6 +323,14 @@ void checkasm_check_vc1dsp(void)
>     LOCAL_ALIGNED_4(uint8_t, filter_buf0, [24 * 24]);
>     LOCAL_ALIGNED_4(uint8_t, filter_buf1, [24 * 24]);
>
> +    /* This appears to be a typical length of buffer in use */
> +#define LOG2_UNESCAPE_BUF_SIZE 17
> +#define UNESCAPE_BUF_SIZE (1u<<LOG2_UNESCAPE_BUF_SIZE)
> +    LOCAL_ALIGNED_8(uint8_t, escaped0, [UNESCAPE_BUF_SIZE]);
> +    LOCAL_ALIGNED_8(uint8_t, escaped1, [UNESCAPE_BUF_SIZE]);
> +    LOCAL_ALIGNED_8(uint8_t, unescaped0, [UNESCAPE_BUF_SIZE]);
> +    LOCAL_ALIGNED_8(uint8_t, unescaped1, [UNESCAPE_BUF_SIZE]);
> +
>     VC1DSPContext h;
>
>     ff_vc1dsp_init(&h);
> @@ -349,4 +371,41 @@ void checkasm_check_vc1dsp(void)
>     CHECK_LOOP_FILTER(vc1_h_loop_filter16);
>
>     report("loop_filter");
> +
> +    if (check_func(h.vc1_unescape_buffer, "vc1dsp.vc1_unescape_buffer")) {
> +        int len0, len1, escaped_offset, unescaped_offset, escaped_len;
> +        declare_func_emms(AV_CPU_FLAG_MMX, int, const uint8_t *, int, uint8_t *);
> +
> +        /* Test data which consists of escapes sequences packed as tightly as possible */
> +        for (int x = 0; x < UNESCAPE_BUF_SIZE; ++x)
> +            escaped1[x] = escaped0[x] = 3 * (x % 3 == 0);
> +        TEST_UNESCAPE;
> +
> +        /* Test random data */
> +        RANDOMIZE_BUFFER8(escaped, UNESCAPE_BUF_SIZE);
> +        TEST_UNESCAPE;
> +
> +        /* Test data with escape sequences at random intervals */
> +        for (int x = 0; x <= UNESCAPE_BUF_SIZE - 4;) {
> +            int gap, gap_msb;
> +            escaped1[x+0] = escaped0[x+0] = 0;
> +            escaped1[x+1] = escaped0[x+1] = 0;
> +            escaped1[x+2] = escaped0[x+2] = 3;
> +            escaped1[x+3] = escaped0[x+3] = rnd() & 3;
> +            gap_msb = 2u << (rnd() % 8);
> +            gap = (rnd() &~ -gap_msb) | gap_msb;
> +            x += gap;
> +        }
> +        TEST_UNESCAPE;
> +
> +        /* Test data which is known to contain no escape sequences */
> +        memset(escaped0, 0xFF, UNESCAPE_BUF_SIZE);
> +        memset(escaped1, 0xFF, UNESCAPE_BUF_SIZE);
> +        TEST_UNESCAPE;
> +
> +        /* Benchmark the no-escape-sequences case */
> +        bench_new(escaped1, UNESCAPE_BUF_SIZE, unescaped1);
> +    }
> +
> +    report("unescape_buffer");
> }

The test looks great otherwise! But please split the code for it into a 
standalonef unction, e.g. check_unescape(), so the main 
checkasm_check_vc1dsp() just is a list of calls to check_loopfilter(), 
check_idct(), check_unescape() etc.

// Martin