Bug 37804 – [Qt] Image shadow doesn't work in Qt

• Component: New Bugs
• Status: RESOLVED FIXED

[qi]

Created attachment 53681 [details]
image shadow

Image shadow doesn't work in Qt
Test case see attachment.

[qi]

Only image shadow failed, others like line, fillrect, etc success.

[qi]

Created attachment 53711 [details]
ImageShadowPatch

Implement image shadow

[Laszlo Gombos]

Should this have a test-case ?

[Simon Hausmann]

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.

[qi]

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.

[WebKit Review Bot]

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.

[qi]

Created attachment 53820 [details]
patch3

Resort the "#include" due to style check failed.

[Simon Hausmann]

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.

[Dirk Schulze]

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

[qi]

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

[Dirk Schulze]

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

[Zoltan Herczeg]

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.

[qi]

(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

[qi]

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

[Zoltan Herczeg]

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

[Dirk Schulze]

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

[qi]

Created attachment 56105 [details]
patch4

Using composition mode to implement image shadow.

[Dirk Schulze]

Comment on attachment 53820 [details]
patch3

The bug report log looks a bit confusing. 3rd patch got r-.

[Dirk Schulze]

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?

[qi]

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

[Dirk Schulze]

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

[qi]

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

[Dirk Schulze]

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

[Kenneth Rohde Christiansen]

QImage on the other hand might result in slighly different rendering as painting to a QImage uses the raster engine.

[Kenneth Rohde Christiansen]

Why no blur? is the shadow always to the right/bottom? Do you have any tests? layout tests, pages using this?

[qi]

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)

[Dirk Schulze]

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

[qi]

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.

[Dirk Schulze]

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

[qi]

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.

[qi]

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.

[Dirk Schulze]

Comment on attachment 57593 [details]
patch5

r=me

[WebKit Commit Bot]

Comment on attachment 57593 [details]
patch5

Clearing flags on attachment: 57593

Committed r60546: <http://trac.webkit.org/changeset/60546>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/60546 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/60544
http://trac.webkit.org/changeset/60545
http://trac.webkit.org/changeset/60546
http://trac.webkit.org/changeset/60547
http://trac.webkit.org/changeset/60543

[Eric Seidel (no email)]

The likely culprit of the crasher.

[Eric Seidel (no email)]

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.

[Csaba Osztrogonác]

r60546 is absolutely innocent, I think r60547 is the culprit. I'll check it.

[Simon Hausmann]

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?

[Dirk Schulze]

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

[qi]

I will try Simon's idea.

[qi]

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.

[Kenneth Rohde Christiansen]

Start your app with '-graphicssystem raster'

[qi]

Created attachment 58061 [details]
shadow on pixmap 2

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