ffvvc / FFmpeg

VVC Decoder for ffmpeg
Other
50 stars 12 forks source link

Remove unused SAD size #228

Closed nuomi2021 closed 4 months ago

nuomi2021 commented 4 months ago

From Benjamin Bross: DMVR, I recently saw that also 32x32 to 128x128 AVX2 DMVR SAD functions have been added. Not digged deep into that but I was wondering whether these are even used? In VVC DMVR is only allowed for 8x16, 16x8 and 16x16 blocks

@stone-d-chen, could you double confirm it? you can assert the size first. Then search the spec to find the correct size.

stone-d-chen commented 4 months ago

Hmm I was just basing it off of this paper, and assumed any CU size is valid but I will search the spec... it could be the block type used is by definition already a restricted size? e.g. PUs can only be certain sizes already? https://ieeexplore.ieee.org/document/9252910

image

stone-d-chen commented 4 months ago

But I will double check this

nuomi2021 commented 4 months ago

dmvr_mv_refine called by derive_sb_mv. I guess subblocks are constrained. you can check the mi->num_sb_x assignment

stone-d-chen commented 4 months ago

Hi yeah, seems like I was mistaken, the sub-blocks are constrained pg 235 of the spec image

I will submit a patch though it will be mostly cosmetic since the .asm uses the same codepaths for 16-128; I should also change the checkasm as well to not test the other block widths.

nuomi2021 commented 4 months ago

@stone-d-chen, thank you,

Before you send the patch, could you add: av_assert0(w == 16 && h == 16 || w == 8 && h == 16 || w == 16 && h == 8); and run a full conformance test to make sure our assumption is correct? Do not include the assert in your patch, just test it

also, will the height impact your code? it's just two height types 8 and 16

stone-d-chen commented 4 months ago

@stone-d-chen, thank you,

Before you send the patch, could you add: av_assert0(w == 16 && h == 16 || w == 8 && h == 16 || w == 16 && h == 8); and run a full conformance test to make sure our assumption is correct? Do not include the assert in your patch, just test it

Sounds good -- can I assume if it doesn't appear in the conformance clips than it will never appear? Or another way - does the conformance clips test all features / constraints of the spec?

also, will the height impact your code? it's just two height types 8 and 16

The actual asm it doesn't - the height was looped over 1 row at a time already. I think at most there could be additional optimizations given that there's new constraints.

nuomi2021 commented 4 months ago

@stone-d-chen, thank you, Before you send the patch, could you add: av_assert0(w == 16 && h == 16 || w == 8 && h == 16 || w == 16 && h == 8); and run a full conformance test to make sure our assumption is correct? Do not include the assert in your patch, just test it

Sounds good -- can I assume if it doesn't appear in the conformance clips than it will never appear? Or another way - does the conformance clips test all features / constraints of the spec? No for both. But, Benjamin is an expert on the VVC spec. If he says so and the conformance clip proves it, I guess it's low risk.

also, will the height impact your code? it's just two height types 8 and 16

The actual asm it doesn't - the height was looped over 1 row at a time already. I think at most there could be additional Given the new constraints, you can optimize by using as many registers as possible, processing 4 or 8 rows at a time.

Thank you

nuomi2021 commented 4 months ago

merged as https://github.com/FFmpeg/FFmpeg/commit/d82c5035558730f0d1f2e5aec66df6d0db7f4e6f thank you stone

stone-d-chen commented 4 months ago

Great! Switching focus to deblocking now