Skip to content

Warn when webp is asked to decode into grayscale #9101

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.

Sign up for GitHub

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 2 commits into from
Jun 10, 2025
Merged

Warn when webp is asked to decode into grayscale #9101

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e51366f
Warn when webp is asked to decode into grayscale
scotts Jun 9, 2025
bcf6488
Put warning behind conditional; test for it at Python level
scotts Jun 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions test/test_image.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -934,6 +934,20 @@ def test_decode_webp(decode_fun, scripted):
img += 123 # make sure image buffer wasn't freed by underlying decoding lib


@pytest.mark.parametrize("decode_fun", (decode_webp, decode_image))
def test_decode_webp_grayscale(decode_fun):
encoded_bytes = read_file(next(get_images(FAKEDATA_DIR, ".webp")))

# Note that we warn at the C++ layer because dispatching for decode_image
# doesn't happen until we hit C++. The C++ layer does not propagate
# warnings up to Python, so we can't test for them.
img = decode_fun(encoded_bytes, mode=ImageReadMode.GRAY)
Copy link
Member

@NicolasHug NicolasHug Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a shame we don't get a nice python warning, but I guess that'll do for now. We can still catch it by adding the capfdfixture , and then something like:

assert "Webp does not support grayscale conversions." in capfd.readouterr().err

Now the first test parametrization with decode_webp will pass, but the one with decode_image will fail as expected, because we only warn once. Maybe we should only test decode_image then, since it's the highest-level function.

Copy link
Contributor Author

@scotts scotts Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, if we're capturing the output, then both functions should pass the test, as decode_webp() calls the C++ function of the same name - and that's where the check is. I'll see if I can improve it.

Copy link
Contributor Author

@scotts scotts Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: I misread your comment, as you were talking about the warn-once aspect. I addressed that with the API that lets us temporarily turn on warnings all the time. Without it, both tests failed, which means that some test somewhere was already triggering the warning.


# Note that because we do not support grayscale conversions, we expect
# that the number of color channels is still 3.
assert img.shape == (3, 100, 100)


# This test is skipped by default because it requires webp images that we're not
# including within the repo. The test images were downloaded manually from the
# different pages of https://developers.google.com/speed/webp/gallery
Expand Down
5 changes: 5 additions & 0 deletions torchvision/csrc/io/image/cpu/decode_webp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ torch::Tensor decode_webp(
res == VP8_STATUS_OK, "WebPGetFeatures failed with error code ", res);
TORCH_CHECK(
!features.has_animation, "Animated webp files are not supported.");
TORCH_WARN_ONCE(
mode == IMAGE_READ_MODE_GRAY ||
mode == IMAGE_READ_MODE_GRAY_ALPHA,
Copy link
Member

@NicolasHug NicolasHug Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we TORCH_WARN_ONCE works differently than TORCH_CHECK, and doesn't accept a bool as the first parameter (well it does, but it'll just print it). So right now we always warn unconditionally, I think we need to add this condition check in an if block

Copy link
Contributor Author

@scotts scotts Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That explains the output I was seeing! Got it, will fix.

"Webp does not support grayscale conversions. "
"The returned tensor will be in the colorspace of the original image.");

auto return_rgb =
should_this_return_rgb_or_rgba_let_me_know_in_the_comments_down_below_guys_see_you_in_the_next_video(
Expand Down