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

Rewrite of Jpeg2k data handling #1934

Merged
merged 9 commits into from
Jun 30, 2016

Conversation

wiredfool
Copy link
Member

@wiredfool wiredfool commented May 29, 2016

Fixes the intermittent test failures and occasional dos/thread deadlock in the J2k stack by rewriting the data handling to not use threading.

Changes proposed in this pull request:

  • Adds a new interface/mode to the ImageFile where the C layer can write to the python file like object.
  • Rips out the threading in Incremental.c and converts J2k to use the new layer.

Rationale:

It is a relatively common pattern in image libraries to provide a set of read/seek/write functions to the underlying library so that the library can handle pulling/pushing data to a file like object. OpenJpeg does this for the J2k file format, and I worked up a way to do this in the tiff support as well. This additional interface in the C layer allows for a consistent implementation of this core bit of code.

The de/encoder needs to set pushes/pulls_fd =1 from within its initialization routine. If that flag is set, ImageFile.py will call setfd(self.fd) and then call the de/encoder routine. At that point, the de/encoder can point (or wrap) any read/seek/write functions at the _imaging_*_pyfd family of functions which call out to the python file like object. There will be one call to the de/encoder per tile, and the coder will handle any buffer sizes or shuffling within that call. This reduces the complexity of the decoder function, as there is no longer a requirement for multiple calls with additional data to be handled sequentially.

ToDo:

  • There's still cleanup of warnings and such.
  • Make sure we like the interface and that we're returning the right results from the write/seek/read functions.

@homm
Copy link
Member

homm commented Jun 21, 2016

I am in favor of this approach. I would like to see some level of abstraction instead of set of if decoder.pulls_fd in ImageFile.

@wiredfool
Copy link
Member Author

The api is tricky. I'm basing this on:

  • The proper place to define and set this up is in the PyImaging_*De/EncoderNew setup functions.
  • There's a different set of parameters that are required for the pyfd approach than the existing feeder (and the encode_to_file call)
  • The different parameters span both the ImageFile python layer and the lower decoder/encoder layer.
  • I don't really want to change the method signature of all of the PyImaging_*De/EncoderNew functions, as that would break anyone who has a non-contributed image de/encoder.

The encoder is simpler, as it's already got the encode_to_file method. Here I've added encode_to_pyfd. To be cleaner, we could include the pyfd in as a parameter there rather than the flag + the setfd call. But we'd still effectively need a flag somewhere, either try the method and throw an exception if it's not appropriate or check the flag and use the method.

I suppose we could split the decode(bytes) into decode_from_pyfd(self.fp), and eliminate the decoder.setfd call and eliminate the empty bytestring that we're passing in. But again, that doesn't eliminate the flag, it just streamlines the implementation.

@wiredfool
Copy link
Member Author

Ok, I've added some docs for it, with big warnings on the experimental. I'm going to refactor the LibTiff support to use this when I have time, and when I'm doing that, I'll look more closely at the interface. If someone else uses this in the mean time, we'll move them along as the interface moves.

@wiredfool wiredfool merged commit a5dde79 into python-pillow:master Jun 30, 2016
@wiredfool wiredfool deleted the incremental_removal branch October 2, 2017 13:25
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.

None yet

2 participants