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 jpeg encoding tests on windows #3913

Closed
NicolasHug opened this issue May 25, 2021 · 4 comments · Fixed by #7820
Closed

Fix jpeg encoding tests on windows #3913

NicolasHug opened this issue May 25, 2021 · 4 comments · Fixed by #7820

Comments

@NicolasHug
Copy link
Member

In #3908 we fixed the test logic of the tests for encode_jpeg. Unfortunately, the new "correct" tests don't work on windows.

This issue is about a) making those new correct test work on windows and b) remove the old incorrect tests

For b), having a roundtrip test as proposed in #3912 might be enough to remove the old tests.

CC @fmassa @datumbox

@NicolasHug
Copy link
Member Author

Update on this: From #3968 it is clear that:

  • On the linux CI (and very likely on macOS), both torchvision and PIL rely on libjpeg 9.0
  • On the Windows CI, torchvision relies on libjpeg 9.0 but PIL relies on libjpegturbo 8.

This is consistent with python-pillow/Pillow#3833 (comment)

As mentioned before, the Windows wheels are built with libjpeg-turbo, the Linux wheels are not

This can explain why the new correct tests fail on windows: the encoding strategy is likely different between libjpeg9 and libjpegturbo 8.

Also from python-pillow/Pillow#3833 (comment)

If the Pillow wheel does not have the version of libjpeg that you require, then install it and try compiling Pillow from source. Compiling on Windows became easier after the opening of this issue, thanks to #4495.

There are no Windows wheels that don't come with libjpeg-turbo as far as I can tell. Sooooo... for a) I only see 2 options:

  • Start supporting libjpeg turbo so that we can properly compare our implementation on windows
  • build PIL from source in the Windows CI runs

@datumbox
Copy link
Contributor

datumbox commented Jun 4, 2021

@NicolasHug awesome investigation skills. I think we might be in a situation where every solution stinks...

If we do option 1, aka use libjpeg-turbo for our windows build and libjpeg 9.0 for our linux one, don't we run the risk of having different results between platforms on our TorchVision methods? Also if we do change our dependency, should we consider it a breaking change? Another thing to consider is whether the to implementation have differences that can cause more headaches on the C++ code. We already know that libjpeg has differences across versions/platforms and that forced us to pin the version to 9b (see #3787).

Building from source sounds tedious... I wonder if it makes sense to raise an issue to PIL and discuss the possibility of offering an official version on their end.

@NicolasHug
Copy link
Member Author

I think that if we want to support libjpeg-turbo, we would support it across all OSs, not just windows. There is an incentive to support libjpeg-turbo because it's really fast (even faster than our nvjpeg decoding at the moment). And it could probably be an opt-in not necessarily a hard dependency change.

In theory it should just be a matter of linking against libjpeg-turbo instead of libjpeg as they should have the same API. I briefly tried that last time and it didn't compile properly (I didn't insist for long though). But there is something to explore I think.

Regarding the tests, maybe there's another way to "fix" them: the good news is that torchvision uses libjpeg 9 across all OSs. So now we just need is a test that the jpeg encoding is consistent across OSs: we already check that linux == pil and macos == pil in the new correct tests, and if we have linux == macos == windows this will indirectly ensure that windows == pil as well. The only way to ensure that linux == macos == windows though is to dump another reference image (with torchvision this time)... which isn't great, but hopefully it will work and we'll be able to run this test on all OSs, not just on Windows (Also, I'll make sure to properly document this to avoid future headaches). I'll try something next week.

@NicolasHug
Copy link
Member Author

I'll try something next week

I forgot that we still need to account for fbcode here... which might be using yet another different libjpeg version. I changed my mind 😅, it's not even worth trying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants