Bug 37804 - [Qt] Image shadow doesn't work in Qt
Summary: [Qt] Image shadow doesn't work in Qt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: qi
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-04-19 08:53 PDT by qi
Modified: 2010-06-07 11:49 PDT (History)
11 users (show)

See Also:


Attachments
image shadow (691 bytes, text/html)
2010-04-19 08:53 PDT, qi
no flags Details
ImageShadowPatch (1.93 KB, patch)
2010-04-19 13:17 PDT, qi
hausmann: review-
Details | Formatted Diff | Diff
patch2 (1.77 KB, patch)
2010-04-20 07:18 PDT, qi
no flags Details | Formatted Diff | Diff
patch3 (1.97 KB, patch)
2010-04-20 08:08 PDT, qi
krit: review-
Details | Formatted Diff | Diff
patch4 (2.27 KB, patch)
2010-05-14 13:47 PDT, qi
krit: review-
Details | Formatted Diff | Diff
patch5 (3.29 KB, patch)
2010-06-01 13:59 PDT, qi
no flags Details | Formatted Diff | Diff
shadow on pixmap (35.49 KB, image/x-bmp)
2010-06-07 08:45 PDT, qi
no flags Details
shadow on pixmap 2 (39.13 KB, image/x-bmp)
2010-06-07 11:49 PDT, qi
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description qi 2010-04-19 08:53:40 PDT
Created attachment 53681 [details]
image shadow

Image shadow doesn't work in Qt
Test case see attachment.
Comment 1 qi 2010-04-19 09:06:19 PDT
Only image shadow failed, others like line, fillrect, etc success.
Comment 2 qi 2010-04-19 13:17:16 PDT
Created attachment 53711 [details]
ImageShadowPatch

Implement image shadow
Comment 3 Laszlo Gombos 2010-04-19 14:34:45 PDT
Should this have a test-case ?
Comment 4 Simon Hausmann 2010-04-19 16:20:02 PDT
Comment on attachment 53711 [details]
ImageShadowPatch



> +    IntSize shadowSize;
> +    int shadowBlur;
> +    Color shadowColor;
> +    if (ctxt->getShadow(shadowSize, shadowBlur, shadowColor)) {
> +        QImage shadowImage = image->toImage().convertToFormat(QImage::Format_ARGB32_Premultiplied);
> +        FloatRect shadowM(dst);
> +
> +        shadowM.move(shadowSize.width(), shadowSize.height());
> +        // set image color
> +        for (int x = 0; x < shadowImage.height(); x++) {
> +            for (int y = 0; y < shadowImage.width(); y++) {
> +                if (qRed(shadowImage.pixel(x, y)) || qGreen(shadowImage.pixel(x, y)) || qBlue(shadowImage.pixel(x, y)))

Why do you do a conversion to ARGB_Premultiplied?

Also it seems strange to carefully test the RGB components individually instead of just masking
out the alpha component and checking the remainder for != 0.

The most worrying part about this code however is that it means we have to do a conversion from a pixmap
to an image on every paint. That means on systems where QPixmap is implemented as surface that lives in
video memory or in a different process (X11) this is a very expensive operation.

> +                    shadowImage.setPixel(x, y, shadowColor.rgb());

Please don't call setPixel() in an inner loop. As the documentation of QImage::setPixel explains, this is a very
expensive function to call.
Comment 5 qi 2010-04-20 07:18:03 PDT
Created attachment 53814 [details]
patch2

Created a new patch based on Simon's comments.

1. I try to change the image pixel color, the orginal code (setpixel) come from a qt example, it is not efficient.
2. Currently, I created a bitmap first then change the color table.

Appreciate Simon's comments.
Comment 6 WebKit Review Bot 2010-04-20 07:19:50 PDT
Attachment 53814 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/qt/ImageQt.cpp:48:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 qi 2010-04-20 08:08:21 PDT
Created attachment 53820 [details]
patch3

Resort the "#include" due to style check failed.
Comment 8 Simon Hausmann 2010-04-20 17:23:19 PDT
Comment on attachment 53814 [details]
patch2

Marking this attachment as obsolete as it appears that attachment number three addresses the style issues of this patch.
Comment 9 Dirk Schulze 2010-05-11 01:40:38 PDT
I have a question to this bug. IIRC this doesn't blur anything. Why do you use images at all for the shadow. Can't you just draw a rect with the shadow color? Sorry if I misunderstand somehting here :-)
Comment 10 qi 2010-05-11 06:09:22 PDT
(In reply to comment #9)
> I have a question to this bug. IIRC this doesn't blur anything. Why do you use images at all for the shadow. Can't you just draw a rect with the shadow color? Sorry if I misunderstand somehting here :-)

This case is for image shadow. Almost everything can have a shadow, for example, text, line, rect etc. Currently, our implementation is redraw text, line with a shadow color. For example, redraw a rect with the shadow color. Currently, we didn't implement the image shadow. That's this patch purpose. Image shadow is different with other shadow. We can't just redraw image, because image contains a lot of color inside (we want to keep the image shape, but change all the color to the shadow color), that' what I did and what you find.
Comment 11 Dirk Schulze 2010-05-11 06:41:06 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > I have a question to this bug. IIRC this doesn't blur anything. Why do you use images at all for the shadow. Can't you just draw a rect with the shadow color? Sorry if I misunderstand somehting here :-)
> 
> This case is for image shadow. Almost everything can have a shadow, for example, text, line, rect etc. Currently, our implementation is redraw text, line with a shadow color. For example, redraw a rect with the shadow color. Currently, we didn't implement the image shadow. That's this patch purpose. Image shadow is different with other shadow. We can't just redraw image, because image contains a lot of color inside (we want to keep the image shape, but change all the color to the shadow color), that' what I did and what you find.

I want to know what you'll see with your shadow code, just a simple rect, filled with the shadow color? Or does it have the same affect like masking? That means all colors turn to the shadow color, but the opacity/alpha is still availlable?

If your code just fill a rect, we don't need all this image manipulations and just draw the rect ourself.
Comment 12 Zoltan Herczeg 2010-05-11 06:54:23 PDT
QBitmap is 1 bit / pixel map, so the alpha channel has no fading effect on it. If we want to keep the alpha channel, we need to use porter duff based painting.
Comment 13 qi 2010-05-11 07:23:52 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > I have a question to this bug. IIRC this doesn't blur anything. Why do you use images at all for the shadow. Can't you just draw a rect with the shadow color? Sorry if I misunderstand somehting here :-)
> > 
> > This case is for image shadow. Almost everything can have a shadow, for example, text, line, rect etc. Currently, our implementation is redraw text, line with a shadow color. For example, redraw a rect with the shadow color. Currently, we didn't implement the image shadow. That's this patch purpose. Image shadow is different with other shadow. We can't just redraw image, because image contains a lot of color inside (we want to keep the image shape, but change all the color to the shadow color), that' what I did and what you find.
> I want to know what you'll see with your shadow code, just a simple rect, filled with the shadow color? Or does it have the same affect like masking? That means all colors turn to the shadow color, but the opacity/alpha is still availlable?
> If your code just fill a rect, we don't need all this image manipulations and just draw the rect ourself.

