From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 97452426E4 for ; Tue, 29 Mar 2022 20:37:57 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 9363A68B159; Tue, 29 Mar 2022 23:37:54 +0300 (EEST) Received: from mail8.parnet.fi (mail8.parnet.fi [77.234.108.134]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 589C068A6F6 for ; Tue, 29 Mar 2022 23:37:48 +0300 (EEST) Received: from mail9.parnet.fi (mail9.parnet.fi [77.234.108.21]) by mail8.parnet.fi with ESMTP id 22TKblcK002132-22TKblcL002132; Tue, 29 Mar 2022 23:37:47 +0300 Received: from foo.martin.st (host-97-187.parnet.fi [77.234.97.187]) by mail9.parnet.fi (Postfix) with ESMTPS id 9AAF1A143A; Tue, 29 Mar 2022 23:37:47 +0300 (EEST) Date: Tue, 29 Mar 2022 23:37:47 +0300 (EEST) From: =?ISO-8859-15?Q?Martin_Storsj=F6?= To: FFmpeg development discussions and patches In-Reply-To: <20220325185257.513933-5-bavison@riscosopen.org> Message-ID: <182b9cbc-9fde-c281-be72-4ec5e14bf79c@martin.st> References: <20220317185819.466470-1-bavison@riscosopen.org> <20220325185257.513933-1-bavison@riscosopen.org> <20220325185257.513933-5-bavison@riscosopen.org> MIME-Version: 1.0 X-FE-Policy-ID: 3:14:2:SYSTEM Subject: Re: [FFmpeg-devel] [PATCH 04/10] avcodec/vc1: Introduce fast path for unescaping bitstream buffer X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: Ben Avison Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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< + 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 _______________________________________________ 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".