Alpha color spaces refactoring
ClosedPublic

Authored by dkazakov on Apr 27 2017, 8:34 AM.

Details

Summary

Rewrite alpha colorspaces using templates and make them correct

  1. Non-U8 alpha color spaces were not connected to the color conversion system. Therefore, one couldn't convert to/from them any data
  1. The code of their methods were blindly copied from U8 color space, therefore all the functions (incl. toQColor/fromQColor/difference) just read wrong values from pointers (quint8 instead of quint16 or bigger).
  1. Now all the alpha color spaces are rewritten using a common template and the same code reused for all of them, with correct pointer conversions using KoColorSpaceMaths.
  1. The new color spaces are converted to the conversion system using existing Factory::colorConversionLinks() framework. Yes, they are created using factories.

Open Questions:

  1. In KoColorSpaceRegistry there are some special cases about persistent storage of d->alphaCs and 'OwnedByRegistryRegistryDeletes' flags. I couldn't find any info about it, so it might be incorrect.

Other patches included

  • Implement reading of zipped layer transparency masks from PSD
  • Fix writing of 16-bit transparency masks to a PSD file

BUG:376836

Test Plan

Test everything related to transparency masks, painting on them and operating on them.

Please also check the lifetime of the newly added colorspaces, is it still correct?

Diff Detail

Repository
R37 Krita
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dkazakov created this revision.Apr 27 2017, 8:34 AM
rempt accepted this revision.Apr 28 2017, 9:18 AM

Just one question and two memory leaks that I could find, otherwise, fine. (And the KisPaintDevice::convertTo api really needs refactoring, we're always leaking when we use it.)

libs/pigment/colorspaces/KoAlphaColorSpace.h
222 ↗(On Diff #13854)

Even the conversions between the alpha colorspaces themselves?

plugins/impex/psd/psd_layer_record.cpp
602 ↗(On Diff #13854)

This leaks the undo command.

605 ↗(On Diff #13854)

This too.

This revision is now accepted and ready to land.Apr 28 2017, 9:18 AM
dkazakov marked 2 inline comments as done.Apr 28 2017, 10:23 AM
dkazakov added inline comments.
libs/pigment/colorspaces/KoAlphaColorSpace.h
222 ↗(On Diff #13854)

Yes, right now the conversion between AlphaU8<->AlphaU16<->AlphaU32 is done via LABA16 color space (this is the shortest between them in the graph). We can also implement custom conversion transformations and add them to colorConversionLinks() to each of the alpha color spaces. That will solve the issue at least for inter-alpha transformations.

plugins/impex/psd/psd_layer_record.cpp
602 ↗(On Diff #13854)

Thank you very much for pointing this out! Fixed! :)

This revision was automatically updated to reflect the committed changes.
dkazakov marked an inline comment as done.