Bug 32410

Summary: [Qt] Allow to use WebCore image decoders
Product: WebKit Reporter: Holger Freyther <zecke>
Component: ImagesAssignee: Zoltan Horvath <zoltan>
Status: RESOLVED FIXED    
Severity: Minor CC: benjamin, commit-queue, enne, hausmann, laszlo.gombos, markus, noel.gordon, s.mathur, vestbo, webkit.review.bot, yongjun.zhang, zoltan
Priority: P5 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 79414    
Bug Blocks: 80398, 80400, 83764    
Attachments:
Description Flags
Patch to use WebCore image decoders...
none
Patch to use WebCore image decoder
none
Allow to compile WebCore imagedecoders
hausmann: review-, hausmann: commit-queue-
Allow to use WebCore image decoders
none
proposed patch - the change will not affect on other ports
none
proposed patch hausmann: review+, zoltan: commit-queue-

Description Holger Freyther 2009-12-11 00:00:30 PST
For the Symbian and other ports the WebCore Image Decoders look like an interesting choiche. We avoid using the QBuffer, we get the downsampling, we more directly pick the decoder...
Comment 1 Holger Freyther 2009-12-14 02:49:22 PST
I landed the usage of setQuality(49) in r52086.
Comment 2 Holger Freyther 2009-12-14 02:50:24 PST
That is really not my day... forget the comment.
Comment 3 Holger Freyther 2009-12-14 04:11:43 PST
Created attachment 44785 [details]
Patch to use WebCore image decoders...

This is my current version of the patch. It needs to be completed.
Comment 4 Holger Freyther 2009-12-14 05:51:26 PST
The crash is due one thing we can do in the pure Qt case...

In RGBA32Buffer::asNewNativeImage we throw the QImage away... with progressive loading we are doing this while decoding... the fix should be to...

a.) guard that with the ENABLE/USE macro..
b.) Check the state of the frame and throw it away once it has been decoded
c.) Just remove it and hope someone is calling ::clear...
Comment 5 Holger Freyther 2009-12-14 21:02:04 PST
Created attachment 44838 [details]
Patch to use WebCore image decoder

This is fixing the crash. If someone wants to continue working on it and test changes please go ahead. The things that need to be changed are documented in the commit message.

To be able to land the patch we will need to:
       - Implement locking of the RGBA32Buffer from the GIFImageDecoder. So we can still throw away the QImage when it will not be used for decoding anymore.
       - Remove the ::scanLine call from getAddr as this will go through a detach.

Does anyone volunteer to carry this forward?
Comment 6 Yongjun Zhang 2009-12-29 11:35:41 PST
Hi Holger, I can take this over since I have already started looking into using webcore image decoders in Symbian.
Comment 7 Markus Goetz 2010-02-24 07:58:57 PST
Hi,
I was looking into Qt image decoders just for fun. What is the current state of your work on it, Yongjun?

