[Qt] Failure on http://philip.html5.org/tests/canvas/suite/tests/2d.shadow.alpha.5.html

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[qi]

In this case, shadow should use the alpha information which was set at style in painting

ctx.fillStyle = '#f00';
ctx.fillRect(0, 0, 100, 50);
ctx.fillStyle = 'rgba(64, 0, 0, 0.5)';
ctx.shadowColor = '#00f';
ctx.shadowOffsetY = 50;
ctx.fillRect(0, -50, 100, 50);

[qi]

Created attachment 54822 [details]
patch

Set the shadow color with alpha if fillstyle has alpha information but shadow color don't have it.

[Dirk Schulze]

Comment on attachment 54822 [details]
patch

Your code replaces the alpha of shadowColor with the alpha of fillStyle. IIRC both alpha values should be multiplied:

ctx.fillStyle = '#f00';
ctx.fillRect(0, 0, 100, 50);
ctx.fillStyle = 'rgba(64, 0, 0, 0.5)';
ctx.shadowColor = 'rgba(0, 0, 255, 0.5)';
ctx.shadowOffsetY = 50;
ctx.fillRect(0, -50, 100, 50);

Would make the shadowColor (0,0,255,0.25). There is also a DRT test missing. Can be tested with getImageData, see fast/canvas

[qi]

Created attachment 61072 [details]
patch2

Created a patch for using composition mode to generate shadow for fillRect.

[Andreas Kling]

Comment on attachment 61072 [details]
patch2

Awesome that you're working on shadows :-)

