Bug 48819 – JPEG decoders should understand color profiles

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

JPEG decoders should understand color profiles

[Adam Barth]

Created attachment 72632 [details]
work in progress

[Adam Barth]

Created attachment 72639 [details]
patch (needs iccjpeg before landing)

[Eric Seidel (no email)]

Comment on attachment 72639 [details]
patch (needs iccjpeg before landing)

View in context: https://bugs.webkit.org/attachment.cgi?id=72639&action=review

In general I support this change.  I'd like to see your comments first.

I think ColorProfile should probably not be just a Vector but rather a class wrapping a vector or similar?

Should we make ColorProfile RefCounted to avoid the copy?  I'm not sure how large these structs are, or how hot these paths are, but copies seem bad. :)

> WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:106
> +    ColorProfile colorProfile;
> +    colorProfile.append(reinterpret_cast<char*>(profile), profileLength);
> +    free(profile);

Can we just adopt it?

> WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:152
> +        for (unsigned i = JPEG_APP0; i < JPEG_COM; ++i)
> +            jpeg_save_markers(&m_info, i, 0xFFFF);

This needs a comment, methinks.

> WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:453
> +        buffer.setColorProfile(m_colorProfile);

Very confusing that there are multiple classes in this file.

> WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.h:57
> +        void setColorProfile(const ColorProfile& colorProfile) { m_colorProfile = colorProfile; }

Copying seems "bad" here.

[Adam Barth]

Comment on attachment 72639 [details]
patch (needs iccjpeg before landing)

View in context: https://bugs.webkit.org/attachment.cgi?id=72639&action=review

>> WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:106
>> +    ColorProfile colorProfile;
>> +    colorProfile.append(reinterpret_cast<char*>(profile), profileLength);
>> +    free(profile);
> 
> Can we just adopt it?

The problem is that if we adopted it, we'd free it with delete instead of free().  :(

>> WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:152
>> +        for (unsigned i = JPEG_APP0; i < JPEG_COM; ++i)
>> +            jpeg_save_markers(&m_info, i, 0xFFFF);
> 
> This needs a comment, methinks.

Okiedokes.  It's pretty mysterious.

>> WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:453
>> +        buffer.setColorProfile(m_colorProfile);
> 
> Very confusing that there are multiple classes in this file.

Yes.  We could clean up all this code.

>> WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.h:57
>> +        void setColorProfile(const ColorProfile& colorProfile) { m_colorProfile = colorProfile; }
> 
> Copying seems "bad" here.

We could make it RefCounted.  I'm tempted to do that in a separate patch though since ColorProfile is used by PNGs too now.

[Adam Barth]

Created attachment 72640 [details]
patch (needs iccjpeg before landing)

[Eric Seidel (no email)]

Attachment 72640 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4932011

[Adam Barth]

Committed r71152: <http://trac.webkit.org/changeset/71152>

[WebKit Review Bot]

Attachment 72640 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4919025

[Adam Barth]

I landed this disabled for the time being.  I'll file another bug to actually enable the code once we have the ICCJPEG library in DEPS.

[WebKit Review Bot]

http://trac.webkit.org/changeset/71152 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
canvas/philip/tests/toDataURL.jpeg.primarycolours.html
fast/backgrounds/background-repeat-computed-style.html
fast/backgrounds/size/backgroundSize17.html
fast/backgrounds/size/backgroundSize18.html
fast/backgrounds/size/backgroundSize19.html
fast/backgrounds/size/contain-and-cover.html
fast/blockflow/background-horizontal-bt.html
fast/blockflow/background-vertical-lr.html
fast/blockflow/background-vertical-rl.html
fast/canvas/toDataURL-supportedTypes.html
fast/dom/HTMLImageElement/image-load-cross-document.html
fast/dom/HTMLImageElement/image-loading-gc.html
fast/dom/HTMLImageElement/image-natural-width-height.html
fast/dom/HTMLLinkElement/onload-completion-test.html
fast/dom/beforeload/image-before-load-innerHTML.html
fast/dom/beforeload/image-before-load.html
fast/dom/beforeload/image-object-before-load-innerHTML.html
fast/dom/beforeload/image-object-before-load.html
fast/dom/beforeload/remove-image-in-beforeload-listener.html
fast/dom/inner-text-001.html

[James Robinson]

Reverted: http://trac.webkit.org/changeset/71162.  Apparently some of this code does run somewhere.

[Adam Barth]

Committed r71174: <http://trac.webkit.org/changeset/71174>

[noel gordon]

(In reply to comment #3)

> Should we make ColorProfile RefCounted to avoid the copy?  I'm not sure how large these structs are, or how hot these paths are, but copies seem bad. :)

557188 bytes of ICC profile data on this image, http://i.imgur.com/KZGJX.jpg :)
 
> > WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.h:57
> > +        void setColorProfile(const ColorProfile& colorProfile) { m_colorProfile = colorProfile; }
> 
> Copying seems "bad" here.

557188 x 2 bytes in toto then?