Markus
Comment 8 Holger Freyther 2010-02-25 07:04:08 PST
One more issue... The current RGBA32Buffer class will call QImage::detach() for each pixel... So it would make sense to keep the image in a Vector like the others decoders are doing it too (this would also make us use fastMalloc for the imagedata), or at least keep the result of QImage::bits somewhere.
Comment 9 Yongjun Zhang 2010-02-25 07:22:32 PST
(In reply to comment #8)
> One more issue... The current RGBA32Buffer class will call QImage::detach() for
> each pixel... So it would make sense to keep the image in a Vector like the
> others decoders are doing it too (this would also make us use fastMalloc for
> the imagedata), or at least keep the result of QImage::bits somewhere.

(In reply to comment #7)
> Hi,
> I was looking into Qt image decoders just for fun. What is the current state of
> your work on it, Yongjun?
> 
> Markus

Hi Markus,

I had worked on it for a while but was dragged to other more urgent tasks.  I have a patch which fixed some issues Holger mentioned.  However I didn't get time to benchmark my patch against the original code.  It would be great if you can help with that.

I will upload my patch later.

thanks,
Yongjun
Comment 10 Holger Freyther 2010-03-02 02:05:26 PST
It is too early.
Comment 11 Holger Freyther 2010-03-02 02:05:47 PST
It is too early.
Comment 12 Benjamin Poulain 2010-11-19 04:44:03 PST
Yongjun, do you plan to finish this? Otherwise I think the task can be closed until someone want to work on it.
Comment 13 Siddharth Mathur 2011-03-26 14:02:10 PDT
Closing old bug. Please reopen if need resurfaces and someone starts work on a patch.
Comment 14 Zoltan Horvath 2012-02-20 07:38:28 PST
I reopened this bug because there were no real objections on the qtwebkit mailing list against switching to WebCore Imagedecoders: https://lists.webkit.org/pipermail/webkit-qt/2012-February/002470.html)

In that cases where WebCore does not supports the image format we'll try to fallback to QtImageDecoders.

I assign myself to the bug and I will send patches on this week.
Comment 15 Benjamin Poulain 2012-02-20 11:51:01 PST
> In that cases where WebCore does not supports the image format we'll try to fallback to QtImageDecoders.

You might not always want that. QtImageDecoders are have more security issues, and ideally you want to avoid some of them (e.g. TIFF and SVG).

See https://bugs.webkit.org/show_bug.cgi?id=38554
Comment 16 Zoltan Horvath 2012-02-21 00:31:11 PST
(In reply to comment #15)
> > In that cases where WebCore does not supports the image format we'll try to fallback to QtImageDecoders.
> 
> You might not always want that. QtImageDecoders are have more security issues, and ideally you want to avoid some of them (e.g. TIFF and SVG).
> 
> See https://bugs.webkit.org/show_bug.cgi?id=38554

Thanks for pointing me to that bug!

As I see now, we should change to WebCore imagedecoders, with an ENABLE macro guarded fallback to QtImageDecoders and when we solved the security issues we can remove the macros and use the fallback as a default behavior.

What is your opinion about this?
Comment 17 Markus Goetz 2012-02-21 04:17:46 PST
(In reply to comment #16)
> As I see now, we should change to WebCore imagedecoders, with an ENABLE macro guarded fallback to QtImageDecoders and when we solved the security issues we can remove the macros and use the fallback as a default behavior.

In general I guess it makes sense to prefer WebCore decoders as they are hopefully more secure and more optimized for the web case.
I think Qt decoders should still be used as fallback, applications might rely on that.
Comment 18 Zoltan Horvath 2012-02-21 11:48:27 PST
(In reply to comment #17)
> In general I guess it makes sense to prefer WebCore decoders as they are hopefully more secure and more optimized for the web case.
> I think Qt decoders should still be used as fallback, applications might rely on that.

Exactly, this is what I want to do. :)
Comment 19 Benjamin Poulain 2012-02-21 12:09:21 PST
> As I see now, we should change to WebCore imagedecoders, with an ENABLE macro guarded fallback to QtImageDecoders and when we solved the security issues we can remove the macros and use the fallback as a default behavior.

Sounds like a good plan.

If you have a browser based on the same library, you might want to keep a runtime check to avoid Qt image decoders for the browser.
There are nasty stuff in there, and Qt does not have a security team to verify that.
Comment 20 Zoltan Horvath 2012-02-22 02:46:39 PST
Created attachment 128165 [details]
Allow to compile WebCore imagedecoders

This patch adds ENABLE guards around QtImageDecoders only, but I set it to use it by default, so there are no behavior change. If somebody wants to test WebCore imgdecoders then just pass ENABLE_QT_IMAGE_DECODER=0 to the build.

The next step would be to put the fallback to QtImageDecoders into WebCore and set WebCore ImageDecoders to default. In this case compiling of QtImageDecoders will be disabled until we fix security issues.
Comment 21 Simon Hausmann 2012-02-22 11:59:17 PST
Comment on attachment 128165 [details]
Allow to compile WebCore imagedecoders

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

Looks good in general! I like the minimalistic approach. A few nitpicks below :)

> Source/WebCore/WebCore.pri:212
> +        $$SOURCE_DIR/platform/image-decoders/webp

Doesn't webp require another library? (just curious, don't have the sources here right now)

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:106
> +void error_exit(j_common_ptr cinfo) NO_RETURN;

Why do we need this and the other ports don't? Do we enable a compiler warning the others don't see? Is it because of -Werror? Please explain in the ChangeLog :)

> Source/WebCore/platform/image-decoders/qt/ImageFrameQt.cpp:43
> +            fmt = QImage::Format_RGB32;

The indentation of this block looks broken :)
Comment 22 Zoltan Horvath 2012-02-23 02:51:38 PST
Created attachment 128446 [details]
Allow to use WebCore image decoders

(In reply to comment #21)

> > Source/WebCore/WebCore.pri:212
> > +        $$SOURCE_DIR/platform/image-decoders/webp
> 
> Doesn't webp require another library? (just curious, don't have the sources here right now)

Yeap, you are right. You need installed libwebp on the system. So I put it into macro guards.
 
> > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:106
> > +void error_exit(j_common_ptr cinfo) NO_RETURN;
> 
> Why do we need this and the other ports don't? Do we enable a compiler warning the others don't see? Is it because of -Werror? Please explain in the ChangeLog :)

