[FFmpeg-devel] [FFmpeg-cvslog] swscale/aarch64: Fix rgb24toyv12 only works with aligned width

[Martin Storsjö]

On Wed, 25 Sep 2024, Martin Storsjö wrote:

> On Wed, 25 Sep 2024, Zhao Zhili wrote:
>
>>> On Sep 25, 2024, at 16:01, Martin Storsjö <martin at martin.st> wrote:
>>> 
>>> On Tue, 24 Sep 2024, Zhao Zhili wrote:
>>> 
>>>> ffmpeg | branch: master | Zhao Zhili <zhilizhao at tencent.com> | Wed Sep 18 
>>>> 21:11:44 2024 +0800| [e18b46d95fadcbaaf450bda9f1871849f2b0c586] | 
>>>> committer: Zhao Zhili
>>>> 
>>>> swscale/aarch64: Fix rgb24toyv12 only works with aligned width
>>>> 
>>>> Since c0666d8b, rgb24toyv12 is broken for width non-aligned to 16.
>>>> Add a simple wrapper to handle the non-aligned part.
>>>> 
>>>> Co-authored-by: johzzy <hellojinqiang at gmail.com>
>>>> Signed-off-by: Zhao Zhili <zhilizhao at tencent.com>
>>>> 
>>>>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=e18b46d95fadcbaaf450bda9f1871849f2b0c586
>>>> ---
>>>> 
>>>> libswscale/aarch64/rgb2rgb.c | 23 ++++++++++++++++++++++-
>>>> tests/checkasm/sw_rgb.c      |  2 +-
>>>> 2 files changed, 23 insertions(+), 2 deletions(-)
>>>> --- a/tests/checkasm/sw_rgb.c
>>>> +++ b/tests/checkasm/sw_rgb.c
>>>> @@ -129,7 +129,7 @@ static int cmp_off_by_n(const uint8_t *ref, const 
>>>> uint8_t *test, size_t n, int a
>>>> 
>>>> static void check_rgb24toyv12(struct SwsContext *ctx)
>>>> {
>>>> -    static const int input_sizes[] = {16, 128, 512, MAX_LINE_SIZE, 
>>>> -MAX_LINE_SIZE};
>>>> +    static const int input_sizes[] = {2, 16, 128, 540, MAX_LINE_SIZE, 
>>>> -MAX_LINE_SIZE};
>>>>
>>>>    LOCAL_ALIGNED_32(uint8_t, src, [BUFSIZE * 3]);
>>>>    LOCAL_ALIGNED_32(uint8_t, buf_y_0, [BUFSIZE]);
>>> 
>>> These new test cases fail on x86_32; we have got a version of rgb24toyv12 
>>> which is specific to "#if ARCH_X86_32 && HAVE_7REGS".
>>> 
>>> Can you have a look?
>> 
>> Sorry for the break. I’m on a short vacation without access to x86_32 test 
>> environment. And I’m not familiar with x86 asm. I’m afraid removing the new 
>> test is what I can do for now, if that’s an option.
>
> Thanks - yeah I think that's the practically best thing to do at the moment. 
> I guess this assembly has existed in this form for a very long time already, 
> so while it probably is incorrect for these cases, it doesn't seem to be an 
> urgent thing. (But I guess whatever case that was noted on aarch64 also would 
> be noted on x86_32?) So silencing the test for now probably is simplest, 
> until the assembly can be fixed.

I pushed a commit to remove these testcases from checkasm, for now. 
Thanks!

// Martin