WIP: Readimage png + jpeg

[r-zenine]

Hi everyone,

This PR adds reading functions for PNG using libpng . It follows a discussion held in #1179.

My C++ being a bit rusty (it's been 8 years haha). Feel free to make feedback. It will be gratefully welcomed.
@fmassa I did my best to fix the formatting issue. Sorry about that I did not notice at the time.

Thanks

[r-zenine]

Hi,

I just noticed that CI is broken with the PR. I've looked at some of the logs. It seems to boil down to libpng not being found.

Do you have any resources on how to update circleci so that i can tell it to install libpng ?

Thanks

[fmassa]

Hi @r-zenine ,

We would need to install libpng in a few places.
We currently use both CircleCI and TravisCI (for legacy reasons).
For TravisCI, you'd need to install it in

vision/.travis.yml

Line 44 in cec7ea7

For CircleCI, it's a bit more involved because we have builds for Linux, OSX and Windows (which are all slightly different)

For CUDA CI:

vision/.circleci/config.yml.in

Line 120 in cec7ea7

For conda:

vision/packaging/torchvision/meta.yaml

Line 14 in cec7ea7

- setuptools

(maybe using libpng from anaconda?

vision/packaging/torchvision/meta.yaml

Line 14 in cec7ea7

- setuptools

)

For wheels:
Somewhere like

vision/packaging/build_wheel.sh

Line 14 in cec7ea7

python setup.py clean

maybe?

And for windows I don't actually know much... but probably somewhere in the vs folders in https://github.com/pytorch/vision/tree/master/packaging

And once we make CI pass, we still need to see how we will be seeing the story around packaging for the binaries. But let's start with small steps.

Thanks a lot for the PR!

[fmassa]

One more thing is that the setup.py script needs to be aware of the location of libpng so that it can compile everything properly, including the headers. This is something that should probably be handled in setup.py as well

Also, one thing to check is how PIL package its libraries internally. Maybe just installing PIL is enough to ensure we have the libraries that we need, and we just need to point to those libraries installed by PIL.

[r-zenine]

Hi @fmassa,

I was planning to dig deep into PIL build system this week-end. I took a first quick look at it and it seemed quite complex. Therefore, I planned to do it this week-end when I have more time.

I will take care of the CI in the meantime.

Could you please take a look at the code? Or do you prefer to wait until everything is done ? your call !

Have a good day.

[fmassa]

I think this looks pretty good, thanks a lot!

I did a quick pass and made some comments, let me know what you think.

[r-zenine]

@fmassa Thanks for the review.
I'll try to fix everything in the coming days. I'll message you when everything is updated.
Have a good evening !

[fmassa]

Hi @r-zenine

Sounds good, looking forward to the updated version!

[codecov-io]

Codecov Report

Merging #1632 into master will increase coverage by 65.53%.
The diff coverage is 55.55%.

@@             Coverage Diff             @@
##           master    #1632       +/-   ##
===========================================
+ Coverage    0.43%   65.96%   +65.53%     
===========================================
  Files          92       93        +1     
  Lines        7396     7346       -50     
  Branches     1113     1111        -2     
===========================================
+ Hits           32     4846     +4814     
+ Misses       7356     2180     -5176     
- Partials        8      320      +312

Impacted Files	Coverage Δ
torchvision/io/image.py	55.55% <55.55%> (ø)
torchvision/datasets/celeba.py	18.75% <0%> (+18.75%)	⬆️
torchvision/transforms/_functional_video.py	21.42% <0%> (+21.42%)	⬆️
torchvision/datasets/voc.py	21.64% <0%> (+21.64%)	⬆️
torchvision/datasets/caltech.py	21.83% <0%> (+21.83%)	⬆️
torchvision/datasets/sbu.py	23.72% <0%> (+23.72%)	⬆️
torchvision/datasets/phototour.py	24.24% <0%> (+24.24%)	⬆️
torchvision/datasets/lsun.py	24.48% <0%> (+24.48%)	⬆️
torchvision/datasets/flickr.py	24.67% <0%> (+24.67%)	⬆️
... and 84 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2573a7...1ed3ed3. Read the comment docs.

[r-zenine]

Hi @fmassa,

I managed to fix one part of the CI ( Travis, linux_wheel, macos_conda ). I spent a day trying various tricks without success for the remaining pipelines. I have no experience with windows nor with anaconda packaging. I might not be able to do more. Can anyone of the team help?

Also, I looked into how we could ship the libpng with the package. My understanding is that adding this line below will add a -lpng flag when linking vision C++ code. I should be enough to build vision from source if libpng is installed in a standard location ( At least for Linux).

https://github.com/r-zenine/vision/blob/fd980232b76216364a5fc5d846be073dee907b59/setup.py#L146

The questions that remain are :

• Do we support non-standard installs of libpng via build options on Linux?
• How does it work for macOS and Windows?

[vadimkantorov]

@fmassa If extra care isn't taken to disable GCC mulitlib / multiarch and standard location OS headers, we can again run into the old linking problem when building from source: (my old investigation here: torch/image#171 (comment) )

[r-zenine]

Hi @vadimkantorov,

My understanding is that cmake allows configuring a custom location for libraries.
It seems to me that it is the same as when we want to build torchvision from source against a specific build of libtorch. I the later situation, we can configure a custom libtorch location via an environment variable Torch_DIR. I think it's possible to do the same with libpng. If I am not mistaken the variable should be PNG_DIR.

Would this strategy address the issue you raise? Am I missing something?

Thanks

[vadimkantorov]

@r-zenine In that old investigation there were two problems discovered: 1) CMake passing library filenames instead of full paths even if a custom library location was specified (maybe this is no longer relevant), 2) at linking time GCC preferring system libraries over custom-built ones (including conda ones) because of multilib / multiarch. Don't remember how binary loading was mixing with this.

