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

[Ben Avison]

On 29/03/2022 21:37, Martin Storsjö wrote:
> On Fri, 25 Mar 2022, Ben Avison wrote:
>> +#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?

The main idea was to catch examples of writing to the buffer beyond the 
length reported (and less likely, writes before the start of the 
buffer). I suppose it's possible that someone might want to deliberately 
overwrite in specific conditions, but the test could always be loosened 
up at that point once those conditions become clearer.

>> +            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.

I realise I should have made the memcmp length UNESCAPE_BUF_SIZE here to 
achieve what I intended. Testing len0 bytes from the start of the buffer 
neither checks all the written bytes nor checks the byte after those 
written :-$

> 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).

In the case of TEST_UNESCAPE, I think it has to remain as a macro, 
otherwise the next function up ends up with a declare_func_emms() and a 
bench_new() but no call_ref() or call_new(), which means some builds end 
up with an unused function warning.

I can, however, split all the unescape tests out of 
checkasm_check_vc1dsp into a separate function (and separate functions 
for inverse-transform and deblocking tests).

Ben