Re: [FFmpeg-devel] [PATCH v3] lavc/aarch64: add hevc horizontal qpel/uni/bi

["J. Dekker" <jdek@itanimul.li>]

On 24 Oct 2022, at 14:01, Martin Storsjö wrote:

> On Tue, 11 Oct 2022, J. Dekker wrote:
>
>> [...]
>> diff --git a/libavcodec/aarch64/hevcdsp_init_aarch64.c b/libavcodec/aarch64/hevcdsp_init_aarch64.c
>> index 644cc17715..44399b05d8 100644
>> --- a/libavcodec/aarch64/hevcdsp_init_aarch64.c
>> +++ b/libavcodec/aarch64/hevcdsp_init_aarch64.c
>> @@ -69,6 +69,46 @@ void ff_hevc_sao_edge_filter_16x16_8_neon(uint8_t *dst, const uint8_t *src, ptrd
>>                                           const int16_t *sao_offset_val, int eo, int width, int height);
>> void ff_hevc_sao_edge_filter_8x8_8_neon(uint8_t *dst, const uint8_t *src, ptrdiff_t stride_dst,
>>                                         const int16_t *sao_offset_val, int eo, int width, int height);
>> +void ff_hevc_put_hevc_qpel_h4_8_neon(int16_t *dst, uint8_t *_src, ptrdiff_t _srcstride, int height,
>> +                                     intptr_t mx, intptr_t my, int width);
>
> The function pointers in the dsp context has gotten 'const' on the source pointers now, which makes it emit a lot of warnings with GCC, and fail with latest Clang. Please rebase and check that it builds without warnings.
>

Fixed.

>> [...]
>> +.ifc \type, qpel
>> +function ff_hevc_put_hevc_h4_8_neon, export=0
>> +        uxtl            v16.8h,  v16.8b
>> +        uxtl            v17.8h,  v17.8b
>> +        uxtl            v18.8h,  v18.8b
>> +        uxtl            v19.8h,  v19.8b
>> +
>> +        mul             v23.4h,  v16.4h, v0.h[0]
>> +        mul             v24.4h,  v18.4h, v0.h[0]
>> +
>> +.irpc i, 1234567
>> +        ext             v20.16b, v16.16b, v17.16b, #(2*\i)
>> +        ext             v21.16b, v18.16b, v19.16b, #(2*\i)
>> +        mla             v23.4h,  v20.4h, v0.h[\i]
>> +        mla             v24.4h,  v21.4h, v0.h[\i]
>> +.endr
>> +        ret
>> +endfunc
>> +.endif
>> +
>> +function ff_hevc_put_hevc_\type\()_h4_8_neon, export=1
>> +        load_filter     mx
>> +.ifc \type, qpel_bi
>> +        mov             x16, #(MAX_PB_SIZE << 2) // src2bstridel
>> +        add             x15, x4, #(MAX_PB_SIZE << 1) // src2b
>
> Beware that you can't in general rely on x16/x17 keeping their values for long. If you branch to a function which is implemented in a different object file, it may end up linked at a place in the address space which is too far away for a regular 'bl' branch, so the linker has to insert a range extension thunk, which clobbers x16/x17. But as long as everything here is branched within the same object file, it should be ok.
>
> In general, if you need to use x16/x17, use it only for very short-lived temporaries.

Alright, thanks for the consideration. Left as is since as you said we're not branching anywhere outside this file.

>> +.endif
>> +        sub             src, src, #3
>> +        mov             mx, lr
>
> Please use literal 'x30' instead of 'lr' - older binutils don't support the 'lr' register name alias.

Fixed.

> Other than that, the code seems to run correctly, and the code looks mostly reasonable now. (I didn't do a very deep read-through this time, but it looks like you've addressed my earlier concerns.)

Thanks for the review, pushed with above fixes.

-- 
jd
_______________________________________________
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".