Bug 80400

Created attachment 130624 [details] proposed patch

The details are in the changelog of the patch!

Comment on attachment 130624 [details] proposed patch Attachment 130624 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11856076

Comment on attachment 130624 [details] proposed patch Attachment 130624 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11851114

Created attachment 130632 [details] proposed patch, chrome-ews fix I need to check some jpeg library on our EWS, please don't send the patch to EWS until that.

Comment on attachment 130632 [details] proposed patch, chrome-ews fix Attachment 130632 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11853122

Comment on attachment 130632 [details] proposed patch, chrome-ews fix View in context: https://bugs.webkit.org/attachment.cgi?id=130632&action=review This is a good start :) A few comments, in general I think there's a possibility of reducing the complexity of this patch a lot. > Source/WebCore/ChangeLog:7 > + however I add a new macro (called WTF_USE_FALLBACK_TO_QT_IMAGE_DECODER) which allows you to compile with I think it should be our goal for the Qt port to reduce the number of configurations of features we have to maintain. In this case I don't think we do need this as another option and I suggest that we simply do image decoding this way #if PLATFORM(QT). This should also make your patch slightly simpler. > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:149 > +#if PLATFORM(QT) > + m_pixmap = QPixmap(); > + m_image = QImage(); > +#endif I don't think that this block of code is needed, given that this is a constructor and that m_pixmap and m_image already have proper default constructors. > Source/WebCore/platform/image-decoders/ImageDecoder.h:338 > + virtual bool isImageDecoderQt() { return false; } This should be const, but another option would be to simply take the existing booleans in the class, compress them into a bitfield and add another bit indicating if the constructed decoder is a Qt decoder or not, removing the need for a virtual function. In an ideal world however the Qt image decoder would simply be _another_ ImageDecoder subclass that is instantiated, just like the png/jpeg/bmp/ico/etc. decoders, with as few #ifdefs in ImageDecoder.cpp as they are needed for the others (i.e. zero). I suspect that one reason for this used to be that we had this _need_ to load images into QPixmaps, where pixmaps used to be potentially server-side objects. Since this "constraint" has gone, perhaps it is possible to operate on the same data structures like the other decoders and avoid extra #ifdefs here?