I modified the changelog! :)

> > Source/WebCore/platform/image-decoders/qt/ImageFrameQt.cpp:43
> > +            fmt = QImage::Format_RGB32;
> 
> The indentation of this block looks broken :)

Fixed.
Comment 23 Simon Hausmann 2012-02-23 03:41:38 PST
Comment on attachment 128446 [details]
Allow to use WebCore image decoders

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

r=me.

> Source/WebCore/WebCore.pri:214
> +    LIBS += -ljpeg -lpng12

It would be nice to replace the direct linkage with qmake config tests that print out a message or abort. That can be done in a follow-up patch of course :)
Comment 24 WebKit Review Bot 2012-02-23 15:37:30 PST
Comment on attachment 128446 [details]
Allow to use WebCore image decoders

Clearing flags on attachment: 128446

Committed r108685: <http://trac.webkit.org/changeset/108685>
Comment 25 WebKit Review Bot 2012-02-23 15:37:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Adrienne Walker 2012-02-23 16:06:14 PST
Rolled out in http://trac.webkit.org/changeset/108692

Broke Chrome compile on all platforms.

From http://build.chromium.org/p/chromium.webkit/builders/Win%20Builder/builds/17066/steps/compile/logs/stdio_html:
--------------------Configuration: webcore_platform - Release Win32-----------------------
Compiling...
PNGImageDecoder.cpp
JPEGImageDecoder.cpp
..\platform\image-decoders\png\PNGImageDecoder.cpp(67) : error C2144: syntax error : 'int' should be preceded by ';'
..\platform\image-decoders\png\PNGImageDecoder.cpp(67) : warning C4091: '__declspec(noreturn)' : ignored on left of 'int' when no variable is declared
..\platform\image-decoders\jpeg\JPEGImageDecoder.cpp(106) : error C2144: syntax error : 'int' should be preceded by ';'
..\platform\image-decoders\jpeg\JPEGImageDecoder.cpp(106) : warning C4091: '__declspec(noreturn)' : ignored on left of 'int' when no variable is declared
Build log was saved at "file://c:\b\build\slave\win-latest-rel\build\src\build\Release\obj\webcore_platform\BuildLog.htm"
webcore_platform - 2 error(s), 2 warning(s)

From http://build.chromium.org/p/chromium.webkit/builders/Mac%20Builder%20%28dbg%29/builds/4713/steps/compile/logs/stdio:

/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../platform/image-decoders/jpeg/JPEGImageDecoder.cpp:406:1:error: function declared 'noreturn' should not return [-Werror,-Winvalid-noreturn]
}
^
1 error generated.
/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../../../../../xcodebuild/WebCore.build/Debug/webcore_platform.build/Objects-normal/i386/PNGImageDecoder.o
/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../platform/image-decoders/png/PNGImageDecoder.cpp:71:1:error: function declared 'noreturn' should not return [-Werror,-Winvalid-noreturn]
}
^
1 error generated.
Comment 27 WebKit Commit Bot 2012-02-24 01:43:16 PST
Attachment 128446 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:106:  error_exit is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:67:  The parameter name "png" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 28 Zoltan Horvath 2012-02-24 03:04:08 PST
Created attachment 128697 [details]
proposed patch - the change will not affect on other ports

(In reply to comment #23)
> > Source/WebCore/WebCore.pri:214
> > +    LIBS += -ljpeg -lpng12
> 
> It would be nice to replace the direct linkage with qmake config tests that print out a message or abort. That can be done in a follow-up patch of course :)

Okay. I prefer to make it in a separate patch.

