Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix negative MV diff rate calculation when allow_high_precision_mv is false #3003

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

redzic
Copy link
Collaborator

@redzic redzic commented Aug 27, 2022

Bit shifting for division by a power of 2 causes incorrect rounding for negative numbers. This should very slightly increase quality as a result of more accurate rate distortion calculations.

@codecov-commenter
Copy link

Codecov Report

Base: 86.52% // Head: 86.48% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (9d50709) compared to base (c113c00).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3003      +/-   ##
==========================================
- Coverage   86.52%   86.48%   -0.04%     
==========================================
  Files          89       89              
  Lines       34046    34046              
==========================================
- Hits        29457    29445      -12     
- Misses       4589     4601      +12     
Impacted Files Coverage Δ
src/me.rs 95.10% <100.00%> (-0.49%) ⬇️
src/tiling/tile_state.rs 89.13% <0.00%> (-0.73%) ⬇️
src/api/internal.rs 97.47% <0.00%> (-0.23%) ⬇️
v_frame/src/plane.rs 88.78% <0.00%> (-0.23%) ⬇️
src/header.rs 68.93% <0.00%> (-0.13%) ⬇️
src/deblock.rs 95.43% <0.00%> (ø)
src/encoder.rs 87.09% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@barrbrain
Copy link
Collaborator

barrbrain commented Aug 27, 2022

The logic is sound. I started an AWCY run on objective-1-fast at default speed:

PSNR Y PSNR Cb PSNR Cr CIEDE2000 SSIM MS-SSIM PSNR-HVS Y PSNR-HVS Cb PSNR-HVS Cr PSNR-HVS VMAF VMAF-NEG
0.0260 0.0176 -0.0239 0.0250 0.0430 0.0421 0.0239 -0.0521 -0.0485 0.0202 0.1038 0.1044

@barrbrain
Copy link
Collaborator

I think the confounding factor is that this function is called for motion estimation on subsampled frames.

@redzic
Copy link
Collaborator Author

redzic commented Aug 27, 2022

@barrbrain I also saw your other commit (barrbrain@5b45ccf)

At least for me, it wasn't obvious at first that it changes the rate curve itself rather than just the rounding (see below)

https://www.desmos.com/calculator/ekdizwd572

perhaps the rate curve could somehow be adjusted for better gains? I'll look into it further

@barrbrain
Copy link
Collaborator

@barrbrain I also saw your other commit (barrbrain@5b45ccf)

This was a mistake, I shouldn't have changed the scale.

I tried another experiment which gave modest gains on top of the rounding change, barrbrain@7baa93f :
"Do not round in get_mv_rate except for original resolution"

PSNR Y PSNR Cb PSNR Cr CIEDE2000 SSIM MS-SSIM PSNR-HVS Y PSNR-HVS Cb PSNR-HVS Cr PSNR-HVS VMAF VMAF-NEG
-0.0248 0.1399 0.0763 -0.0095 -0.0751 -0.0429 -0.0080 0.1179 0.1972 -0.0085 -0.1348 -0.1132

However, the more surprising result was that if that change were applied first, the rounding change had no impact on decisions.

Compared to the baseline, results for this experiment were a wash:

PSNR Y PSNR Cb PSNR Cr CIEDE2000 SSIM MS-SSIM PSNR-HVS Y PSNR-HVS Cb PSNR-HVS Cr PSNR-HVS VMAF VMAF-NEG
0.0010 0.1568 0.0512 0.0156 -0.0321 -0.0008 0.0159 0.0657 0.1465 0.0116 -0.0314 -0.0091

@redzic
Copy link
Collaborator Author

redzic commented Aug 30, 2022

Hmm, I'll mark this as draft for now then unless I can find an improvement later on

@redzic redzic marked this pull request as draft August 30, 2022 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants