> On Jun 16, 2025, at 19:03, Andreas Rheinhardt
> <andreas.rheinha...@outlook.com> wrote:
>
> Zhao Zhili:
>>
>>
>>> On Jun 16, 2025, at 17:46, Andreas Rheinhardt
>>> <andreas.rheinha...@outlook.com> wrote:
>>>
>>> Zhao Zhili:
>>>> From: Zhao Zhili <zhiliz...@tencent.com>
>>>>
>>>> ---
>>>> tests/checkasm/h264dsp.c | 20 +++++++++++++++-----
>>>> 1 file changed, 15 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/tests/checkasm/h264dsp.c b/tests/checkasm/h264dsp.c
>>>> index f5f9650224..a0f8fd858a 100644
>>>> --- a/tests/checkasm/h264dsp.c
>>>> +++ b/tests/checkasm/h264dsp.c
>>>> @@ -328,25 +328,35 @@ static void check_idct_multiple(void)
>>>> static void check_idct_dequant(void)
>>>> {
>>>> static const int depths[5] = { 8, 9, 10, 12, 14 };
>>>> - LOCAL_ALIGNED_16(int16_t, src, [16]);
>>>> - /* Ensure dst buffers are large enough to hold dctcoefs of all
>>>> bit-depths. */
>>>> + /* Ensure buffers are large enough to hold dctcoefs of all
>>>> bit-depths. */
>>>> + LOCAL_ALIGNED_16(uint8_t, src_buf, [16 * sizeof(int32_t)]);
>>>> LOCAL_ALIGNED_16(uint8_t, dst0, [16 * 16 * sizeof(int32_t)]);
>>>> LOCAL_ALIGNED_16(uint8_t, dst1, [16 * 16 * sizeof(int32_t)]);
>>>> + int16_t *src = (int16_t *)src_buf;
>>>> int16_t *dst_ref = (int16_t *)dst0;
>>>> int16_t *dst_new = (int16_t *)dst1;
>>>> H264DSPContext h;
>>>> int bit_depth, i, qmul;
>>>> declare_func_emms(AV_CPU_FLAG_MMX | AV_CPU_FLAG_SSE2, void, int16_t
>>>> *output, int16_t *input, int qmul);
>>>>
>>>> - for (int j = 0; j < 16; j++)
>>>> - src[j] = (rnd() % 512) - 256;
>>>> -
>>>> qmul = rnd() % 4096;
>>>>
>>>> for (i = 0; i < FF_ARRAY_ELEMS(depths); i++) {
>>>> bit_depth = depths[i];
>>>> ff_h264dsp_init(&h, bit_depth, 1);
>>>>
>>>> + if (bit_depth == 8) {
>>>> + for (size_t j = 0; j < 16; j++) {
>>>> + int16_t r = (rnd() % 512) - 256;
>>>> + AV_WN16A(&src_buf[j << 1], r);
>>>> + }
>>>> + } else {
>>>> + for (size_t j = 0; j < 16; j++) {
>>>> + int32_t r = (rnd() % (1 << (bit_depth + 1))) - (1 <<
>>>> bit_depth);
>>>> + AV_WN32A(&src_buf[j << 2], r);
>>>> + }
>>>> + }
>>>> +
>>>> memset(dst0, 0, 16 * 16 * SIZEOF_COEF);
>>>> memset(dst1, 0, 16 * 16 * SIZEOF_COEF);
>>>>
>>>
>>> This still has an effective-type violation: src_buf is of type uint8_t,
>>> yet the ff_h264_luma_dc_dequant_idct functions will read it as
>>> int16_t/int32_t. It also still has the downside that buffer overflows
>>> for the 8bit case can go undetected.
>>
>> A bunch of template has cast like
>>
>> pixel *dst = (pixel *)_dst;
>> const pixel *src = (const pixel *)_src;
>>
>> then read and write as int16_t.
>>
>> And a bunch of checkasm use uint8_t[] array on stack as src and dst,
>> which leading to UB.
>>
>> This patch isn’t specific. And this patch add zero UB (it’s there before the
>> patch,
>> both src and dst are accessed as int32_t/int16_t while they are int16_t and
>> uint8_t).
>>
>
> This patch adds UB: src was int16_t before, so that the accesses in the
> eight bit function were fine, but are not with this patch. Anyway, it is
> irrelevant now.
Why it suddenly becomes a big problem access to properly aligned uint8_t *?
I don’t mind to discuss the rules regarding to these violating of strict
aliasing,
especially in checkasm. But why it suddenly becomes a rule blocking a patch
trying to fix a fate failure.
I don’t buy the reason "the accesses in the eight bit function were fine”.
>
> - Andreas
>
> _______________________________________________
> 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 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".
