Bug 38390 – [Qt] Failed on http://philip.html5.org/tests/canvas/suite/tests/2d.drawImage.negativesource.html

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[qi]

Qt failed on http://philip.html5.org/tests/canvas/suite/tests/2d.drawImage.nowrap.html, but Safari passed.

[qi]

Also failed on:
http://philip.html5.org/tests/canvas/suite/tests/2d.drawImage.negativedest.html
http://philip.html5.org/tests/canvas/suite/tests/2d.drawImage.negativedir.html

[qi]

Created attachment 54807 [details]
patch

This bug is pretty much similar with 56619. Both of them is about negative width or height in drawing a image. The difference is 56619 was drawing a special image - canvas (drawing a canvas to another canvas), in Qt, StillImage draw is used for this purpose. This case is a normal image (draw a normal image to canvas), ImageQt draw is used for this purpose. I copy the code from StillImage to ImageQt for the same reason.

[Dirk Schulze]

(In reply to comment #2)
> Created an attachment (id=54807) [details]
> patch
> 
> This bug is pretty much similar with 56619. Both of them is about negative width or height in drawing a image. The difference is 56619 was drawing a special image - canvas (drawing a canvas to another canvas), in Qt, StillImage draw is used for this purpose. This case is a normal image (draw a normal image to canvas), ImageQt draw is used for this purpose. I copy the code from StillImage to ImageQt for the same reason.

IIRC I talked to hixie, and you also need to flip the image on negative sizes. So 56619 might be wrong too.

[qi]

(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=54807) [details] [details]
> > patch
> > 
> > This bug is pretty much similar with 56619. Both of them is about negative width or height in drawing a image. The difference is 56619 was drawing a special image - canvas (drawing a canvas to another canvas), in Qt, StillImage draw is used for this purpose. This case is a normal image (draw a normal image to canvas), ImageQt draw is used for this purpose. I copy the code from StillImage to ImageQt for the same reason.
> IIRC I talked to hixie, and you also need to flip the image on negative sizes. So 56619 might be wrong too.

Hi Dirk,

Can you explain what's your means on "flip the image on negative sizes"?

What I did is following the HTML5 spec:
The source rectangle is the rectangle whose corners are the four points (sx, sy), (sx+sw, sy), (sx+sw, sy+sh), (sx,sy+sh).
The destination rectangle is the rectangle whose corners are the four points (dx, dy), (dx+dw, dy), (dx+dw,dy+dh), (dx, dy+dh).

In this case, the sw(width) or sh(height) could be negative. Based on the spec, as long as sx+sw > 0 or sy+sh, it is still legal.
In Qt, QPainter don't support a negative width or height.

Note: 56619 is the wrong id, it is 35005.

[Dirk Schulze]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Created an attachment (id=54807) [details] [details] [details]
> > > patch
> > > 
> > > This bug is pretty much similar with 56619. Both of them is about negative width or height in drawing a image. The difference is 56619 was drawing a special image - canvas (drawing a canvas to another canvas), in Qt, StillImage draw is used for this purpose. This case is a normal image (draw a normal image to canvas), ImageQt draw is used for this purpose. I copy the code from StillImage to ImageQt for the same reason.
> > IIRC I talked to hixie, and you also need to flip the image on negative sizes. So 56619 might be wrong too.
> 
> Hi Dirk,
> 
> Can you explain what's your means on "flip the image on negative sizes"?
> 
> What I did is following the HTML5 spec:
> The source rectangle is the rectangle whose corners are the four points (sx, sy), (sx+sw, sy), (sx+sw, sy+sh), (sx,sy+sh).
> The destination rectangle is the rectangle whose corners are the four points (dx, dy), (dx+dw, dy), (dx+dw,dy+dh), (dx, dy+dh).
> 
> In this case, the sw(width) or sh(height) could be negative. Based on the spec, as long as sx+sw > 0 or sy+sh, it is still legal.
> In Qt, QPainter don't support a negative width or height.
> 
> Note: 56619 is the wrong id, it is 35005.

Of course are negative values allowed, but a neg with means the same like using scale(-1, 1); a neg height means a additional scale(1,-1);

[qi]

I think what your means is using 

     QTransform transform;
     transform.scale(1, -1); //or (-1,1)or (-1,-1)
     painter.setTransform(transform);

Is that correct?

If so, the issue is in this case:

     painter->drawPixmap(dstM, *image, srcM);    

We have source rect - srcM and destination rect - dstM. Any one of them or both of them have negative width or height. 

The transform will apply to both of them anyway. That's not what we expect. You know, maybe in some case if dstM has negative width, but srcM has no negative width or height. I think we just want to apply the transformation to dstM, not the srcM. But can we do that? Looks like in Qt, the transformation is for painter, we can't use it to transform by ourselves, is this correct?

[Dirk Schulze]

(In reply to comment #6)
> I think what your means is using 
> 
>      QTransform transform;
>      transform.scale(1, -1); //or (-1,1)or (-1,-1)
>      painter.setTransform(transform);
> 
> Is that correct?
> 
> If so, the issue is in this case:
> 
>      painter->drawPixmap(dstM, *image, srcM);    
> 
> We have source rect - srcM and destination rect - dstM. Any one of them or both of them have negative width or height. 
> 
> The transform will apply to both of them anyway. That's not what we expect. You know, maybe in some case if dstM has negative width, but srcM has no negative width or height. I think we just want to apply the transformation to dstM, not the srcM. But can we do that? Looks like in Qt, the transformation is for painter, we can't use it to transform by ourselves, is this correct?

Not sure if transforming the painter is the only solution. But it should be possible to check if just one of the width values (just one of the height values) of dstRect or srcRect is negative and transform the image or the context befor the drawing.
Please also check what Safari is doing. We also have a test in our test suite to determine this problem: fast/canvas/drawImage-with-negative-source-destination.html. The test checks both, negative values for srcRect as well as negative dstRect values. Nevertheless, the test needs a change and should use the apple logo from the resources instead of the green rect.

[Dirk Schulze]

Created attachment 56470 [details]
Test

Test for negative srcRect or dstRect.

[qi]

1. I think using transform in painter is not a good idea. The reason is source rect and destination rect may need different transform.

2. What transformation is for our purpose? I think scale might not be correct one.
   for example: if we have rect like(100, 0, -100, 50); 

   What we expect after transformation is (0, 0, 100 , 50); scale(-1, 1) doesn't do it.

3. Do you think my code is wrong from theory or just not good enough? I asked this question because I think my code is straight forward, based on the spec:

The source rectangle is the rectangle whose corners are the four points (sx, sy), (sx+sw, sy), (sx+sw, sy+sh), (sx,sy+sh).
The destination rectangle is the rectangle whose corners are the four points (dx, dy), (dx+dw, dy), (dx+dw,dy+dh), (dx, dy+dh).

If you think scale might be better solution, Actually I think it is wrong.

[Simon Hausmann]

Comment on attachment 54807 [details]
patch

In principle this looks good to me, but wouldn't it be much nicer if FloatRect had a member function, like this?

FloatRect FloatRect::normalized() const;

and then you could simply write:

painter->drawPixmap(dst.normalized(), *image, src.normalized())

What do you think?

[qi]

(In reply to comment #10)
> (From update of attachment 54807 [details])
> In principle this looks good to me, but wouldn't it be much nicer if FloatRect had a member function, like this?
> 
> FloatRect FloatRect::normalized() const;
> 
> and then you could simply write:
> 
> painter->drawPixmap(dst.normalized(), *image, src.normalized())
> 
> What do you think?

That's great idea, but we need change webkit common code. Do you think it deserve to change common code (new API)?

[qi]

Created attachment 59691 [details]
patch2

New patch based on comments.

[Andreas Kling]

Comment on attachment 59691 [details]
patch2

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 61783)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,17 @@
> [phone]  Qi Zhang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt]  Failed on http://philip.html5.org/tests/canvas/suite/tests/2d.drawImage.negativesource.html
> +
> +        Support negative width and height in canvas image draw
> +
> +        * platform/graphics/FloatRect.h:
> +        * platform/graphics/qt/FloatRectQt.cpp:
> +        (WebCore::FloatRect::normalized):
> +        * platform/graphics/qt/ImageQt.cpp:
> +        (WebCore::BitmapImage::draw):
> +
>  2010-06-24  Nate Chapin  <[e-mail]>
>  
>          Reviewed by Adam Barth.
> Index: WebCore/platform/graphics/FloatRect.h
> ===================================================================
> --- WebCore/platform/graphics/FloatRect.h	(revision 61759)
> +++ WebCore/platform/graphics/FloatRect.h	(working copy)
> @@ -143,6 +143,7 @@ public:
>  #if PLATFORM(QT)
>      FloatRect(const QRectF&);
>      operator QRectF() const;
> +    FloatRect normalized() const;
>  #endif
>  
>  #if PLATFORM(WX) && USE(WXGC)
> Index: WebCore/platform/graphics/qt/FloatRectQt.cpp
> ===================================================================
> --- WebCore/platform/graphics/qt/FloatRectQt.cpp	(revision 61759)
> +++ WebCore/platform/graphics/qt/FloatRectQt.cpp	(working copy)
> @@ -44,6 +44,21 @@ FloatRect::operator QRectF() const
>      return QRectF(x(), y(), width(), height());
>  }
>  
> +FloatRect FloatRect::normalized() const
> +{
> +    FloatRect normalizedRect = *this;
> +
> +    if (width() < 0) {
> +        normalizedRect.setX(x() + width());
> +        normalizedRect.setWidth(-width());
> +    }
> +    if (height() < 0) {
> +        normalizedRect.setY(y() + height());
> +        normalizedRect.setHeight(-height());
> +    }
> +    return normalizedRect;
> +}
> +
>  }
>  
>  // vim: ts=4 sw=4 et
> Index: WebCore/platform/graphics/qt/ImageQt.cpp
> ===================================================================
> --- WebCore/platform/graphics/qt/ImageQt.cpp	(revision 61759)
> +++ WebCore/platform/graphics/qt/ImageQt.cpp	(working copy)
> @@ -164,6 +164,9 @@ void BitmapImage::invalidatePlatformData
>  void BitmapImage::draw(GraphicsContext* ctxt, const FloatRect& dst,
>                         const FloatRect& src, ColorSpace styleColorSpace, CompositeOperator op)
>  {
> +    FloatRect normalizedDst = dst.normalized();
> +    FloatRect normalizedSrc = src.normalized();
> +
>      startAnimation();
>  
>      QPixmap* image = nativeImageForCurrentFrame();
> @@ -171,7 +174,7 @@ void BitmapImage::draw(GraphicsContext* 
>          return;
>  
>      if (mayFillWithSolidColor()) {
> -        fillWithSolidColor(ctxt, dst, solidColor(), styleColorSpace, op);
> +        fillWithSolidColor(ctxt, normalizedDst, solidColor(), styleColorSpace, op);
>          return;
>      }
>  
> @@ -191,22 +194,22 @@ void BitmapImage::draw(GraphicsContext* 
>      float shadowBlur;
>      Color shadowColor;
>      if (ctxt->getShadow(shadowSize, shadowBlur, shadowColor)) {
> -        FloatRect shadowImageRect(dst);
> +        FloatRect shadowImageRect(normalizedDst);
>          shadowImageRect.move(shadowSize.width(), shadowSize.height());
>  
> -        QImage shadowImage(QSize(static_cast<int>(src.width()), static_cast<int>(src.height())), QImage::Format_ARGB32_Premultiplied);
> +        QImage shadowImage(QSize(static_cast<int>(normalizedSrc.width()), static_cast<int>(normalizedSrc.height())), QImage::Format_ARGB32_Premultiplied);
>          QPainter p(&shadowImage);
>          p.setCompositionMode(QPainter::CompositionMode_Source);
>          p.fillRect(shadowImage.rect(), shadowColor);
>          p.setCompositionMode(QPainter::CompositionMode_DestinationIn);
> -        p.drawPixmap(dst, *image, src);
> +        p.drawPixmap(normalizedDst, *image, normalizedSrc);
>          p.end();
> -        painter->drawImage(shadowImageRect, shadowImage, src);
> +        painter->drawImage(shadowImageRect, shadowImage, normalizedSrc);
>      }
>  
>      // Test using example site at
>      // http://www.meyerweb.com/eric/css/edge/complexspiral/demo.html
> -    painter->drawPixmap(dst, *image, src);
> +    painter->drawPixmap(normalizedDst, *image, normalizedSrc);
>  
>      painter->setCompositionMode(lastCompositionMode);
>  
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog	(revision 61783)
> +++ LayoutTests/ChangeLog	(working copy)
> @@ -1,3 +1,16 @@
> [phone]  Qi Zhang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [Qt]  Failed on http://philip.html5.org/tests/canvas/suite/tests/2d.drawImage.negativesource.html
> +        
> +        Remove the following test case from Skipped:
> +        canvas/philip/tests/2d.drawImage.negativedest.html
> +        canvas/philip/tests/2d.drawImage.negativedir.html
> +        canvas/philip/tests/2d.drawImage.negativesource.html
> +
> +        * platform/qt/Skipped:
> +
>  2010-06-24  Martin Robinson  <[e-mail]>
>  
>          Unreviewed.
> Index: LayoutTests/platform/qt/Skipped
> ===================================================================
> --- LayoutTests/platform/qt/Skipped	(revision 61759)
> +++ LayoutTests/platform/qt/Skipped	(working copy)
> @@ -5247,9 +5247,6 @@ canvas/philip/tests/2d.composite.uncover
>  canvas/philip/tests/2d.composite.uncovered.pattern.source-out.html
>  canvas/philip/tests/2d.drawImage.broken.html
>  canvas/philip/tests/2d.drawImage.incomplete.html
> -canvas/philip/tests/2d.drawImage.negativedest.html
> -canvas/philip/tests/2d.drawImage.negativedir.html
> -canvas/philip/tests/2d.drawImage.negativesource.html
>  canvas/philip/tests/2d.drawImage.null.html
>  canvas/philip/tests/2d.drawImage.wrongtype.html
>  canvas/philip/tests/2d.drawImage.zerocanvas.html

WebCore/platform/graphics/FloatRect.h:146
 +      FloatRect normalized() const;
IMO this should be a cross-platform method and replace normalizeRect(FloatRect) in CanvasRenderingContext2D.cpp

Rest looks good to me.

[Andreas Kling]

Comment on attachment 59691 [details]
patch2

Heh, looks like the review-tool didn't quite work as I thought. Sorry about the mess.

[Laszlo Gombos]

Comment on attachment 59691 [details]
patch2

looks good to me, r+.

To Andreas point - I could not find another port that would need the normalized function and I do not think we should force the overhead of calling this function on other ports that do not need it.

[WebKit Commit Bot]

Comment on attachment 59691 [details]
patch2

Clearing flags on attachment: 59691

Committed r62394: <http://trac.webkit.org/changeset/62394>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.