Created attachment 131330 [details] proposed patch (In reply to comment #7) > I think it should be our goal for the Qt port to reduce the number of configurations of features we have to maintain. In this case I don't think we do need this as another option and I suggest that we simply do image decoding this way #if PLATFORM(QT). This should also make your patch slightly simpler. I totally agree with you, I removed the newly added (FALLBACK) and the old (WTF_USE_QT_IMAGE_DECODER) macro. > > Source/WebCore/platform/image-decoders/ImageDecoder.cpp:149 > > +#if PLATFORM(QT) > > + m_pixmap = QPixmap(); > > + m_image = QImage(); > > +#endif > > I don't think that this block of code is needed, given that this is a constructor and that m_pixmap and m_image already have proper default constructors. Yes, you're right. I removed these. > > Source/WebCore/platform/image-decoders/ImageDecoder.h:338 > > + virtual bool isImageDecoderQt() { return false; } > > This should be const, but another option would be to simply take the existing booleans in the class, compress them into a bitfield and add another bit indicating if the constructed decoder is a Qt decoder or not, removing the need for a virtual function. I find a simpler way to solve this. Do you like it? > In an ideal world however the Qt image decoder would simply be _another_ ImageDecoder subclass that is instantiated, just like the png/jpeg/bmp/ico/etc. decoders, with as few #ifdefs in ImageDecoder.cpp as they are needed for the others (i.e. zero). I try to reduce the number of ifdefs, but I left some, so this patch may need further iterations. > I suspect that one reason for this used to be that we had this _need_ to load images into QPixmaps, where pixmaps used to be potentially server-side objects. Since this "constraint" has gone, perhaps it is possible to operate on the same data structures like the other decoders and avoid extra #ifdefs here? I removed the qpixmap from ImageDecoder, so currently we use only qimage for QImageDecoder. asNewNativeImage() still returns with NativeImagePtr. Modifying NativeImagePtr from QPixmap* to QImage* would be a bigger task. What do you think of it? Notice: this patch adds build depedency for libpng-dev and libjpeg-dev, which has been installed on our EWS by Ossy already.

I used http://zoltan.sed.hu/WebKit/imgdec/formats.htm as a test site. WebCoreImageDecoder decodes only: jpeg, png, gif, bmp, ico. (svg, but it's not from imagedecoder) QImageDecoder decodes all others, depends on your Qt build, and system libraries.

Is it correct that the ImageDecoderQt.cpp is in platform/graphics/qt/? Because other ports store this file in platform/image-decoders/portname/. Related question: should I delete platform/image-decoders/qt directory as well? Because it'll be empty (depends on the first question) :-)

Comment on attachment 131330 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=131330&action=review Nice, this is a lot simpler. I think only the image storage is left as an issue. A few comments below. > Source/WebCore/platform/graphics/qt/ImageDecoderQt.cpp:238 > +// The image must not have format 8888 pre multiplied... It is unfortunately not very clear from this comment what the problem is. Could you elaborate a bit more? Is there an ASSERT we should use in the function below? > Source/WebCore/platform/graphics/qt/ImageDecoderQt.cpp:244 > +void ImageFrame::setImage(const QImage& image) > +{ > + m_image = image; > + m_size = image.size(); > + m_hasAlpha = image.hasAlphaChannel(); > +} I suppose the answer to my m_bytes question lies here. Perhaps we can make m_bytes point to the image's data and somehow make sure the vector doesn't take ownership? Alternatively perhaps we can make m_bytes our _real_ storage, get rid of m_image as a member and when using the QImageReader construct a temporary QImage around the m_bytes data that we pass to the QImageReader::read(QImage*) function. > Source/WebCore/platform/image-decoders/ImageDecoder.h:143 > -#elif PLATFORM(QT) && USE(QT_IMAGE_DECODER) > - m_image = m_pixmap.toImage(); > - m_pixmap = QPixmap(); > - return reinterpret_cast_ptr<QRgb*>(m_image.scanLine(y)) + x; > #else > return m_bytes + (y * width()) + x; Hmm, what about the qt image decoder case? Won't this break if m_image is set but m_bytes isn't?

Created attachment 132784 [details] proposed patch without Changelog update I haven't updated changelog yet, so I didn't mark it for review. I'll update the changelog in the next iteration. (In reply to comment #11) > (From update of attachment 131330 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131330&action=review > > Nice, this is a lot simpler. I think only the image storage is left as an issue. A few comments below. > > > Source/WebCore/platform/graphics/qt/ImageDecoderQt.cpp:238 > > +// The image must not have format 8888 pre multiplied... > > It is unfortunately not very clear from this comment what the problem is. Could you elaborate a bit more? Is there an ASSERT we should use in the function below? Neither for me. I just copied it with the function, it was added by you or zecke; but I removed setImage now, so it doesn't matter. > > Source/WebCore/platform/graphics/qt/ImageDecoderQt.cpp:244 > > +void ImageFrame::setImage(const QImage& image) > > +{ > > + m_image = image; > > + m_size = image.size(); > > + m_hasAlpha = image.hasAlphaChannel(); > > +} > > I suppose the answer to my m_bytes question lies here. Perhaps we can make m_bytes point to the image's data and somehow make sure the vector doesn't take ownership? Alternatively perhaps we can make m_bytes our _real_ storage, get rid of m_image as a member and when using the QImageReader construct a temporary QImage around the m_bytes data that we pass to the QImageReader::read(QImage*) function. I made m_bytes our real storage and I removed the usage of m_image. Check ImageDecoderQt.cpp::internalHandleCurrentImage(...) > > Source/WebCore/platform/image-decoders/ImageDecoder.h:143 > > -#elif PLATFORM(QT) && USE(QT_IMAGE_DECODER) > > - m_image = m_pixmap.toImage(); > > - m_pixmap = QPixmap(); > > - return reinterpret_cast_ptr<QRgb*>(m_image.scanLine(y)) + x; > > #else > > return m_bytes + (y * width()) + x; > > Hmm, what about the qt image decoder case? Won't this break if m_image is set but m_bytes isn't? getAddr was never called, so it was safe to remove. If I used m_image member I could display xpm format image, but if I tried to reproduce it from m_bytes (in asNewNativeImage) I couldn't. It displays a black rectangle only. Its type is Format_Indexed8, I couldn't display it neither when I tried to use Format_Indexed8 for it (in asNewNativeImage). Do you have any idea what can be the problem? I found this in QImage documentation btw: Warning: Painting on a QImage with the format QImage::Format_Indexed8 is not supported.

Attachment 132784 [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/graphics/qt/ImageDecoderQt.cpp:192: Missing space after , [whitespace/comma] [3] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 136448 [details] proposed patch with proper changelog I updated the patch to TOT and I modified the changelog. Questions from https://bugs.webkit.org/show_bug.cgi?id=80400#c12 still stand.

Attachment 136448 [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/graphics/qt/ImageDecoderQt.cpp:192: Missing space after , [whitespace/comma] [3] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 136448 [details] proposed patch with proper changelog View in context: https://bugs.webkit.org/attachment.cgi?id=136448&action=review r=me. There's a style nitpick to fix before landing and the diff in bugzilla also shows "new mode 100755" for Source/WebCore/platform/graphics/qt/ImageDecoderQt.h, which would an incorrect mode for the file (should remain 100644). >> Source/WebCore/platform/graphics/qt/ImageDecoderQt.cpp:192 >> + QImage image(reinterpret_cast<uchar*>(buffer->getAddr(0,0)), imageSize.width(), imageSize.height(), sizeof(ImageFrame::PixelData) * imageSize.width(), m_reader->imageFormat()); > > Missing space after , [whitespace/comma] [3] The style bot is right here :)

Created attachment 139180 [details] same as last + MIMETypeRegistry.cpp (In reply to comment #16) > (From update of attachment 136448 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136448&action=review > > r=me. There's a style nitpick to fix before landing and the diff in bugzilla also shows "new mode 100755" for Source/WebCore/platform/graphics/qt/ImageDecoderQt.h, which would an incorrect mode for the file (should remain 100644). Thanks, fixed! > >> Source/WebCore/platform/graphics/qt/ImageDecoderQt.cpp:192 > >> + QImage image(reinterpret_cast<uchar*>(buffer->getAddr(0,0)), imageSize.width(), imageSize.height(), sizeof(ImageFrame::PixelData) * imageSize.width(), m_reader->imageFormat()); > > > > Missing space after , [whitespace/comma] [3] > > The style bot is right here :) Fixed! Since I removed the QT_IMAGE_DECODER macro I needed to edit MIMETypeRegistry.cpp and Source/WebKit/blackberry/WebCoreSupport/AboutDataEnableFeatures.in (+changelogs) also. Please you have a look at these, after that we will be ready to commit the change.

Comment on attachment 139180 [details] same as last + MIMETypeRegistry.cpp View in context: https://bugs.webkit.org/attachment.cgi?id=139180&action=review > Tools/qmake/mkspecs/features/features.prf:117 > # Policy decisions: for using a particular third-party library or optional OS service Maybe changing the preceding comment or removing it would be nice, I suppose it is related to the line you just removed.

(In reply to comment #18) > (From update of attachment 139180 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=139180&action=review > > > Tools/qmake/mkspecs/features/features.prf:117 > > # Policy decisions: for using a particular third-party library or optional OS service > > Maybe changing the preceding comment or removing it would be nice, I suppose it is related to the line you just removed. Yeap, I can remove it when I land, since it's not needed temporarily. We have to wait a little bit with the landing, since the guys need to install the new dependencies on the ARM bots.

We have a compile problem on MAC, because of JPEG lib dependency. - Chromium uses its own copy in their 3rdparty directory - Apple port uses it through ImageIO.framework We have the required headers in Qt5's 3rdparty dir, but Qt5 source is not required for WK build. - we require jpeglib to be installed -> how do detect the path? -> pkgconfig -> other possibility? We discussed about using the ImageDecoderCG thing. Yes, we can use the CG image containers, but it still not resolves the jpeglib.h dependency, so I think it doesn't makes sense. What should be the direction now? Any idea?

(In reply to comment #20) > We have a compile problem on MAC, because of JPEG lib dependency. > - Chromium uses its own copy in their 3rdparty directory > - Apple port uses it through ImageIO.framework Is that accessible for us? (I see it referenced in KB articles, but it seems it disappeared from the developer docs?) > We have the required headers in Qt5's 3rdparty dir, but Qt5 source is not required for WK build. > > - we require jpeglib to be installed > -> how do detect the path? > -> pkgconfig > -> other possibility? > > We discussed about using the ImageDecoderCG thing. Yes, we can use the CG image containers, but it still not resolves the jpeglib.h dependency, so I think it doesn't makes sense. > > What should be the direction now? Any idea? In principle I think that we should do what Apple is doing here. If we cannot do that (*), then I don't see any way other than disabling the use of the webcore image decoders on Qt/Mac builds and falling back to QImageDecoder. (*) It could for example be that the frameworks in question are semi-public, meaning that we could use them and WebKit would work. But if an application is submitted to the Mac App Store that uses QtWebKit and thus such a semi-public framework, then there's a high risk of the app getting rejected. So that's an example of "cannot do" :) The more problems like these pop up, the more it looks like we should try to take a slightly "softer" approach: (1) Use WebCore decoders if we can find the external libraries in question (2) Fall back to the Qt image decoders at run-time (3) Skip (1) altogether if the libraries are not available and print a message (config tests can do that) indicating that the developer is missing out here. What do you think?

(In reply to comment #21) > (In reply to comment #20) > > We have a compile problem on MAC, because of JPEG lib dependency. > > - Chromium uses its own copy in their 3rdparty directory > > - Apple port uses it through ImageIO.framework > > Is that accessible for us? (I see it referenced in KB articles, but it seems it disappeared from the developer docs?) I'm examining it now. It seems we can use it, because I found examples for Qt-Mac usage. It (ApplicationServices) sounds cool. > In principle I think that we should do what Apple is doing here. If we cannot do that (*), then I don't see any way other than disabling the use of the webcore image decoders on Qt/Mac builds and falling back to QImageDecoder. > > > (*) It could for example be that the frameworks in question are semi-public, meaning that we could use them and WebKit would work. But if an application is submitted to the Mac App Store that uses QtWebKit and thus such a semi-public framework, then there's a high risk of the app getting rejected. So that's an example of "cannot do" :) I got it now, so this way is unacceptable. Let's consider the others. :) > The more problems like these pop up, the more it looks like we should try to take a slightly "softer" approach: > > (1) Use WebCore decoders if we can find the external libraries in question > (2) Fall back to the Qt image decoders at run-time > (3) Skip (1) altogether if the libraries are not available and print a message (config tests can do that) indicating that the developer is missing out here. If we couldn't use the ApplicationServices approach, then this way can work. > What do you think? 1-2-3 should be the almost the last chance, since it adds extra complexity both to build and to code files. I'll write soon if I managed to do something with the AS. :)

(In reply to comment #22) > I'm examining it now. It seems we can use it, because I found examples for Qt-Mac usage. It (ApplicationServices) sounds cool. From Qt5 we don't have QPixmap::fromMacCGImageRef function. It might be a problem, because it may increases the complexity. Do you know the reason why it has been removed?

(In reply to comment #22) > (In reply to comment #21) > > (In reply to comment #20) > > > We have a compile problem on MAC, because of JPEG lib dependency. > > > - Chromium uses its own copy in their 3rdparty directory > > > - Apple port uses it through ImageIO.framework > > > > Is that accessible for us? (I see it referenced in KB articles, but it seems it disappeared from the developer docs?) > > I'm examining it now. It seems we can use it, because I found examples for Qt-Mac usage. It (ApplicationServices) sounds cool. Unfortunately, this approach requires too much modification, since (e.g.) we need to modify NativeImagePtr in ImageSource.h, but we use that type in several other codes than just in ImageDecoder. This still could be a good approach, but later. (In reply to comment #21) > The more problems like these pop up, the more it looks like we should try to take a slightly "softer" approach: > > (1) Use WebCore decoders if we can find the external libraries in question > (2) Fall back to the Qt image decoders at run-time > (3) Skip (1) altogether if the libraries are not available and print a message (config tests can do that) indicating that the developer is missing out here. For fire-fighting, I'm going to come up with a solution to build only with the fallback case on qtmac, if we don't have the required libraries (+warning message); and build normally if we have. How does this sound?

(In reply to comment #24) > > The more problems like these pop up, the more it looks like we should try to take a slightly "softer" approach: > > > > (1) Use WebCore decoders if we can find the external libraries in question > > (2) Fall back to the Qt image decoders at run-time > > (3) Skip (1) altogether if the libraries are not available and print a message (config tests can do that) indicating that the developer is missing out here. > > For fire-fighting, I'm going to come up with a solution to build only with the fallback case on qtmac, if we don't have the required libraries (+warning message); and build normally if we have. How does this sound? Sounds good and like a safe approach that might come in handy on other platforms, too. (Like Windows) Hence time well spent :)

Created attachment 144042 [details] proposed patch Well... :) I found another solution to the problem: - we don't compile jpeg, png, ico (uses png) imagedecoders if we compile on mac or win - in these cases we fallback to QImageDecoder - every other platform and case uses WebCoreImageDecoder With this solution we can get rid of QT_IMAGE_DECODER macro, and a lots of platform specific code from common files. In ImageDecoder.cpp I used OS(MAC_OS_X) to determine whether the platform is mac or not, since PLATFORM(MAC) didn't work for me. Do somebody know the reason for it? Let's give a shot for the EWSs!

If you like this approach then we can improve on library checking side later.

Tor Arne enlightened me about PLATFORM macros, so I need to modify PLATFORM(WIN) to OS(WINDOWS) in the patch!

(In reply to comment #26) > Created an attachment (id=144042) [details] > proposed patch > > Well... :) > > I found another solution to the problem: > > - we don't compile jpeg, png, ico (uses png) imagedecoders if we compile on mac or win > - in these cases we fallback to QImageDecoder > - every other platform and case uses WebCoreImageDecoder > > With this solution we can get rid of QT_IMAGE_DECODER macro, and a lots of platform specific code from common files. Sounds good, but isn't it a very short distance from here to taking the result from the configure test, turning it into a HAVE(LIBPNG) (or something) and use that instead of #if OS(WIN)?

Created attachment 144737 [details] proposed patch (In reply to comment #29) > Sounds good, but isn't it a very short distance from here to taking the result from the configure test, turning it into a HAVE(LIBPNG) (or something) and use that instead of #if OS(WIN)? Done. :) Ossy tested the patch on windows for me and I tested it on mac, with qt5 and with 4.8.

Comment on attachment 144737 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=144737&action=review > Source/WebCore/platform/graphics/ImageSource.cpp:33 > I can remove this line before landing. :)

Comment on attachment 144737 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=144737&action=review > Source/WebCore/ChangeLog:12 > + becuase PNG and JPEG imagedecoders need not only these libraries, but their headers also. Qmake-config becuase -> because > Source/WebCore/ChangeLog:18 > + Reviewed by Simon Hausmann. Technically that shouldn't be there for patches marked as r? > Source/WebCore/WebCore.pri:234 > +contains(DEFINES, WTF_USE_WEBP=1) { > + INCLUDEPATH += $$SOURCE_DIR/platform/image-decoders/webp > + LIBS += -lwebp > +} The WebP support should probably also have a configure test and the use HAVE_WEBP for consistency. > Source/WebKit/blackberry/ChangeLog:6 > + Get rid off QT_IMAGE_DECODER flag. off -> of

Comment on attachment 144737 [details] proposed patch r+ because the typos can be fixed before landing and the WebP change is kind of separate. (introducing a configure test instead of making it a have-to-enable-it-explicitly-to-use-it feature)

(In reply to comment #33) > (From update of attachment 144737 [details]) > r+ because the typos can be fixed before landing and the WebP change is kind of separate. (introducing a configure test instead of making it a have-to-enable-it-explicitly-to-use-it feature) Thanks for the review! WebP conditional was in the scope of contains(DEFINES, WTF_USE_QT_IMAGE_DECODER=0), since I removed that conditional I had to leave it there. I'm going to write a config-test for it after the landing! :)

I made WEBP config tests: bug #87841

Intel Lion tests fixed in http://trac.webkit.org/changeset/118980. The patch was landed in http://trac.webkit.org/changeset/118909. Closing bug.