Skip to content

Fix a few loop filtering issues to match libwebp output #148

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

Merged
merged 5 commits into from
Jun 28, 2025

Conversation

tristanphease
Copy link
Contributor

Fixes these three issues with the loop filtering so that #146 shouldn't have discrepancies:

  1. The filtering threshold level is set incorrectly in some cases; doesn't match the spec
  2. We don't check correctly for if we should do subblock filtering on a macroblock by checking if there exists a non-zero dct
  3. Results don't match for some of the final macroblocks since the yuv buffers don't contain all the data on the end of the macroblocks so would cause issues. Therefore we have to expand the yuv buffers so they always cover macroblocks and do loop filtering for all the macroblocks evenly. The rgb buffers should still be filled correctly because of the way we zip the iterators.

Tested with the image in the gallery mentioned in the issue: reftest_gallery2_1_webp_a by decoding with libwebp -nofancy to a png, loading both the png and webp and comparing all the pixels which matched exactly. Will need to check some more images to see if there's any remaining discrepancies though.

Fixing the two following issues:
1. filtering threshold level is set incorrectly in one case
2. we don't check correctly for if we should do subblock filtering on a macroblock by checking if there exists a non-zero dct
@Shnatsel
Copy link
Member

I see tests fail because of an animated image. This is may be expected because libwebp uses an approximate alpha blending method, and we use a more precise version of it than libwebp: #123

Another possibility is that since all animation frames have a fully opaque alpha channel, libwebp skips blending even though the blend flag is set for the 2nd frame of the animation, improving both performance and accuracy.

@tristanphease
Copy link
Contributor Author

tristanphease commented Jun 28, 2025

Sorry was just because I messed up changing the buffers, have fixed that up now. Having the buffers as a different size from the image is a bit confusing now though I don't really see an alternative since we need to have them large enough to contain the macroblocks

@tristanphease
Copy link
Contributor Author

Were those images all generated with cwebp -nofancy ?
Tried with assert_eq!(num_bytes_different, 0); and seems to work with the tests in the repo.

@Shnatsel
Copy link
Member

The creation process for the images is described at https://github.com/image-rs/image-webp/blob/main/tests/CREDITS.md

Although I fudged the animated lossless one in #123

@Shnatsel
Copy link
Member

Percentage difference was used because we couldn't achieve bit-exact output. Now that we can, using assert_eq!(num_bytes_different, 0); sounds like a great idea.

@Shnatsel
Copy link
Member

Will need to check some more images to see if there's any remaining discrepancies though.

I have scraped 7500 images from the web that can be used for tests, the archive can be found here.

I'll run a comparison on this corpus and report back with the results.

@Shnatsel
Copy link
Member

With this PR we go from slightly diverging from dwebp -nofancy on 6000 images from this corpus to only slightly diverging on 31 images:

webp-loop-pr-divergences.zip

dwebp doesn't support animated images so those weren't tested.

@awxkee
Copy link

awxkee commented Jun 28, 2025

With this PR we go from slightly diverging from dwebp -nofancy on 6000 images from this corpus to only slightly diverging on 31 images:

webp-loop-pr-divergences.zip

dwebp doesn't support animated images so those weren't tested.

I'd suggest not worrying, not only because of #123, but also because if you were testing on x86 without explicitly disabling SSE/AVX in libwebp, the YUV decoding math in libwebp doesn't match the math used here.

@197g 197g merged commit 1c42ddd into image-rs:main Jun 28, 2025
7 checks passed
@Shnatsel
Copy link
Member

We've actually tuned the YUV->RGB conversion to exactly match the SSE codepath in libwebp. And there is no difference between dwebp -nofancy and dwebp -nofancy -noasm.

So it is likely that there is a lingering bug somewhere that only manifests in some rare edge cases. But I agree slight visual differences on 31 images out of 6000+ are not a big deal in the grand scheme of things.

@tristanphease tristanphease deleted the loop-filtering-issues branch June 29, 2025 01:10
@tristanphease
Copy link
Contributor Author

Might have a look at some of those remaining discrepancies anyway

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.

5 participants