Re: [FFmpeg-devel] [PATCH 04/10] avcodec/vc1: Introduce fast path for unescaping bitstream buffer
On Thu, 31 Mar 2022, Ben Avison wrote: On 29/03/2022 21:37, Martin Storsjö wrote: On Fri, 25 Mar 2022, Ben Avison wrote: 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. Oh, right - yes, call_ref and call_new need to be in the same scope as declare_func, yes. 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). Awesome, thanks! // 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-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 04/10] avcodec/vc1: Introduce fast path for unescaping bitstream buffer
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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 04/10] avcodec/vc1: Introduce fast path for unescaping bitstream buffer
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<
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-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 04/10] avcodec/vc1: Introduce fast path for unescaping bitstream buffer
Includes a checkasm test.
Signed-off-by: Ben Avison
---
libavcodec/vc1dec.c | 20 +++---
libavcodec/vc1dsp.c | 2 ++
libavcodec/vc1dsp.h | 3 +++
tests/checkasm/vc1dsp.c | 59 +
4 files changed, 74 insertions(+), 10 deletions(-)
diff --git a/libavcodec/vc1dec.c b/libavcodec/vc1dec.c
index 1c92b9d401..6a30b5b664 100644
--- a/libavcodec/vc1dec.c
+++ b/libavcodec/vc1dec.c
@@ -490,7 +490,7 @@ static av_cold int vc1_decode_init(AVCodecContext *avctx)
size = next - start - 4;
if (size <= 0)
continue;
-buf2_size = vc1_unescape_buffer(start + 4, size, buf2);
+buf2_size = v->vc1dsp.vc1_unescape_buffer(start + 4, size, buf2);
init_get_bits(, buf2, buf2_size * 8);
switch (AV_RB32(start)) {
case VC1_CODE_SEQHDR:
@@ -680,7 +680,7 @@ static int vc1_decode_frame(AVCodecContext *avctx, void
*data,
case VC1_CODE_FRAME:
if (avctx->hwaccel)
buf_start = start;
-buf_size2 = vc1_unescape_buffer(start + 4, size, buf2);
+buf_size2 = v->vc1dsp.vc1_unescape_buffer(start + 4, size,
buf2);
break;
case VC1_CODE_FIELD: {
int buf_size3;
@@ -697,8 +697,8 @@ static int vc1_decode_frame(AVCodecContext *avctx, void
*data,
ret = AVERROR(ENOMEM);
goto err;
}
-buf_size3 = vc1_unescape_buffer(start + 4, size,
-slices[n_slices].buf);
+buf_size3 = v->vc1dsp.vc1_unescape_buffer(start + 4, size,
+
slices[n_slices].buf);
init_get_bits([n_slices].gb, slices[n_slices].buf,
buf_size3 << 3);
slices[n_slices].mby_start = avctx->coded_height + 31 >> 5;
@@ -709,7 +709,7 @@ static int vc1_decode_frame(AVCodecContext *avctx, void
*data,
break;
}
case VC1_CODE_ENTRYPOINT: /* it should be before frame data */
-buf_size2 = vc1_unescape_buffer(start + 4, size, buf2);
+buf_size2 = v->vc1dsp.vc1_unescape_buffer(start + 4, size,
buf2);
init_get_bits(>gb, buf2, buf_size2 * 8);
ff_vc1_decode_entry_point(avctx, v, >gb);
break;
@@ -726,8 +726,8 @@ static int vc1_decode_frame(AVCodecContext *avctx, void
*data,
ret = AVERROR(ENOMEM);
goto err;
}
-buf_size3 = vc1_unescape_buffer(start + 4, size,
-slices[n_slices].buf);
+buf_size3 = v->vc1dsp.vc1_unescape_buffer(start + 4, size,
+
slices[n_slices].buf);
init_get_bits([n_slices].gb, slices[n_slices].buf,
buf_size3 << 3);
slices[n_slices].mby_start =
get_bits([n_slices].gb, 9);
@@ -761,7 +761,7 @@ static int vc1_decode_frame(AVCodecContext *avctx, void
*data,
ret = AVERROR(ENOMEM);
goto err;
}
-buf_size3 = vc1_unescape_buffer(divider + 4, buf + buf_size -
divider - 4, slices[n_slices].buf);
+buf_size3 = v->vc1dsp.vc1_unescape_buffer(divider + 4, buf +
buf_size - divider - 4, slices[n_slices].buf);
init_get_bits([n_slices].gb, slices[n_slices].buf,
buf_size3 << 3);
slices[n_slices].mby_start = s->mb_height + 1 >> 1;
@@ -770,9 +770,9 @@ static int vc1_decode_frame(AVCodecContext *avctx, void
*data,
n_slices1 = n_slices - 1;
n_slices++;
}
-buf_size2 = vc1_unescape_buffer(buf, divider - buf, buf2);
+buf_size2 = v->vc1dsp.vc1_unescape_buffer(buf, divider - buf,
buf2);
} else {
-buf_size2 = vc1_unescape_buffer(buf, buf_size, buf2);
+buf_size2 = v->vc1dsp.vc1_unescape_buffer(buf, buf_size, buf2);
}
init_get_bits(>gb, buf2, buf_size2*8);
} else{
diff --git a/libavcodec/vc1dsp.c b/libavcodec/vc1dsp.c
index a29b91bf3d..11d493f002 100644
--- a/libavcodec/vc1dsp.c
+++ b/libavcodec/vc1dsp.c
@@ -34,6 +34,7 @@
#include "rnd_avg.h"
#include "vc1dsp.h"
#include "startcode.h"
+#include "vc1_common.h"
/* Apply overlap transform to horizontal edge */
static void vc1_v_overlap_c(uint8_t *src, int stride)
@@ -1030,6 +1031,7 @@ av_cold void ff_vc1dsp_init(VC1DSPContext *dsp)
#endif /* CONFIG_WMV3IMAGE_DECODER || CONFIG_VC1IMAGE_DECODER */
dsp->startcode_find_candidate