(In reply to comment #26)
> Rolled out in http://trac.webkit.org/changeset/108692

Thank you!

I put extra guards into the change to not break other ports.
Comment 29 WebKit Commit Bot 2012-02-24 03:06:23 PST
Attachment 128697 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:107:  error_exit is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:109:  error_exit is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 30 Zoltan Horvath 2012-02-24 03:16:45 PST
(In reply to comment #29)
> Attachment 128697 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:107:  error_exit is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:109:  error_exit is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> Total errors found: 2 in 11 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.
This patch is not about JPEGImageDecoder.cpp refactoring. Functions in JPEGImageDecoder.cpp should be renamed to fit to the guidelines, but that is not a goal of this patch.
Comment 31 Simon Hausmann 2012-02-24 05:56:39 PST
Comment on attachment 128697 [details]
proposed patch - the change will not affect on other ports

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

>> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:109
>> +void error_exit(j_common_ptr cinfo) NO_RETURN;
> 
> error_exit is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

Did you intend to remove the NO_RETURN in this branch perhaps?
Comment 32 Zoltan Horvath 2012-02-24 06:03:36 PST
Created attachment 128722 [details]
proposed patch

(In reply to comment #31)
> (From update of attachment 128697 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128697&action=review
> 
> >> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:109
> >> +void error_exit(j_common_ptr cinfo) NO_RETURN;
> > 
> > error_exit is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> 
> Did you intend to remove the NO_RETURN in this branch perhaps?

Ups. Sorry. I removed it.
Comment 33 WebKit Commit Bot 2012-02-24 06:07:19 PST
Attachment 128722 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:107:  error_exit is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 34 Simon Hausmann 2012-02-24 06:10:30 PST
Comment on attachment 128722 [details]
proposed patch

Alright, the EWS was green on the earlier patch, this one just fixes the NORETURN in the #else case. Let's give it a shot :)
Comment 35 Zoltan Horvath 2012-02-24 06:17:51 PST
Comment on attachment 128722 [details]
proposed patch

Thanks for the review. I'm going to land it by hand.
Comment 36 Zoltan Horvath 2012-02-24 06:49:54 PST
(In reply to comment #35)
> (From update of attachment 128722 [details])
> Thanks for the review. I'm going to land it by hand.

Landed in:
http://trac.webkit.org/changeset/108792
Comment 37 noel gordon 2012-02-24 23:54:34 PST
Source/WebCore/platform/image-decoders/ImageDecoder.cpp

+#if USE(WEBP)
+#include "WEBPImageDecoder.h"
+#endif

Needed?  WEBPImageDecoder.h internally checks for USE(WEBP) right?
Comment 38 Zoltan Horvath 2012-02-25 10:26:30 PST
(In reply to comment #37)
> Source/WebCore/platform/image-decoders/ImageDecoder.cpp
> 
> +#if USE(WEBP)
> +#include "WEBPImageDecoder.h"
> +#endif
> 
> Needed?  WEBPImageDecoder.h internally checks for USE(WEBP) right?

Yeap, it checks. But if I didn't put into these guards then we needed to add the directory of this file to the includepath pointlessly. 
I prefer this straightforward way. :)
Comment 39 Tor Arne Vestbø 2012-02-27 03:10:16 PST
Comment on attachment 128722 [details]
proposed patch

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

> Source/WebCore/ChangeLog:8
> +        Add ENABLE(QT_IMAGE_DECODER) guards around Qt imagedecoders and set it to default.
> +        By passing ENABLE_QT_IMAGE_DECODER=0 define to the build system, WebKit will build
> +        with WebCore's imagedecoders.

For the future, this should have been a USE() macro, not an ENABLE() macro, which is used primarily for web-facing features. From Platform.h:

USE() - use a particular third-party library or optional OS service 
ENABLE() - turn on a specific feature of WebKit
Comment 40 Tor Arne Vestbø 2012-02-27 03:30:47 PST
(In reply to comment #39)
> (From update of attachment 128722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128722&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Add ENABLE(QT_IMAGE_DECODER) guards around Qt imagedecoders and set it to default.
> > +        By passing ENABLE_QT_IMAGE_DECODER=0 define to the build system, WebKit will build
> > +        with WebCore's imagedecoders.
> 
> For the future, this should have been a USE() macro, not an ENABLE() macro, which is used primarily for web-facing features. From Platform.h:
> 
> USE() - use a particular third-party library or optional OS service 
> ENABLE() - turn on a specific feature of WebKit

Renamed in r108981
Comment 41 Zoltan Horvath 2012-02-27 07:16:02 PST
(In reply to comment #40)

> Renamed in r108981

Thank you!
Comment 42 Zoltan Horvath 2012-03-06 02:07:56 PST
I close this bug, because patches are landed, so you are allowed to compile with WebCore's imagedecoders.

For tracking the switching I opened a new bug: bug #80400

(In reply to comment #23)
> It would be nice to replace the direct linkage with qmake config tests that print out a message or abort. That can be done in a follow-up patch of course :)

For this I opened a new bug: bug #80398
Comment 43 noel gordon 2012-04-11 01:46:32 PDT
Comment on attachment 128722 [details]
proposed patch

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

> Source/WebCore/platform/MIMETypeRegistry.cpp:294
>      for (int i = 0; i < formats.size(); ++i) {

Zoltan a question: if/when QT_IMAGE_DECODER is enabled, this part of the change appears to break image encoding on the <canvas> element. QImageWriter is for used for encoding images. I don't think ENABLE(QT_IMAGE_DECODER) should change that.  Reading the code, if you enable the flag, the SupportedImageMIMETypesForEncoding registry would be empty on the QT port, right?
Comment 44 noel gordon 2012-04-12 01:21:28 PDT
meant: "if/when QT_IMAGE_DECODER is disabled ..."
Comment 45 Zoltan Horvath 2012-04-12 01:42:34 PDT
(In reply to comment #44)
> meant: "if/when QT_IMAGE_DECODER is disabled ..."

Hmm. I think QImageDecoder should not do anything with canvas, but I'm going to check this.

(In reply to comment #43)
> (From update of attachment 128722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128722&action=review
> 
> > Source/WebCore/platform/MIMETypeRegistry.cpp:294
> >      for (int i = 0; i < formats.size(); ++i) {
> 
> Zoltan a question: if/when QT_IMAGE_DECODER is enabled, this part of the change appears to break image encoding on the <canvas> element. QImageWriter is for used for encoding images. I don't think ENABLE(QT_IMAGE_DECODER) should change that.  Reading the code, if you enable the flag, the SupportedImageMIMETypesForEncoding registry would be empty on the QT port, right?

We use QImageReader for encoding images. :)

It would not be empty, check the #else part from Source/WebCore/platform/MIMETypeRegistry.cpp:246.
Comment 46 noel gordon 2012-04-12 02:29:04 PDT
(In reply to comment #45)
> (In reply to comment #44)
> > meant: "if/when QT_IMAGE_DECODER is disabled ..."
> 
> Hmm. I think QImageDecoder should not do anything with canvas, but I'm going to check this.
>
> (In reply to comment #43)
> > (From update of attachment 128722 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=128722&action=review
> > 
> > > Source/WebCore/platform/MIMETypeRegistry.cpp:294
> > >      for (int i = 0; i < formats.size(); ++i) {
> > 
> > Zoltan a question: if/when QT_IMAGE_DECODER is enabled, this part of the change appears to break image encoding on the <canvas> element. QImageWriter is for used for encoding images. I don't think ENABLE(QT_IMAGE_DECODER) should change that.  Reading the code, if you enable the flag, the SupportedImageMIMETypesForEncoding registry would be empty on the QT port, right?
> 
> We use QImageReader for encoding images. :)

Hmm? QImageWriter is in the comment I recently found in the <canvas>.toDataURL image encoder.  I think QImageReader only decodes images. 
   http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp#L398

 
> It would not be empty, check the #else part from Source/WebCore/platform/MIMETypeRegistry.cpp:246.

Sorry, I was not talking about line 246.  From the review tool, it's line 294.  As of today, it's http://trac.webkit.org/browser/trunk/Source/WebCore/platform/MIMETypeRegistry.cpp#L290
Comment 47 Zoltan Horvath 2012-04-12 02:59:02 PDT
(In reply to comment #46)
> (In reply to comment #45)
> > (In reply to comment #44)
> > > meant: "if/when QT_IMAGE_DECODER is disabled ..."
> > 
> > Hmm. I think QImageDecoder should not do anything with canvas, but I'm going to check this.
> >
> > (In reply to comment #43)
> > > (From update of attachment 128722 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=128722&action=review
> > > 
> > > > Source/WebCore/platform/MIMETypeRegistry.cpp:294
> > > >      for (int i = 0; i < formats.size(); ++i) {
> > > 
> > > Zoltan a question: if/when QT_IMAGE_DECODER is enabled, this part of the change appears to break image encoding on the <canvas> element. QImageWriter is for used for encoding images. I don't think ENABLE(QT_IMAGE_DECODER) should change that.  Reading the code, if you enable the flag, the SupportedImageMIMETypesForEncoding registry would be empty on the QT port, right?
> > 
> > We use QImageReader for encoding images. :)
> 
> Hmm? QImageWriter is in the comment I recently found in the <canvas>.toDataURL image encoder.  I think QImageReader only decodes images. 
>    http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/qt/ImageBufferQt.cpp#L398
> 
> 
> > It would not be empty, check the #else part from Source/WebCore/platform/MIMETypeRegistry.cpp:246.
> 
> Sorry, I was not talking about line 246.  From the review tool, it's line 294.  As of today, it's http://trac.webkit.org/browser/trunk/Source/WebCore/platform/MIMETypeRegistry.cpp#L290

Okay, I misread, sorry. Now it's clear for me that we are talking about encoding things. Two canvas test fail for me with disabled qimagedecoders. I'm going to examine it. I opened a new bug (#83764) for it, let's continue the discussion there.
Comment 48 noel gordon 2012-04-12 04:39:33 PDT
Right thank you, let's continue there.