JPEG decoders should understand color profiles
Created attachment 72632 [details] work in progress
Created attachment 72639 [details] patch (needs iccjpeg before landing)
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.
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.
Created attachment 72640 [details] patch (needs iccjpeg before landing)
Attachment 72640 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4932011
Committed r71152: <http://trac.webkit.org/changeset/71152>
Attachment 72640 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4919025
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.
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
Reverted: http://trac.webkit.org/changeset/71162. Apparently some of this code does run somewhere.
Committed r71174: <http://trac.webkit.org/changeset/71174>
(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?