Skip to content

read jpeg linux #1909

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

Closed
wants to merge 28 commits into from
Closed

read jpeg linux #1909

wants to merge 28 commits into from

Conversation

r-zenine
Copy link
Contributor

@r-zenine r-zenine commented Feb 23, 2020

Hi,

As discussed earlier. This PR brings support for jpeg decoding on linux.
The link is done statically.

On open question remains though: How do we test this ?

Jpeg decoders have few differences and don't always output the same image depending on the implementation. For instance, pillow and pillow-simd will not output the same Tensors. Because libjpeg and libjpeg-turbo don't.

The issue is the following: In order to build pillow-simd we need to install zlib libpng and libjpeg-turbo on the machine. We then take the risk of linking the system libs at build time instead of our own vendored versions.

There is small package called PyTurboJPEG that is available on pip that vendors it's dependencies as we do. We could use it for our unit test ( see this commit 3599c64f64db131eccd3dffec6663eaa20e98113). However, it's not available on conda.

Thoughts ?

Have a good day.
Thanks

@r-zenine r-zenine requested a review from cpuhrsch February 23, 2020 12:48
@@ -7,7 +7,7 @@ script_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
export BUILD_TYPE=wheel
setup_env 0.6.0
setup_wheel_python
pip_install numpy pyyaml future ninja
pip_install numpy pyyaml future ninja pillow-simd
Copy link
Contributor

@cpuhrsch cpuhrsch Feb 23, 2020

Choose a reason for hiding this comment

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

I don't think it's worth adding this only for unit testing. We're adding an libjpeg-turbo as an optimization for an existing feature and, I think, keep the PR centered around that. If we also want to evaluate and test pillow-simd we should do that separately and evaluate it as another potential optimization.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right.

@cpuhrsch
Copy link
Contributor

Small differences in output are expected when using different JPEG libraries. In particular, when you use vectorized libraries the order in which e.g. numbers are added has influence on the result. However, those differences are small and should be within a few machine epsilons of the other libraries. We could also look at other libraries and how they test their JPEG decoding testing.

@r-zenine
Copy link
Contributor Author

r-zenine commented Feb 24, 2020

Hi @cpuhrsch,

The jpeg ISO spec seems to be behind a paywall so there is no way to get the exact precision requirements. . However, I took a look at how pillow is doing this and implemented a similar strategy.

https://github.com/python-pillow/Pillow/blob/de179eb5c6877d7da9a7759512bb5acbfbc0b76a/Tests/helper.py#L115

@r-zenine r-zenine force-pushed the own/read-jpeg-linux branch from 8ef7bd8 to db586c1 Compare March 8, 2020 14:40
@r-zenine
Copy link
Contributor Author

r-zenine commented Mar 9, 2020

Hi @fmassa,

I am just leaving a small message to update you on the on going work.

The linux_wheel pipelines are working.
The conda pipelines are not. conda is doing weird things with binary relocation. I tried many different setups without success. I am out of ideas on how to make it work. Sadly, there is little to no documentation on how to do this.

Thanks

@andfoy
Copy link
Contributor

andfoy commented Jun 16, 2020

Hi @r-zenine, thanks for your contribution! We have migrated the most relevant commits into #2301, where we also added your work for loading png files. Thus, we're closing this one

@andfoy andfoy closed this Jun 16, 2020
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