Bug 32410

I landed the usage of setQuality(49) in r52086.

That is really not my day... forget the comment.

Created attachment 44785 [details] Patch to use WebCore image decoders... This is my current version of the patch. It needs to be completed.

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...

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?

Hi Holger, I can take this over since I have already started looking into using webcore image decoders in Symbian.

Hi, I was looking into Qt image decoders just for fun. What is the current state of your work on it, Yongjun? Markus

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 #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

It is too early.

It is too early.

Yongjun, do you plan to finish this? Otherwise I think the task can be closed until someone want to work on it.

Closing old bug. Please reopen if need resurfaces and someone starts work on a patch.

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.

> 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

(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?

(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.

(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. :)

> 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.

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 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 :)

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 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 on attachment 128446 [details] Allow to use WebCore image decoders Clearing flags on attachment: 128446 Committed r108685: <http://trac.webkit.org/changeset/108685>

All reviewed patches have been landed. Closing bug.

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.

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.

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.

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.

(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 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?

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.

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 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 on attachment 128722 [details] proposed patch Thanks for the review. I'm going to land it by hand.

(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

Source/WebCore/platform/image-decoders/ImageDecoder.cpp +#if USE(WEBP) +#include "WEBPImageDecoder.h" +#endif Needed? WEBPImageDecoder.h internally checks for USE(WEBP) right?

(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 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

(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

(In reply to comment #40) > Renamed in r108981 Thank you!

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 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?

meant: "if/when QT_IMAGE_DECODER is disabled ..."

(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.

(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

(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.

Right thank you, let's continue there.