>WebCore/platform/graphics/qt/GraphicsContextQt.cpp:725
> +              if (m_common->state.shadowColor.isValid()) {
This is redundant since getShadow() will check shadowColor.isValid().
The same shadowSize/shadowBlur/shadowColor variables could be used for all shadow paths.

>WebCore/platform/graphics/qt/GraphicsContextQt.cpp:731
> +                      FloatRect shadowImageRect(normalizedRect);
This variable name is a bit confusing when used together with shadowImage.rect() below.

>WebCore/platform/graphics/qt/GraphicsContextQt.cpp:734
> +                      QImage shadowImage(QSize(static_cast<int>(normalizedRect.width()), static_cast<int>(normalizedRect.height())), QImage::Format_ARGB32_Premultiplied);
The QSize part would read nicer as "roundedIntSize(normalizedRect.size())" IMO.

[qi]

Created attachment 61215 [details]
patch3

Based on comments to make a change.

[Dirk Schulze]

(In reply to comment #5)
> Created an attachment (id=61215) [details]
> patch3
> 
> Based on comments to make a change.

Is it possible to share some code? All shadow related stuff looks pretty similar, maybe you can put it into an inline function?

[qi]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=61215) [details] [details]
> > patch3
> > 
> > Based on comments to make a change.
> 
> Is it possible to share some code? All shadow related stuff looks pretty similar, maybe you can put it into an inline function?

I also think about it using a common function to handle the shadow drawing. The difficult is how can I let the function know how to draw (repeat pattern, gradient or just rect ???)

static inline void drawRectShadow(GraphicsContext* context, QPainter* p, const FloatRect& rect)
{
    FloatSize shadowSize;
    float shadowBlur;
    Color shadowColor;
    if (context->getShadow(shadowSize, shadowBlur, shadowColor)) {
        FloatRect destRect(rect);
        destRect.move(shadowSize.width(), shadowSize.height());

        QImage shadowImage(roundedIntSize(rect.size()), QImage::Format_ARGB32_Premultiplied);
        QPainter pShadow(&shadowImage);
        pShadow.setCompositionMode(QPainter::CompositionMode_Source);
        pShadow.fillRect(shadowImage.rect(), shadowColor);
        pShadow.setCompositionMode(QPainter::CompositionMode_DestinationIn);
        
        if ((m_common->state.fillPattern) {
           // need image !!!!
        } else if (m_common->state.fillGradient) {
           // need brush !!!!
        } else if (fillColor().alpha()) {
           // need brush  !!!!
        }
        pShadow.end();
        p->drawImage(destRect, shadowImage, shadowImage.rect());
    }
}

Because pattern need a image, and gradient need a brush, both of them are private member under context (m_common->state.fillPattern->tileImage()->nativeImageForCurrentFrame() or m_common->state.fillGradient->platformGradient()), so I have to pass them to the function, like:

drawRectShadow(context, painter, rect, image, brush)  or
drawRectShadow(context, painter, rect, void* image_brush)

I think both of them are ugly.

Do you have any good idea?

[qi]

Dirk, Laszlo give me some other suggestion.

How about do not create a new inline function, but move the duplicate code to the top as the common part, like:

void GraphicsContext::fillRect(const FloatRect& rect)
{
...
            // Draw shadow  -- duplicate code , common code
            FloatSize shadowSize;
            float shadowBlur;
            Color shadowColor;
            if (getShadow(shadowSize, shadowBlur, shadowColor)) {
                FloatRect destRect(normalizedRect);
                destRect.move(shadowSize.width(), shadowSize.height());

                QImage shadowImage(roundedIntSize(normalizedRect.size()), QImage::Format_ARGB32_Premultiplied);
                QPainter pShadow(&shadowImage);
                pShadow.setCompositionMode(QPainter::CompositionMode_Source);
                pShadow.fillRect(shadowImage.rect(), shadowColor);
                pShadow.setCompositionMode(QPainter::CompositionMode_DestinationIn)

 if (m_common->state.fillPattern || m_common->state.fillGradient || fillColor().alpha()) {
        if (m_common->state.fillPattern) {
             // draw shadow
             drawRepeatPattern(..);
             pShadow.end();
             p->drawImage(destRect, shadowImage, shadowImage.rect());
         }
         p->fillRect(normalizedRect, brush);
 } else if(){
 } else {

 }

[Dirk Schulze]

(In reply to comment #8)
> Dirk, Laszlo give me some other suggestion.
> 
> How about do not create a new inline function, but move the duplicate code to the top as the common part, like:
> 
> void GraphicsContext::fillRect(const FloatRect& rect)
> {
> ...
>             // Draw shadow  -- duplicate code , common code
>             FloatSize shadowSize;
>             float shadowBlur;
>             Color shadowColor;
>             if (getShadow(shadowSize, shadowBlur, shadowColor)) {
>                 FloatRect destRect(normalizedRect);
>                 destRect.move(shadowSize.width(), shadowSize.height());
> 
>                 QImage shadowImage(roundedIntSize(normalizedRect.size()), QImage::Format_ARGB32_Premultiplied);
>                 QPainter pShadow(&shadowImage);
>                 pShadow.setCompositionMode(QPainter::CompositionMode_Source);
>                 pShadow.fillRect(shadowImage.rect(), shadowColor);
>                 pShadow.setCompositionMode(QPainter::CompositionMode_DestinationIn)
> 
>  if (m_common->state.fillPattern || m_common->state.fillGradient || fillColor().alpha()) {
>         if (m_common->state.fillPattern) {
>              // draw shadow
>              drawRepeatPattern(..);
>              pShadow.end();
>              p->drawImage(destRect, shadowImage, shadowImage.rect());
>          }
>          p->fillRect(normalizedRect, brush);
>  } else if(){
>  } else {
> 
>  }

Sorry, didn't realized that we spoke about just one function fllRect() :-P In this case it should be realtively easy to share the code, not? ;-)

At first you should check if none of the operands (m_common->state.fillPattern || m_common->state.fillGradient || fillColor().alpha()) are true, and return earlier in this case.
Yes of course you have to split the part that is used by all fill operations and move it to the top. Create a boolean, that stores if you have a shadow or not and handle the rest on the actual drawing.

[qi]

Created attachment 61513 [details]
patch4

Based on comments, change the code to reduce the duplication of the code.

[Andreas Kling]

Comment on attachment 61513 [details]
patch4

>WebCore/platform/graphics/qt/GraphicsContextQt.cpp:725
> +      QImage shadowImage(roundedIntSize(normalizedRect.size()), QImage::Format_ARGB32_Premultiplied);
> +      QPainter pShadow(&shadowImage);
This will add huge performance penalty to fillRect() calls where no shadow is used.
This QImage and QPainter should only be initialized if they're actually going to be used.

r- for this, the rest of the code is shaping up nicely.

[Andreas Kling]

One way of doing this would be:

QPainter pShadow;
QImage shadowImage;

if (hasShadow) {
    shadowImage = QImage(...);
    pShadow.begin(&shadowImage);
    ...

That said, it's still adding a heap allocation (pShadow's QPainterPrivate) to the common path, which I'm not a huge fan of.

I would probably do something like a ShadowHelper class that is cheap to instantiate and only creates the painter and image when needed.

[qi]

I don't think it is a good idea to create a helper class. From comments history, you can find. Dirk asked me to create a function to handle the shadow, but I found it is difficult to do it, because for each one (repeat pattern, gradient or normal rect) has different parameter(image or brush) and different draw function. So, current proposal is just put the common code to the top to reduce the duplicate code. The common code is just part of the shadow drawing. If we put these code into a helper class, I think it will make confused. 

Actually I prefer your comment 12 's proposal.

[Andreas Kling]

(In reply to comment #13)
> I don't think it is a good idea to create a helper class. From comments history, you can find. Dirk asked me to create a function to handle the shadow, but I found it is difficult to do it, because for each one (repeat pattern, gradient or normal rect) has different parameter(image or brush) and different draw function. So, current proposal is just put the common code to the top to reduce the duplicate code. The common code is just part of the shadow drawing. If we put these code into a helper class, I think it will make confused. 

Ah, bad wording on my part. What I had in mind was a helper class to encapsulate the image and painter.

Adding rarely-used allocations to the common path of something as basic as fillRect() is a very bad idea IMO.

[qi]

How about using pointer? I think it is straight forward, how do you think about it?

[Andreas Kling]

(In reply to comment #15)
> How about using pointer? I think it is straight forward, how do you think about it?

Sure, it should be fairly clean.

[qi]

Created attachment 61687 [details]
patch5

Using pointer to reduce the cost when shadow is not required

[Andreas Kling]

Comment on attachment 61687 [details]
patch5

This is looking pretty okay. Just some nits:

>WebCore/platform/graphics/qt/GraphicsContextQt.cpp:725
> +      FloatRect destRect;
Misleading name, should be something like "shadowDestRect"

>WebCore/platform/graphics/qt/GraphicsContextQt.cpp:746
> +          // Draw shadow
Redundant comment.

>WebCore/platform/graphics/qt/GraphicsContextQt.cpp:757
> +          // Draw shadow
Ditto.

>WebCore/platform/graphics/qt/GraphicsContextQt.cpp:765
> +          // Draw shadow
Ditto.

>WebCore/platform/graphics/qt/GraphicsContextQt.cpp:774
> +      if (hasShadow) {
If you initialize shadowImage and pShadow to 0 at the start of this function, you won't need the "if (hasShadow)"

[Kenneth Rohde Christiansen]

Comment on attachment 61687 [details]
patch5

r=me, but please consider Kling's comments.

[qi]

Created attachment 61693 [details]
patch6

Minor change based on Kling's comments.

[Andreas Kling]

Committed r63614: <http://trac.webkit.org/changeset/63614>

[Andreas Kling]

Comment on attachment 61693 [details]
patch6

Removing r?, committed this as <http://trac.webkit.org/changeset/63614>