matthewwithanm / pilkit

Utilities and processors built for, and on top of PIL
BSD 3-Clause "New" or "Revised" License
196 stars 54 forks source link

Create Convert processor #32

Closed kravemir closed 5 years ago

kravemir commented 5 years ago

The PR contains utility processor, which is to be used in conjunction of other processors. Conversion to RGBa (pre-multiplied alpha) is needed in order to perform alpha composite correctly (for more details, see discussion in https://github.com/python-pillow/Pillow/issues/3494).

For example, it is used in pipeline like:

  1. convert to RGBa,
  2. perform pre-multiplied alpha dependent operation,...
  3. perform pre-multiplied alpha dependent operation,...
  4. perform pre-multiplied alpha dependent operation,...
  5. convert to RGBA.

Final pipeline of processors contains multiple other custom processors. I'll create another PR for those, which are reusable, and not specific to custom solution.

kravemir commented 5 years ago

I'm not a python expert, but following failure doesn't seem to be related to any PR changes:

======================================================================
FAIL: tests.test_utils.test_format_to_extension_no_init
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/matthewwithanm/pilkit/.eggs/nose-1.3.7-py2.7.egg/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/matthewwithanm/pilkit/tests/test_utils.py", line 19, in test_format_to_extension_no_init
    eq_(format_to_extension('PNG'), '.png')
AssertionError: '.apng' != '.png'
kravemir commented 5 years ago

@vstoykov, what do you think about this PR? Does it look fine for you?

vstoykov commented 5 years ago

I'm not sure if pilkit.processors.image is the right module for that processor. Basically all processor process images. More appropriate name should be convert or mode. Probably convert is better choice.

kravemir commented 5 years ago

I'm not sure if pilkit.processors.image is the right module for that processor. Basically all processor process images. More appropriate name should be convert or mode. Probably convert is better choice.

I was thinking about that, also. I didn't pick convert as this would be probably only convert function (which is based only on PIL/Pillow, without any else libraries/dependency). Same for mode, there's not much to do about image's mode, and would be just single processor within package. I picked image package, as mode is a property of an image, and an image has got multiple properties, therefore higher probability of additional image (properties) related processors.

I was also thinking, that ConvertMode or ModeConvert might be a more appropriate / specific name.

@vstoykov Regardless of my opinion / reasoning, I'll refactor names to what maintainers decide, because I care more about provided functionality. So, should I move it to convert package?

vstoykov commented 5 years ago

Sorry for the delay of my answer. Yes I think for now that convert is a better name, because for example all processors related to resizing are in resize module, and they are using resize method of the Image

kravemir commented 5 years ago

Yes I think for now that convert is a better name, because for example all processors related to resizing are in resize module, and they are using resize method of the Image

Thank you for the answer. I did refactor / move Convert processor to convert module.

Does PR look fine now?

vstoykov commented 5 years ago

Yes the PR is looking good.

One more thing. Can you rebase against master to see if the problem with the tests will still be here?

kravemir commented 5 years ago

Yes the PR is looking good.

One more thing. Can you rebase against master to see if the problem with the tests will still be here?

@vstoykov I did rebase it in different branch, but fails anyway: https://travis-ci.org/kravemir/pilkit/jobs/489724487

I debugged pilkit.utils._format_to_extension:

https://github.com/matthewwithanm/pilkit/blob/f1e17e58b5a6e8b03d3097d94011f41c4f424e8e/pilkit/utils.py#L61-L75

And, after few method invocations (debug: Resume program (F9)), watch Image.EXTENSION has got value:

<class 'dict'>: {'.bmp': 'BMP', '.gif': 'GIF', '.tif': 'TIFF', '.tiff': 'TIFF', '.jfif': 'JPEG', '.jpe': 'JPEG', '.jpg': 'JPEG', '.jpeg': 'JPEG', '.pbm': 'PPM', '.pgm': 'PPM', '.ppm': 'PPM', '.png': 'PNG', '.apng': 'PNG', '.blp': 'BLP', '.bufr': 'BUFR', '.cur': 'CUR', '.pcx': 'PCX', '.dcx': 'DCX', '.dds': 'DDS', '.ps': 'EPS', '.eps': 'EPS', '.fit': 'FITS', '.fits': 'FITS', '.fli': 'FLI', '.flc': 'FLI', '.ftc': 'FTEX', '.ftu': 'FTEX', '.gbr': 'GBR', '.grib': 'GRIB', '.h5': 'HDF5', '.hdf': 'HDF5', '.jp2': 'JPEG2000', '.j2k': 'JPEG2000', '.jpc': 'JPEG2000', '.jpf': 'JPEG2000', '.jpx': 'JPEG2000', '.j2c': 'JPEG2000', '.icns': 'ICNS', '.ico': 'ICO', '.im': 'IM', '.iim': 'IPTC', '.mpg': 'MPEG', '.mpeg': 'MPEG', '.mpo': 'MPO', '.msp': 'MSP', '.palm': 'PALM', '.pcd': 'PCD', '.pdf': 'PDF', '.pxr': 'PIXAR', '.psd': 'PSD', '.bw': 'SGI', '.rgb': 'SGI', '.rgba': 'SGI', '.sgi': 'SGI', '.ras': 'SUN', '.tga': 'TGA', '.webp': 'WEBP', '.wmf': 'WMF', '.emf': 'WMF', '.xbm': 'XBM', '.xpm': 'XPM'}

As you can see, there are multiple entries for PNG format: ... '.png': 'PNG', '.apng': 'PNG' ....

Therefore, the result of _format_to_extension is unstable, because depends on order of items in the dict (ie. on order of serving them for for k, v in Image.EXTENSION.items() iteration).

vstoykov commented 5 years ago

After the merge of the fix for the PNG images, can you rebase one more time? Thank you!

kravemir commented 5 years ago

@vstoykov I did rebase it again. Now, the build is finished successfully.

kravemir commented 5 years ago

@vstoykov Can you please take (probably) the last look at the PR? Everything should be in place now. And.. I was sick, and wasn't at the computer, so there was a big delay in my rebase reaction to this PR.

vstoykov commented 5 years ago

No problem. I'm also not very free and my reactions can be slow also (for which I'm sorry).

kravemir commented 5 years ago

@vstoykov Thank you for reviewing, and merging the PR! :-)