Differently from many PyTorch dependencies, libjpg/libpng are often present in system OS locations.

Concretely, I'm proposing having some test to confirm that build and OS binary loader picks the right libraries in presence of multilib/multiarch system library versions.

I also brought this up in pytorch/pytorch#13188

[r-zenine]

HI,

Another update,

Still a work in progress. However, we got macos_wheel working.

conda based setups are not working, tests are failing because of a failure to load the C extension. It might be happening because we are not using the same compiler for everything. Not sure how to fix it.
I'll investigate this at the end.

Windows fail because of a linking fail preventing us from building libpng. I will investigate tomorrow.
Going to work.

[r-zenine]

Hi There,

Making progress here! Even though I have to admit, I am way out of my comfort zone.

The linux_conda pipeline are timing out. Any idea how to fix it?

I have tried several times to fix the windows pipeline without success. The feedback loop is too slow ( having to push the code and wait for the CI to run). Do you have any tips to make the feedback loop go faster? I guess not having a windows machine ready and configured is not helping either.

The code is not clean by any means. There are tons of commits, I'll clean things up when we get everything up and running and rebase and clean the commits.

I would appreciate some help with the windows pipeline if anyone can take a look into it.

Big thanks guys,
Going to work

[cpuhrsch]

Thanks for working on this @r-zenine! Believe me, I understand how frustrating it is to add a dependency. Here are some suggestions on how to break this problem into smaller pieces given that this is becoming too complex.

You could split this into two PRs: png and jpeg separately. That way we can reduce the error surface. In particular if you can split it such that only one dependency needs to be introduced.

On Windows: I'd focus on fixing all the other cases first. In fact, there is nothing wrong with selectively disabling the support for these ops for Windows and Mac first. You can use @unittest.skipIf('win' in sys.platform) to disable the tests on Windows. We can then later on add support for this.

On the topic of speeding up the feedback loop: Ninja was supposed to help with build times but until we have support for a flag that limits the threads based on the environment we can't enable it just yet. Windows is always its own story and in my experience has terrible feedback loop times. We're working on fixing this, but a good way to avoid it for now is to add support for it in a separate PR.

So, in summary, I'd create a PR that adds jpeg reader support for linux only for now. Then a PR that does the same for png. Then a PR for Mac support for jpeg and png each. Then a PR for Windows support for jpeg and png each.

I hope this helps.

[r-zenine]

Hi @cpuhrsch ,

Let's proceed that way. I'll do PR's for:

1. png for linux
2. jpeg for linux
3. png for mac
4. jpeg for mac
5. png for windows
6. jpeg for windows

In the meantime, if we could find help for the windows pipeline, I would greatly appreciate it.
Okay for you ?
Thanks

[cpuhrsch]

@r-zenine - sounds great! I'll drum up some help once we get to Windows.

[r-zenine]

@cpuhrsch,

Awesome, Thanks !

[fmassa]

Subsumed by #2388 and #2382