My code is only for image shadow, that's why I need to manipulate image. For rect, I didn't change it, it just draw the rect itself with offset
Comment 14 qi 2010-05-11 07:32:53 PDT
(In reply to comment #12)
> QBitmap is 1 bit / pixel map, so the alpha channel has no fading effect on it. If we want to keep the alpha channel, we need to use porter duff based painting.

This is a good question. Currently, all the shadow didn't concern about the element(image, rect, text) color. We just set the shadow color as provided color. However, I read the spec again, I found that "Multiply the alpha component of every pixel in B by the alpha component of the color of
shadowColorp" (4.8.10.1.6). As I understand, we need to combine the element color with shadow color if shadow color has alpha.
Comment 15 Zoltan Herczeg 2010-05-11 08:41:11 PDT
You could use porter-duff painting modes:

http://doc.qt.nokia.com/4.6/qpainter.html#CompositionMode-enum

And you might be interested in the following patch:

https://bugs.webkit.org/show_bug.cgi?id=24289
Comment 16 Dirk Schulze 2010-05-11 09:27:46 PDT
(In reply to comment #15)
> You could use porter-duff painting modes:
> 
> http://doc.qt.nokia.com/4.6/qpainter.html#CompositionMode-enum
> 
> And you might be interested in the following patch:
> 
> https://bugs.webkit.org/show_bug.cgi?id=24289

I think using a rect (filled with the shadow color) and composite it with the image and the operator source-in, is faster than using clipToImageBuffer (which also uses a different AlphaLayer).

Nevertheless, that means the current patch for review is not what we want. So either we use the compositing or just draw a rect.
Comment 17 qi 2010-05-14 13:47:11 PDT
Created attachment 56105 [details]
patch4

Using composition mode to implement image shadow.
Comment 18 Dirk Schulze 2010-05-18 09:26:24 PDT
Comment on attachment 53820 [details]
patch3

The bug report log looks a bit confusing. 3rd patch got r-.
Comment 19 Dirk Schulze 2010-05-18 09:37:00 PDT
Comment on attachment 56105 [details]
patch4

WebCore/platform/graphics/qt/ImageQt.cpp:194
 +          FloatRect shadowM(dst);
This name is not the best choice. How about shadowImageRect?

WebCore/platform/graphics/qt/ImageQt.cpp:198
 +          QPainter p(&shadowImage);
Isn't a bit expansive to create a new Image, create a new context of this image, set the compositing, take the original image draw it into the context of the shadowImage and draw this shadowImage to the real painter?

Isn't it better, to save the current composite operator of painter, make the drawing directly on the painter (without a intermediate image) and set the comp-op of the painter back?
Comment 20 qi 2010-05-18 10:05:28 PDT
(In reply to comment #19)
> (From update of attachment 56105 [details])
> WebCore/platform/graphics/qt/ImageQt.cpp:194
>  +          FloatRect shadowM(dst);
> This name is not the best choice. How about shadowImageRect?
> 
> WebCore/platform/graphics/qt/ImageQt.cpp:198
>  +          QPainter p(&shadowImage);
> Isn't a bit expansive to create a new Image, create a new context of this image, set the compositing, take the original image draw it into the context of the shadowImage and draw this shadowImage to the real painter?
> 
> Isn't it better, to save the current composite operator of painter, make the drawing directly on the painter (without a intermediate image) and set the comp-op of the painter back?

In theory, I don't think it is OK to draw directly. 
Currently, what we want to do is drawing a shadow to canvas with current composite operator (let's say composite A). That's mean we will apply composite A between shadow and background.
If we draw image and pure color rect directly to canvas with composite operator B (sourceIn), that means we apply the composite B with background and image and rect. I think that's wrong, do you think so?
Comment 21 Dirk Schulze 2010-05-18 10:16:39 PDT
(In reply to comment #20)
> In theory, I don't think it is OK to draw directly. 
> Currently, what we want to do is drawing a shadow to canvas with current composite operator (let's say composite A). That's mean we will apply composite A between shadow and background.
> If we draw image and pure color rect directly to canvas with composite operator B (sourceIn), that means we apply the composite B with background and image and rect. I think that's wrong, do you think so?

What about surrounding it with save/restore? Isn't this still faster than your operation?
Comment 22 qi 2010-05-18 10:28:34 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > In theory, I don't think it is OK to draw directly. 
> > Currently, what we want to do is drawing a shadow to canvas with current composite operator (let's say composite A). That's mean we will apply composite A between shadow and background.
> > If we draw image and pure color rect directly to canvas with composite operator B (sourceIn), that means we apply the composite B with background and image and rect. I think that's wrong, do you think so?
> 
> What about surrounding it with save/restore? Isn't this still faster than your operation?

We can save current composite operator and push in sourceIn operator, but how can we stop image and rect "sourceIn" with background? 

Maybe I missunderstand something????, let me make it clear:

1. save current composite operator
2. set current composite operator to "sourceIn"
3. paint image     ---  I think image will "sourceIn" background
4. paint rect      ---  same thing
5. set current composite operator back    -- we didn't apply current operator

Basically, the difference is:

(image "sourceIn" rect) "current mode" background
rect "sourceIn" (image "sourceIn" background)
Comment 23 Dirk Schulze 2010-05-19 11:57:56 PDT
(In reply to comment #22)
Talked with Qi and after checking the code with some tests, it shows that save/restore can't fix all issues. So I also don't see another way than either use QImage (like Qi did), or TransparencyLayers like Zoltan suggested.
I'm not sure, but maybe the QImage-version is the faster way, but it might with clipToImageBuffer it might be easier to use blur later. WebKitGtk is already using the GaussianBlur filter from SVG to draw shadows. Any thoughts?
Comment 24 Kenneth Rohde Christiansen 2010-05-19 12:03:15 PDT
QImage on the other hand might result in slighly different rendering as painting to a QImage uses the raster engine.
Comment 25 Kenneth Rohde Christiansen 2010-05-19 13:07:09 PDT
Why no blur? is the shadow always to the right/bottom? Do you have any tests? layout tests, pages using this?
Comment 26 qi 2010-05-19 13:17:42 PDT
Blur functionality is missing now. Not only for image shadow, but for all shadow. Blur is not in this scope.
Shadow can be left top too.
I think Dirk ask me to create a layout test (not sure)
Comment 27 Dirk Schulze 2010-05-31 13:17:24 PDT
(In reply to comment #26)
> Blur functionality is missing now. Not only for image shadow, but for all shadow. Blur is not in this scope.
> Shadow can be left top too.
> I think Dirk ask me to create a layout test (not sure)

Yes, a simple canvas test is missing.
Comment 28 qi 2010-06-01 05:42:06 PDT
Actually, the whole philip tests case went into layout test. My bug come from there, I will enable a test case and make a new patch.
Comment 29 Dirk Schulze 2010-06-01 05:55:29 PDT
(In reply to comment #28)
> Actually, the whole philip tests case went into layout test. My bug come from there, I will enable a test case and make a new patch.

Ok, you don't need to add a new test, if we already have a test in LayoutTests. But shouldn't your patch change some results?
Comment 30 qi 2010-06-01 05:58:38 PDT
yes, I will not add a new test case, but I will enable one, since it is failed and it is in the skipped list. I will go through layout test to make sure everything OK.
Comment 31 qi 2010-06-01 13:59:05 PDT
Created attachment 57593 [details]
patch5

1. Change the variable name based on the Dirk's comment;
2. Remove canvas/philip/tests/2d.shadow.image.basic.html from Skipped list;
3. Add canvas/philip/tests/2d.shadow.image.transparent.1.html into Skipped list;
   The reason is: This case using a transparent image, which Qt display as a black image. 
   This is known issue on: http://bugreports.qt.nokia.com/browse/QTBUG-9072
   Based on spec, if the image is transparent, we should not draw shadow for it. Before this
   patch, we didn't draw image shadow, so it pass this case. But after this patch, we will
   draw image shadow, somehow Qt display it as a black image instead of the transparent, that
   cause us to draw a shadow! So, this is not canvas issue, it is Qt issue.
Comment 32 Dirk Schulze 2010-06-01 21:41:53 PDT
Comment on attachment 57593 [details]
patch5

r=me
Comment 33 WebKit Commit Bot 2010-06-02 01:30:49 PDT
Comment on attachment 57593 [details]
patch5

Clearing flags on attachment: 57593

Committed r60546: <http://trac.webkit.org/changeset/60546>
Comment 34 WebKit Commit Bot 2010-06-02 01:30:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Eric Seidel (no email) 2010-06-02 02:27:01 PDT
The likely culprit of the crasher.
Comment 37 Eric Seidel (no email) 2010-06-02 02:28:16 PDT
http://build.webkit.org/results/Qt%20Linux%20Release/r60549%20(12686)/fast/overflow/overflow-with-local-background-attachment-stderr.txt
is the crash, which I suspect is a result of this bug.
Comment 38 Csaba Osztrogonác 2010-06-02 02:47:14 PDT
r60546 is absolutely innocent, I think r60547 is the culprit. I'll check it.
Comment 39 Simon Hausmann 2010-06-03 02:06:09 PDT
Comment on attachment 57593 [details]
patch5

WebCore/platform/graphics/qt/ImageQt.cpp:197
 +          QImage shadowImage(QSize(static_cast<int>(src.width()), static_cast<int>(src.height())), QImage::Format_ARGB32_Premultiplied);


WebCore/platform/graphics/qt/ImageQt.cpp:204
 +          painter->drawImage(shadowImageRect, shadowImage, src);
Is there any chance these two occurrences of image painting could be turned into QPixmaps?

With the current code the first drawPixmap call downloads the image from video memory into main memory and the following drawImage call uploads back. Wouldn't it be more efficient to let the shadowImage be a QPixmap and replace the drawImage call at the end with drawPimap?
Comment 40 Dirk Schulze 2010-06-04 07:45:55 PDT
(In reply to comment #39)
> (From update of attachment 57593 [details])
> WebCore/platform/graphics/qt/ImageQt.cpp:197
>  +          QImage shadowImage(QSize(static_cast<int>(src.width()), static_cast<int>(src.height())), QImage::Format_ARGB32_Premultiplied);
> 
> 
> WebCore/platform/graphics/qt/ImageQt.cpp:204
>  +          painter->drawImage(shadowImageRect, shadowImage, src);
> Is there any chance these two occurrences of image painting could be turned into QPixmaps?
> 
> With the current code the first drawPixmap call downloads the image from video memory into main memory and the following drawImage call uploads back. Wouldn't it be more efficient to let the shadowImage be a QPixmap and replace the drawImage call at the end with drawPimap?

Qi, what do you think?
Comment 41 qi 2010-06-04 07:50:50 PDT
I will try Simon's idea.
Comment 42 qi 2010-06-07 08:45:16 PDT
Created attachment 58034 [details]
shadow on pixmap

I try the following code:


        QPixmap shadowImage(QSize(static_cast<int>(src.width()), static_cast<int>(src.height())));
        QPainter p(&shadowImage);
        p.setCompositionMode(QPainter::CompositionMode_Source);
        p.fillRect(shadowImage.rect(), shadowColor);
        p.setCompositionMode(QPainter::CompositionMode_DestinationIn);
        p.drawPixmap(dst, *image, src);
        p.end();
        painter->drawPixmap(shadowImageRect, shadowImage, src);

I got a weird shadow which has a black background instead of the transparent background, I think it possible X11 issue????? See attachment.
Comment 43 Kenneth Rohde Christiansen 2010-06-07 11:18:22 PDT
Start your app with '-graphicssystem raster'
Comment 44 qi 2010-06-07 11:49:44 PDT
Created attachment 58061 [details]
shadow on pixmap 2

Backgroud color change to white after I started with '-graphicssystem raster'. See attachment.