[webkit-reviews] review granted: [Bug 81974] Adds color management support to Chromium port. : [Attachment 140828] Patch

[bugzilla-daemon at webkit.org]

Tony Payne <tpayne at chromium.org> has granted  review:
Bug 81974: Adds color management support to Chromium port.
https://bugs.webkit.org/show_bug.cgi?id=81974

Attachment 140828: Patch
https://bugs.webkit.org/attachment.cgi?id=140828&action=review

------- Additional Comments from Tony Payne <tpayne at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=140828&action=review


>> Source/WTF/wtf/Platform.h:467
>> +#define WTF_USE_ICCJPEG
> 
> #define WTF_USE_ICCJPEG 1

Done

>> Source/WebCore/WebCore.gyp/WebCore.gyp:1087
>> +	    '<(chromium_src_dir)/third_party/qcms/qcms.gyp:qcms',
> 
> Again you need to modify this file in three places, not one.

Done, I think. qcms is included in each of the places iccjpeg is included.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:48
>> +#if USE(QCMS)
> 
> Remove these lines 48-51.

Done.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:195
>>	    ColorProfile m_colorProfile;
> 
> Is this ImageFrame member used anymore?  See comments below for the
skia/ImageDecoderSkia.cpp ImageFrame::setColorProfile() function.  Think this
variable can go, and its USE(QCMS) wrapping with it.

Removed.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:229
>> +#if USE(QCMS)
> 
> Remove these lines: 229-234, 236

Done.

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:361
>>	    ColorProfile m_colorProfile;
> 
> Prefix this line with // FIXME: Do we need m_colorProfile anymore, on any
port?

It looks to me like it is still needed for
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/We
bCore/platform/image-decoders/cg/ImageDecoderCG.cpp&exact_package=chromium&q=m_
colorProfile&type=cs&l=96

>> Source/WebCore/platform/image-decoders/ImageDecoder.h:365
>> +	    qcms_transform* m_transform;
> 
> Note an ImageDecoder has a long life-time compared to it's image reader.  Do
these variables need to out-live the reader class for some reason?  Did you
consider making them members of JPEGImageReader and PNGImageReader so they
could be deleted as soon as an image is decoded?  If we don't do that, then by
rights we need to compute the size of the data held by these color correction
variables and tell the browser's live decoded resources cache about that.  This
is a failing of the existing design I note: no one tells the live decoded
resources cache about ImageFrame::m_colorProfile.size() or
ImageDecoder::m_colorProfile.size(), and they can be much larger than the
decoded image bytes.
> 
> For now, move these out of here: make them members of JPEGImageDecoder and
PNGImageDecoder respectively.  That'll allow us to move them into their reader
classes if needed, and also minimise the code changes to the current file.

Okay. I'd actually like m_sRGB to have a much longer lifetime, roughly the
extent of the life of the application.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:50
>> +#include "third_party/qcms/src/qcms.h"
> 
> #include "qcms.h" and everywhere else you see third_party/qcms/...

Done.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:541
>> +		    return false;
> 
> Say we decode 100 rows and then bail (return false) awaiting more network
data.  100 rows of uncorrected image data will we painted to the screen.
http://crbug/115079

Fixed. Transforming by-row or all at once has no effect on performance as long
as the output transform is precached.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:544
>> +	      if (m_transform) {
> 
> Modulo the above, this looks like a great place to measure qcms transform
throughput during your testing.

I don't trust manual testing. I'd really like a performance test that can be
run as part of the normal suite of tests. Still have no idea what the right way
to do that is...

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:569
>> +		qcms_transform_data(m_transform, samples, samples, width);
> 
> |samples| has an extra level of indirection to the row data, so I think you
want
> 
> #if USE(QCMS)
>     if (m_transform)
>	  qcms_transform_data(m_transform, *samples, *samples,
info->output_width);
> #endif
> 
>     int width = m_scaled ? m_scaledColumns.size() : info->output_width;
>     ...

Done.

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:621
>> +					    QCMS_DATA_BGRA_8,
> 
> QCMS_DATA_BGRA_8 is not defined by qcms.  Know you're aware of qcms's pixel
format issues (no BGRX support) as something to be fixed, and that this was
your first time dealing with the PNG and JPEG decoders.  I must say you've made
a decent stab at add color correction to them both.

I took this stab at fixing the missing BGRA feature:
http://codereview.chromium.org/10384114/

Yours looks like it only takes RGBx input. Not sure how that is going to work
with the JPEG turbo-swizzle

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:117
>> +	    , m_cmsRow(0)
> 
> Good you've spotted the need for this (the color correction of interlaced PNG
would be broken without it).
> 
> Please call it m_rowBuffer.  Let's remove all the #if USE(QCMS) wrapping from
it here and then the same ...

Since we are only allocating the rowBuffer if transform exists, rowBuffer will
only be used when USE(QCMS). Are you sure you want to define it when
!USE(QCMS)?

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:196
>> +#endif
> 
> ... through to here.	It's just an m_rowBuffer we have available if we need
it.

Done

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:321
>> +	if ((colorType == PNG_COLOR_TYPE_PALETTE || colorType ==
PNG_COLOR_TYPE_RGB || colorType == PNG_COLOR_TYPE_RGB_ALPHA) &&
!m_ignoreGammaAndColorProfile) {
> 
> Why add PNG_COLOR_TYPE_PALETTE, why change behavior?	Unless you intend add a
test now, I'd fix this later.

The sample page you linked (http://www.libpng.org/pub/png/png-colortest.html)
uses palette colors. Without this change, we'll still fail that test.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:330
>> +#if USE(QCMS)
> 
> Looks similar to the JPEGImageDecoder::setColorProfile() function body though
here.  Seems the PNGImageDecoder wants a similar function?  I should say that
neither want it as member function.

Created a createColorTransform(colorProfile) in both of the Readers.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:422
>> +#if USE(QCMS)
> 
> Create this temporary row buffer only if we have a transform.
> 
> if (m_transform) {
>     m_reader->createCmsRow(colorChannels * size().width());
>     if (!m_reader->cmsRow()) {
>	  ...

Done.

>> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:499
>> +#endif
> 
> These lines (487-499) could be simplified:
> 
> #if USE(QCMS)
>     if (m_transform) {
>	  qcms_transform_data(m_transform, row, m_reader->cmsRow(),
size().width());
>	  row = m_reader->cmsRow();
>     }
> #endif
> 
> and then none of changes below would be needed.

The only difference is we'll be pulling alpha from the output of
qcms_transform_data. I do not know why Mozilla makes a point of reading alpha
from the original buffer. I have not seen any place where qcms fails to pass
the alpha through to the output.

Done

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:27
>> +#include <algorithm>
> 
> You're not using this, let's remove it.

Done

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:32
>>  #include "NotImplemented.h"
> 
> Remove this include.

Done

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:117
>>	m_colorProfile = colorProfile;
> 
> Is ImageFrame::m_colorProfile used anymore?  I think this function might now
be written as
> 
> void ImageFrame::setColorProfile(const ColorProfile& colorProfile)
> {
>     // FIXME: Do we need this ImageFrame function anymore, on any port?
>     UNUSED_PARAM(colorProfile);
> }

Done. It looks to me like ImageDecoderCG still uses this method.

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:126
>>	if (m_status == FrameComplete) {
> 
> Style nit: one line if body, don't use { in that case.

Done.

>> Source/WebCore/platform/image-decoders/skia/ImageDecoderSkia.cpp:-162
>> -	    // premultiplied, so don't apply the color profile if it isn't.
> 
> Hope you've read an understood that comment.

I think I understand it, but I could be sorely mistaken.