603488 - Canvas 'drawImage()' should use imgIContainer::Draw() instead of rasterizing to a temporary surface

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Daniel Holbert [:dholbert]]

See bug 589558 comment 6 & bug 589558 comment 7

Comment 2

[Daniel Holbert [:dholbert]]

attachment 523826 [details] is a nice testcase for this - setting it in URL field.

Comment 3

[kerryliu]

Attachment: test case

Another test case, you can easily see the difference if you compare ff to ie9 or chrome.

Comment 4

[Gavin Kistner]

Another test case: http://phrogz.net/SVG/svg_to_img_to_canvas.html

The bottom table draws from the same source img-referencing-svg as the middle table, but specifies a larger drawImage() size. In Firefox v8 the bottom table is noticeably pixelated, while Chrome v16 draws perfectly anti-aliased graphics. (IE9 draws without pixelation, too, but its source images are broken in many cases.)

Comment 5

[Lea Verou]

A simpler test case: http://lea.verou.me/tests/svg-drawimage/img.upscaling.html

Comment 6

[Daniel Holbert [:dholbert]]

Unassigning, since I'm not actively working on this.

Comment 7

[James Kolb]

I don't have the privilege to assign myself to this bug, but I'm going to try and write a fix for it. I should have a patch written by sometime this weekend.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Careful what you wish for :P

Comment 9

[James Kolb]

Attachment: Patch 1: Refactor nsLayoutUtils to expose imgIContainer

This breaks up the work of GetSurfaceFromElement into two functions (GetImgIContainerFromElement and GetSurfaceFromImgIContainer). This way they can be accessed individually to get an element's imgIContainer.

Comment 10

[James Kolb]

Attachment: Patch 2: Draw vector images using imgIContainer::Draw()

This draws vector images to the canvas directly instead of rasterizing to a temporary surface. Raster images are still drawn the old way so that they can be stored in the canvas cache.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 753102 [details] [diff] [review]
Patch 1: Refactor nsLayoutUtils to expose imgIContainer

Review of attachment 753102 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!!!

::: layout/base/nsLayoutUtils.cpp
@@ +4483,5 @@
>  nsLayoutUtils::SurfaceFromElementResult
>  nsLayoutUtils::SurfaceFromElement(nsIImageLoadingContent* aElement,
>                                    uint32_t aSurfaceFlags)
>  {
> +  return SurfaceFromImgIContainer(ImgIContainerFromElement(aElement, aSurfaceFlags), aSurfaceFlags);

Break line somewhere

@@ +4488,5 @@
> +}
> +
> +nsLayoutUtils::ImgIContainerFromElementResult
> +nsLayoutUtils::ImgIContainerFromElement(nsIImageLoadingContent* aElement,
> +                                  uint32_t aSurfaceFlags)

fix indent

@@ +4544,5 @@
> +  }
> +
> +  result.mImgContainer = imgContainer;
> +  result.mWhichFrame=whichFrame;
> +  result.mFrameFlags=frameFlags;

spaces around =

@@ +4634,5 @@
>    return SurfaceFromElement(static_cast<nsIImageLoadingContent*>(aElement),
>                              aSurfaceFlags);
>  }
>  
> +

Remove blank line

@@ +4778,5 @@
>  }
>  
> +nsLayoutUtils::ImgIContainerFromElementResult
> +nsLayoutUtils::ImgIContainerFromElement(dom::Element* aElement,
> +                                  uint32_t aSurfaceFlags)

fix indent

::: layout/base/nsLayoutUtils.h
@@ +1486,5 @@
> +    /* Whether the element was still loading.  Some consumers need to handle
> +       this case specially. */
> +    bool mIsStillLoading;
> +
> +    /*Loading status for passing to surface request*/

Space after /* and before */

@@ +1495,5 @@
> +
> +    /*Which frame to use*/
> +    uint32_t mWhichFrame;
> +
> +    uint32_t mFrameFlags;

This field needs more documentation.

This shares a lot of structure with SurfaceFromElementResult. Maybe we should share that somehow, but I suspect it's not currently worth it. So no problem.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 753105 [details] [diff] [review]
Patch 2: Draw vector images using imgIContainer::Draw()

Review of attachment 753105 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2966,5 @@
>    }
>  
> +  uint32_t sfeFlags = nsLayoutUtils::SFE_WANT_FIRST_FRAME;
> +  nsLayoutUtils::ImgIContainerFromElementResult iRes =
> +    nsLayoutUtils::ImgIContainerFromElement(element, sfeFlags);

We shouldn't do this call in the case where srcSurf is non-null, since we don't use the result in that case. (This code is very hot in some benchmarks.)

@@ +2967,5 @@
>  
> +  uint32_t sfeFlags = nsLayoutUtils::SFE_WANT_FIRST_FRAME;
> +  nsLayoutUtils::ImgIContainerFromElementResult iRes =
> +    nsLayoutUtils::ImgIContainerFromElement(element, sfeFlags);
> +      

Remove blank line

@@ +2969,5 @@
> +  nsLayoutUtils::ImgIContainerFromElementResult iRes =
> +    nsLayoutUtils::ImgIContainerFromElement(element, sfeFlags);
> +      
> +
> +  if(!srcSurf) {

space before ( (here and elsewhere)

@@ +2974,5 @@
> +      // The canvas spec says that drawImage should draw the first frame
> +      // of animated images
> +      uint32_t sfeFlags = nsLayoutUtils::SFE_WANT_FIRST_FRAME;
> +
> +    //We only need to directly draw vector images. Getting a surface for raster images is faster because of caching.

Space after //

@@ +2975,5 @@
> +      // of animated images
> +      uint32_t sfeFlags = nsLayoutUtils::SFE_WANT_FIRST_FRAME;
> +
> +    //We only need to directly draw vector images. Getting a surface for raster images is faster because of caching.
> +    if (!iRes.mImgContainer || iRes.mImgContainer->GetType()==imgIContainer::TYPE_RASTER) {

spaces around ==. You'll probably also need to put a line break after ||

@@ +2977,5 @@
> +
> +    //We only need to directly draw vector images. Getting a surface for raster images is faster because of caching.
> +    if (!iRes.mImgContainer || iRes.mImgContainer->GetType()==imgIContainer::TYPE_RASTER) {
> +      nsLayoutUtils::SurfaceFromElementResult sRes;
> +      

Remove trailing space

@@ +3019,3 @@
>      }
> +    else {
> +      if (!iRes.mImgContainer) {

if mImgContainer is null we wouldn't reach here, would we?

@@ +3148,5 @@
> +                                    nsIntRect(
> +                                      nsIntPoint(0,0),
> +                                      gfxIntSize(imgSize.width,imgSize.height)),
> +                                    gfxIntSize(imgSize.width,imgSize.height),
> +                                    nullptr, iResult.mWhichFrame, iResult.mFrameFlags );

For extra performance we should add FLAG_CLAMP to the flags here.

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +546,5 @@
>                   double sx, double sy, double sw, double sh,
>                   double dx, double dy, double dw, double dh, 
>                   uint8_t optional_argc, mozilla::ErrorResult& error);
>  
> +  void DrawDirectlyToCanvas( nsLayoutUtils::ImgIContainerFromElementResult iResult,

No space after (

Call iResult "image" and make it a const reference

Comment 13

[James Kolb]

Attachment: Patch 1: Refactor nsLayoutUtils to expose imgIContainer (v2)

Comment 14

[James Kolb]

Attachment: Patch 2: Draw vector images using imgIContainer::Draw() (v2)

Comment 15

[James Kolb]

Attachment: Patch 1: Refactor nsLayoutUtils to expose imgIContainer (real v2)

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 755094 [details] [diff] [review]
Patch 1: Refactor nsLayoutUtils to expose imgIContainer (real v2)

Review of attachment 755094 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.h
@@ +1491,5 @@
> +    /* Loading status for passing to surface request */
> +    uint32_t mLoadingStatus;
> +
> +    /* Whether the element used CORS when loading. */
> +    bool mCORSUsed;

Move this up next to the other bools

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 755090 [details] [diff] [review]
Patch 2: Draw vector images using imgIContainer::Draw() (v2)

Review of attachment 755090 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm. Would it be better overall to just add an imgIContainer to SurfaceFromElementResult, and add another flag that says we'll accept an imgIContainer instead of a surface if there is one? Seems like that would be less code in patch 1 as well as patch 2.

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +2973,2 @@
>    if (!srcSurf) {
> +    iRes=nsLayoutUtils::ImgIContainerFromElement(element, sfeFlags);

spaces around = (more below)

@@ +2978,5 @@
> +        iRes.mImgContainer->GetType() == imgIContainer::TYPE_RASTER) {
> +      nsLayoutUtils::SurfaceFromElementResult sRes;
> +
> +      if (iRes.mImgContainer)
> +      {

{ goes on preceding line (more below)

@@ +2984,3 @@
>        }
> +      else
> +      {

} else {

@@ +2984,4 @@
>        }
> +      else
> +      {
> +        //Did it fail to finish loading an imgIContainer?

Space after // (more below)

Comment 18

[James Kolb]

Attachment: Patch 1: Refactor nsLayoutUtils to expose imgIContainer (v3)

Is this what you had in mind? It seems a lot cleaner to me at least.

Comment 19

[James Kolb]

Attachment: Patch 2: Draw vector images using imgIContainer::Draw() (v3)

Comment 20

[James Kolb]

Attachment: Patch 2: Draw vector images using imgIContainer::Draw() (real v3)

Comment 21

[James Kolb]

Attachment: Patch 3: Draw vector images using imgIContainer::Draw() (real real v3)

Sorry to spam the list, I just can't seem to notice errors until after they're submitted. I tripple checked this one and am pretty sure I got everything though.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 756357 [details] [diff] [review]
Patch 1: Refactor nsLayoutUtils to expose imgIContainer (v3)

Review of attachment 756357 [details] [diff] [review]:
-----------------------------------------------------------------

yeah!

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 756363 [details] [diff] [review]
Patch 3: Draw vector images using imgIContainer::Draw() (real real v3)

Review of attachment 756363 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +3051,5 @@
>      bounds = mgfx::Rect(dx, dy, dw, dh);
>      bounds = mTarget->GetTransform().TransformBounds(bounds);
>    }
>  
> +  if(srcSurf) {

space before (

@@ +3102,5 @@
> +
> +  nsresult rv = image.mImgContainer->Draw(context, gfxPattern::FILTER_GOOD,
> +                                    transformMatrix,
> +                                    contextMatrix.Transform(gfxRect(
> +                                      gfxPoint(dx,dy),gfxIntSize(dw,dh))),

space after ,

@@ +3110,5 @@
> +                                    gfxIntSize(imgSize.width,imgSize.height),
> +                                    nullptr, image.mWhichFrame,
> +                                    modifiedFlags );
> +
> +  NS_ENSURE_SUCCESS_VOID(rv);

Need to save and restore mTarget's current transform here.

After creating 'context', set context's CTM to contextMatrix (i.e. undoing the setting of aTarget's CTM to the identity that gfxContext's constructor does). Then the fill rect is just dx,dy,dw,dh. transformMatrix would be easier to set up.

Comment 24

[James Kolb]

Attachment: Patch 2: Draw vector images using imgIContainer::Draw() (v4)

Oops. I drew multiple scaled images to test this one.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 757008 [details] [diff] [review]
Patch 2: Draw vector images using imgIContainer::Draw() (v4)

Review of attachment 757008 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +3099,5 @@
> +
> +  nsresult rv = image.mImgContainer->
> +    Draw(context, gfxPattern::FILTER_GOOD, transformMatrix,
> +         gfxRect(gfxPoint(dx, dy), gfxIntSize(dw, dh)),
> +         nsIntRect(nsIntPoint(0, 0), gfxIntSize(imgSize.width, imgSize.height)),

I believe this should be sx/sy/sw/sh?

Comment 26

[James Kolb]

I originally thought so too, but the subimage has to contain the whole viewport or it will just internally rasterize to a temporary surface.

From gfxUtils.cpp(461-469):
>        if (doTile || !aSubimage.Contains(aImageRect)) {
>            nsRefPtr<gfxDrawable> restrictedDrawable =
>              CreateSamplingRestrictedDrawable(aDrawable, aContext,
>                                               aUserSpaceToImageSpace, aSourceRect,
>                                               aSubimage, aFormat);
>            if (restrictedDrawable) {
>                drawable.swap(restrictedDrawable);
>            }
>        }

I tested the sx/sy/sw/sh version and can confirm that, on my computer, it is slower and very badly pixelates clipped svgs.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

However I think it's correct. The problem is if the source image is, say, white with a black rectangle in it, and sx/sy/sw/sh correspond to the black rectangle, we need to ensure that only black is drawn and no white pixels are sampled. That's why we're rasterizing and then clipping the rasterized results.

Your patches are still a big improvement for the cases where sx/sy/sw/sh are the whole image, which is definitely the common case. So I think we should go with the correct thing and pass sx/sy/sw/sh here.

Comment 28

[James Kolb]

Sorry, I think I my last post might have been confusing. My current patch DOES display partial images correctly. I have run it through the reftests to make sure it works, including the canvas-drawImage-slice-1b reftest that does exactly what you're describing(except with a green square in red).

My last post was not referring to clipping in general but specifically to using sx,sy,sw,sh for the subimage argument of imgIContainer::Draw, which is unneccesary and gives incorrect results. My code still uses the matrix transformation to correctly clip the given sx,sy,sw,sh to the fill area. The current patch draws partial/clipped images correctly and doesn't raster them.

From reftest.log (This reftest is an "unexpected" pass because the unpatched firefox draws the border wrong due to rasterization.)
> REFTEST TEST-START | file://[REMOVED]/mozilla-central/layout/reftests/svg/as-image/canvas-drawImage-slice-1b.html | 6798 / 9343 (72%)
> REFTEST   IMAGE: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAyAAAAPoCAYAAAAmy5qxAAAVLUlEQVR4nO3XwRGAQAwDseu/aSiB34IHyQ3kmT0XAABA5Lx9AAAA8B8CBAAAyAgQAAAgI0AAAICMAAEAADICBAAAyAgQAAAg8xggxz41AABYJkDGBgAAywTI2AAAYJkAGRsAACwTIGMDAIBlAmRsAACwTICMDQAAlgmQsQEAwDIBMjYAAFgmQMYGAADLBMjYAABgmQAZGwAALBMgYwMAgGUCZGwAALBMgIwNAACWCZCxAQDAMgEyNgAAWCZAxgYAAMsEyNgAAGCZABkbAAAsEyBjAwCAZQJkbAAAsEyAjA0AAJYJkLEBAMAyATI2AABYJkDGBgAAywTI2AAAYJkAGRsAACwTIGMDAIBlAmRsAACwTICMDQAAlgmQsQEAwDIBMjYAAFgmQMYGAADLBMjYAABgmQAZGwAALBMgYwMAgGUCZGwAALBMgIwNAACWCZCxAQDAMgEyNgAAWCZAxgYAAMsEyNgAAGCZABkbAAAsEyBjAwCAZQJkbAAAsEyAjA0AAJYJkLEBAMAyATI2AABYJkDGBgAAywTI2AAAYJkAGRsAACwTIGMDAIBlAmRsAACwTICMDQAAlgmQsQEAwDIBMjYAAFgmQMYGAADLBMjYAABgmQAZGwAALBMgYwMAgGUCZGwAALBMgIwNAACWCZCxAQDAMgEyNgAAWCZAxgYAAMsEyNgAAGCZABkbAAAsEyBjAwCAZQJkbAAAsEyAjA0AAJYJkLEBAMAyATI2AABYJkDGBgAAywTI2AAAYJkAGRsAACwTIGMDAIBlAmRsAACwTICMDQAAlgmQsQEAwDIBMjYAAFgmQMYGAADLBMjYAABgmQAZGwAALBMgYwMAgGUCZGwAALBMgIwNAACWCZCxAQDAMgEyNgAAWCZAxgYAAMsEyNgAAGCZABkbAAAsEyBjAwCAZQJkbAAAsEyAjA0AAJYJkLEBAMAyATI2AABYJkDGBgAAywTI2AAAYJkAGRsAACzz0QIAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEBGgAAAABkBAgAAZAQIAACQESAAAEDmBtk3L0mwSbS2AAAAAElFTkSuQmCC
> REFTEST TEST-UNEXPECTED-PASS | file://[REMOVED]/mozilla-central/layout/reftests/svg/as-image/canvas-drawImage-slice-1b.html | image comparison (==)

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The purpose of the subimage argument is to ensure that pixels outside the subimage aren't sampled if, say, we use bilinear filtering to resample the image for rendering. Just clipping the output doesn't do that.

Comment 30

[James Kolb]

Since my patch (if subimage is not used) rasters at the canvas context's resolution, the filtering is usually irrelevant(since it determines how raster images are sampled) and thus it won't sample outside of the source rect.

As far as I can tell(and of course, I could be wrong), the only exception to this is if a raster image is embedded in the svg. In that case, bilinear filtering, as you stated, will sample outside the source rect. However, the standards don't forbid sampling outside the source rect, and more importantly, firefox currently does sample outside the source rect for normal raster images(as well as svgs), so this behavior is internally consistent.

Even chrome, which normally refuses to sample outside the source rect during drawimage, will sample outside the source rect for svgs with embedded pngs drawn using drawimage.


Summary Chart (tested on linux 3.9.4-1-ARCH):
>IMAGE TYPE                                      Firefox Without Patch      Firefox With Currently Submitted Patch     Firefox With Patch and Subimage        Chromium
>normal raster image(png)                        outside                    outside                                    outside                                inside
>svg(vector only)                                outside                    inside                                     inside                                 inside
>svg(with embedded png at svg resolution)         outside                    outside                                    inside                                 outside
>svg(with embedded png at different resolution)   outside                    outside                                    outside                                outside
>are magnified images ugly and pixelated         yes                        no                                         yes                                    no

Legend:
>outside = some pixels outside src rect are sampled
>inside = only pixels inside src rect are sampled


That being said, this all might be irrelevant since, while testing, I have discovered that (with or without using subimage) my patch renders the text-shadow in the wrong location for your embedded html example: http://people.mozilla.org/~roc/rendering-HTML-elements-to-canvas.html so I'll have to figure out why once I get a chance.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to James Kolb from comment #30)
> Since my patch (if subimage is not used) rasters at the canvas context's
> resolution, the filtering is usually irrelevant(since it determines how
> raster images are sampled) and thus it won't sample outside of the source
> rect.
> 
> As far as I can tell(and of course, I could be wrong), the only exception to
> this is if a raster image is embedded in the svg. In that case, bilinear
> filtering, as you stated, will sample outside the source rect. However, the
> standards don't forbid sampling outside the source rect, and more
> importantly, firefox currently does sample outside the source rect for
> normal raster images(as well as svgs), so this behavior is internally
> consistent.

You're completely correct. My apologies.

Comment 32

[James Kolb]

Attachment: Draw vector images using imgIContainer::Draw() (v5)

I'm now grabbing the transform from the adjusted surface. This fixes problems with canvas context-based shadows, but does not fix the problem with embedded css text-shadows.

I haven't had much time lately to look into it, but next week I should, hopefully, be able to sit down and write a fix for the embedded css shadows.

Comment 33

[James Kolb]

Attachment: Patch 3: Fix gfxContext::mask bug

It turns out, the CSS shadow bug is due to a bug in gfxContext::Mask where it was passing the deviceOffset twice to DrawTargetCairo based contexts.

This patch fixes all the remaining bugs I've found, passes the reftests, and works perfectly in the firefox version I checked out a couple weeks ago, but it collides with the changes in bug 880836. Bug 880836's changes are currently unsupported in DrawTargetCairo, which opens up a whole new can of worms that I'll look at later.

Comment 34

[James Kolb]

Attachment: Patch 2: Draw vector images using imgIContainer::Draw() (v6)

I'm resubmitting this for review now that everything is working.

Comment 35

[James Kolb]

Attachment: Patch 3: Fix gfxContext::mask bug (v2)

Changed patch 3 to work with the new MaskSurface version.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 775337 [details] [diff] [review]
Patch 3: Fix gfxContext::mask bug (v2)

Review of attachment 775337 [details] [diff] [review]:
-----------------------------------------------------------------

Hurrah!!!

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eeed50e8e213
https://hg.mozilla.org/integration/mozilla-inbound/rev/80520a16d7f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/f60f6c301670

Comment 38

[Ryan VanderMeulen [:RyanVM]]

How did I know that landing this without a Try push wasn't going to end well? Backed out for some reftest unexpected passes (as much as I want to make the call as to whether a reftest.list annotation is correct, I feel that's a bit out of my qualifications)...
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa59555856a1

https://tbpl.mozilla.org/php/getParsedLog.php?id=25291827&tree=Mozilla-Inbound

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Android too

Comment 40

[Daniel Holbert [:dholbert]]

Yeah, substantial changes like this should go through our Try server before we mark them as checkin-needed.

(Try server gives us a sanity-check that all unit tests pass/fail in the way we expect them to, before we actually really check it in.)

For now, though, the cycle where this (temporarily) landed on mozilla-inbound can serve as the Try push. :)  That cycle is:
 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b1b391784e53

It looks like this had unexpected reftest passes on some platforms, but those tests still failed on other platforms (matching our current "fails" annotations, hence those platforms are green).

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's fine to mark those three tests as passing on the platforms where they pass now.

Comment 42

[ndmuir]

Is there anything holding this patch up then?

Comment 43

[James Kolb]

It appears to fail the reftest on OSX and Windows XP. I've been trying to get my hands on a mac capable of compiling nightly, but haven't been able to yet. If somebody with a mac or windows xp can figure out what's up, that would be awesome.

Comment 45

[James Kolb]

Attachment: Patch 1: Refactor nsLayoutUtils to expose imgIContainer (v4)

Updated patches to work for current firefox build.

Comment 46

[James Kolb]

Attachment: Patch 2: Draw vector images using imgIContainer::Draw() (v7)

Updated patches to work with newest firefox.

Comment 47

[James Kolb]

Attachment: Patch 3: Fix gfxContext::mask bug (v3)

Updated patches to work with newest version of firefox.

Comment 48

[James Kolb]

I updated the patches to work (or at least compile) with the new firefox build.

The new firefox code has caused two new problems in the patches:
1) SVGs drawn to canvas and scaled (either with context.scale or firefox's zoom) aren't drawn at the correct resolution.
2) In lea's example, the svg drawn to canvas has a green bar underneath the circle. I can reproduce this at some zoom levels on the circle that doesn't use a canvas, so this may be a mostly unrelated svg bug.

Comment 49

[Seth Fowler [:seth] [:s2h]]

(In reply to James Kolb from comment #48)
> I updated the patches to work (or at least compile) with the new firefox
> build.
> 
> The new firefox code has caused two new problems in the patches:
> 1) SVGs drawn to canvas and scaled (either with context.scale or firefox's
> zoom) aren't drawn at the correct resolution.
> 2) In lea's example, the svg drawn to canvas has a green bar underneath the
> circle. I can reproduce this at some zoom levels on the circle that doesn't
> use a canvas, so this may be a mostly unrelated svg bug.

Just took a _very_ superficial look. (I definitely didn't read all the code, so I may well have missed something.) When you are drawing SVGs (imgIContainer::GetType returns TYPE_VECTOR), what you probably should be doing is passing in the size of the destination rect as the image size. In the long term the imgIContainer::Draw interface is going to change to eliminate this issue, but for now that's the way things are being done.

Please needinfo me if you continue to have trouble. I'd appreciate if you'd request review from me on part 2 of this patch before landing.

Comment 50

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] from comment #49)
>  as the image size.

What I mean to say is, in place of the imgSize arguments you have now.

Comment 51

[James Kolb]

Maybe I'm misunderstanding you, but I just tested swapping out imgSize with dw/dh and it made things worse.

To clarify, these patches worked fine in July(on linux); I just had no way of testing them on a mac (where they were failling). Now I have access to a mac, but the firefox code they were patched against changed, causing the test images to be slightly off and only if I change the zoom.

My guess is that I'll have to change the way the svg canvas code deals with zooming slightly to match whatever changed in the rest of the svg code since July. I don't think it'll be a problem, I just haven't had a chance to take a good look at it. :)

Comment 52

[James Kolb]

Attachment: Patch 4: Fix for svg caching

It looks like new svg caching code was the culprit. This patch applies the contextMatrix manually as part of the transform, so that it gets seen by the svg caching code. I'm wondering if it makes more sense to edit the svg caching code instead, but for now, this works.

Comment 53

[James Kolb]

Attachment: Patch 1: Refactor nsLayoutUtils to expose imgIContainer (v5)

This is pretty much the same as the previously approved version of this patch except for the extra "#include imgIContainer" in nsLayoutUtils.h (needed in OSX).

Comment 54

[James Kolb]

Attachment: Patch 2: Draw vector images using imgIContainer::Draw() (v8)

This is also pretty much the same as what was reviewed last time, but with an extra include statement. (I added new changes to this code in patch 4)

Comment 55

[James Kolb]

Attachment: Patch 3: Fix gfxContext::mask bug (v4)

This one really is the same.

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8335843 [details] [diff] [review]
Patch 2: Draw vector images using imgIContainer::Draw() (v8)

Review of attachment 8335843 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +3228,5 @@
> +
> +  nsRefPtr<gfxContext> context = new gfxContext(tempTarget);
> +  context->SetMatrix(contextMatrix);
> +  
> +  // FLAG_CAMP is added for increased performance

FLAG_CLAMP

Comment 57

[James Kolb]

Attachment: Patch 4: Fix for svg caching (v2)

This one's new. (And patches on top of patch 2)

I changed directdraw to apply the contextmatrix manually and then set it after drawing. This is because the code for caching doesn't take into account the context matrix's scale when it fudges the scale for bug 600207.

This works, though it is a little ugly. It might make more sense to change the bug 600207 code instead to also use the contextmatrix, but it looks like it might have been left out intentionally?

Comment 58

[James Kolb]

Attachment: Patch 5: Reftest change

These 3 reftest items should be passing on OSs where this bug is successfully fixed.

Comment 59

[James Kolb]

Attachment: Patch 2: Draw vector images using imgIContainer::Draw() (v9)

Fixed typo

Comment 60

[Seth Fowler [:seth] [:s2h]]

Ah, I see what's going on. This patch is the first caller that has set a non-identity matrix on the gfxContext it passes into VectorImage::Draw, and the code there doesn't handling the scaling correctly.

What you have is fine as a short term solution, but it's not the right place to fix this issue. (As you surmised.) It should really be fixed in VectorImage. Please file a followup bug and assign it to me and I'll make the appropriate changes.

Comment 61

[James Kolb]

I made a follow up, but you'll have to assign yourself. Bugzilla won't let me do it:
https://bugzilla.mozilla.org/show_bug.cgi?id=941467

Comment 62

[Seth Fowler [:seth] [:s2h]]

(In reply to James Kolb from comment #61)
> I made a follow up, but you'll have to assign yourself.

Thanks. I've taken it.

Comment 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to James Kolb from comment #61)
>  Bugzilla won't let me do it:

Fixed that.

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8335848 [details] [diff] [review]
Patch 4: Fix for svg caching (v2)

Review of attachment 8335848 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +3226,5 @@
>    // FLAG_CAMP is added for increased performance
>    uint32_t modifiedFlags = image.mDrawingFlags | imgIContainer::FLAG_CLAMP;
>  
> +  //The contextMatrix is being applied manually, rather than through context->SetMatrix here so that
> +  //the SVG is scaled correctly by it's caching code.

"its"

@@ +3231,3 @@
>    nsresult rv = image.mImgContainer->
>      Draw(context, GraphicsFilter::FILTER_GOOD, transformMatrix,
> +         contextMatrix.Transform(gfxRect(gfxPoint(dx, dy), gfxIntSize(dw, dh))),

This isn't actually correct if the CTM has a rotation, is it? We'll draw to something too big because the rectangle is fluffed out.

Comment 65

[James Kolb]

Attachment: Reftest change (v2)

Ooops, you're right. I didn't think about rotation.

I could make it only use the scaling aspect of the matrix, but it probably makes more sense to leave scaled contexts alone until they are fixed correctly in bug 941467.

Without scaling code, only 2 of the 3 failed reftests are fixed (though scale-2a is off by two pixels on OS X).

Comment 66

[James Kolb]

Can somebody do a try push?

Comment 67

[Daniel Holbert [:dholbert]]

Sure -- here's a Linux/windows reftests/mochitests/crashtests Try run:
  https://tbpl.mozilla.org/?tree=Try&rev=9b65468a1c42
If that goes well, I'll trigger a broader-spectrum run.

Also, two nits I noticed while importing the patches:
 (1) Looks like the final "Reftest change (v2)" patch has "(patch 5)" in its commit message, but there's no part 4. Does that need to be renumbered, or is there a missing patch here?

 (2) Looks like all of the patches have the bug-title as their commit message, plus "Patch N" at the end. Could you update those commit messages to instead briefly say what each patch is changing? (Something like the labels you've got for the attachments on this bug page)
  e.g. patch 1's commit message could be "Bug 603488 part 1: Refactor nsLayoutUtils to expose imgIContainer. r=roc"
(though for part 3, you'll probably want the commit message to be a little more specific than the "Fix gfxContext::mask bug" label that the attachment has here.)

Thanks!

Comment 68

[Daniel Holbert [:dholbert]]

(BTW, if you're wondering why your patches aren't displayed in that Try run, it's because I'd triggered a try push just before -- https://tbpl.mozilla.org/?tree=Try&rev=b0b42805f1ed -- on which I forgot to requrest crashtests, so I canceled that one, updated the try-chooser message, and re-pusehd. And then Try only displays the tipmost commit, because that's the only "new" thing. Just a Try quirk -- don't worry about it.)

Anyway -- the try run from comment 67 is hitting some crashes in the reftest "canvas-drawImage-simple-1a.html", on WinXP reftests and Windows 7/8 unaccelerated reftests:
  https://tbpl.mozilla.org/php/getParsedLog.php?id=31581004&tree=Try
  https://tbpl.mozilla.org/php/getParsedLog.php?id=31581059&tree=Try
  https://tbpl.mozilla.org/php/getParsedLog.php?id=31581480&tree=Try

We need to sort those out before landing. James, do you have a Windows dev environment available, by any chance?

Comment 69

[James Kolb]

In regards to your 1st question: yes, I should have renumbered it. Patch 4's job will be done by bug 941467.

I don't have a Windows dev environment, but it looks like Windows is crashing because VectorImage is trying to draw a non-Skia(cairo, I think?) surface to a Skia canvas and DrawTargetSkia calls MOZ_CRASH.

DrawTargetD2D just converts the SourceSurface to a DataSourceSurface and uses that. Is there any reason why DrawTargetSkia can't do the same?

Comment 70

[James Kolb]

Attachment: (needs Windows testing) Patch 4: Modified DrawTargetSkia to accept non-skia sourcesurfaces

This should hopefully fix the crashes on Windows. I don't have a Windows system, so I can't confirm that it actually does, but if somebody with Windows wants to test it, that would be awesome.

Comment 71

[Nick Cameron [:nrc]]

Hi, I'll test this patch queue on Windows over the (NZ) weekend and let you know how it goes. I'll do a new try push if it looks good locally.

Comment 72

[Nick Cameron [:nrc]]

Comment on attachment 8345109 [details] [diff] [review]
(needs Windows testing) Patch 4: Modified DrawTargetSkia to accept non-skia sourcesurfaces

Review of attachment 8345109 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=me with the nits fixed and if it works when I test it

::: gfx/2d/DrawTargetSkia.cpp
@@ +162,3 @@
>      return static_cast<SourceSurfaceSkia*>(aSurface)->GetBitmap();
> +  }
> +  else

don't need else after return

@@ +162,5 @@
>      return static_cast<SourceSurfaceSkia*>(aSurface)->GetBitmap();
> +  }
> +  else
> +  {
> +      RefPtr<DataSourceSurface> surf = aSurface->GetDataSurface();

nit: this block should be a 2 space indent, not 4

@@ +163,5 @@
> +  }
> +  else
> +  {
> +      RefPtr<DataSourceSurface> surf = aSurface->GetDataSurface();
> +      if(!surf)

space between if and (

Comment 73

[Matt Woodrow (:mattwoodrow)]

Part 3 should no longer be necessary, we now clear device offsets internally in DrawTargetCairo.

Comment 74

[Nick Cameron [:nrc]]

Patch 1 needs some rebasing due to bug 948221 - I couldn't figure out how to do it in nsLayoutUtils.cpp. I'll try to test on top of an earlier revision.

Comment 75

[Nick Cameron [:nrc]]

I built and ran locally. I was not exactly sure what to test. Browsing was fine and the test case in this bug's URL field rendered properly. I tried running the svg/as-image reftest suite and got a crash at svg-image-script-2.svg | 86 / 144 with the below stack. However, I get the same crash without the patches so I don't think it is something to worry about.

 	ntdll.dll!NtRaiseException() + 18 bytes	Unknown
 	[Frames below may be incorrect and/or missing, no symbols loaded for ntdll.dll]	
 	ntdll.dll!NtRaiseException() + 18 bytes	Unknown
 	ntdll.dll!KiUserExceptionDispatcher() + 42 bytes	Unknown
 	kernel32.dll!IsProcessorFeaturePresent() + 169 bytes	Unknown
 	msvcrt.dll!_vsnwprintf_l() + 87 bytes	Unknown
 	d2d1debug1.dll!701c5d8c()	Unknown
 	d2d1debug1.dll!701c5dd0()	Unknown
 	nvwgf2um.dll!608fec36()	Unknown
 	nvwgf2um.dll!6087a1e6()	Unknown
 	nvwgf2um.dll!6087a887()	Unknown
 	nvwgf2um.dll!6087a887()	Unknown
 	d3d11.dll!6edbcdba()	Unknown
 	nvwgf2um.dll!6087ab99()	Unknown
 	d3d11.dll!6edbc146()	Unknown
 	d3d11.dll!6edbc146()	Unknown
 	d3d11.dll!6ee1a83b()	Unknown
 	d3d11.dll!D3D11CreateDeviceAndSwapChain() + 6311 bytes	Unknown
 	d3d11.dll!D3D11CreateDeviceAndSwapChain() + 6311 bytes	Unknown
 	d3d11.dll!6edbc7bd()	Unknown
 	d3d11.dll!6edbc146()	Unknown
 	d3d11.dll!6ed98790()	Unknown
 	d2d1.dll!607b745c()	Unknown
 	d3d11.dll!D3D11CreateDeviceAndSwapChain() + 6282 bytes	Unknown
 	d3d11.dll!D3D11CreateDeviceAndSwapChain() + 2501 bytes	Unknown
 	d3d11.dll!6ed98be8()	Unknown
 	d3d11.dll!6edbc4f4()	Unknown
 	d3d11.dll!6edbc618()	Unknown
 	d3d11.dll!6edbc146()	Unknown
 	d3d11.dll!6ee1849e()	Unknown
 	kernel32.dll!HeapFree() + 20 bytes	Unknown
 	msvcr110d.dll!_free_base() + 39 bytes	Unknown
 	msvcr110d.dll!_unlock() + 20 bytes	Unknown
 	msvcr110d.dll!_free_dbg() + 102 bytes	Unknown
 	msvcr110d.dll!_free_dbg() + 93 bytes	Unknown
 	mozalloc.dll!moz_free() + 13 bytes	Unknown
>	xul.dll!operator delete(void * ptr) Line 225	C++
 	msvcr110d.dll!_unlock() + 20 bytes	Unknown
 	d2d1.dll!6078c786()	Unknown
 	d2d1.dll!606e4d15()	Unknown
 	d2d1.dll!606dc079()	Unknown
 	d2d1debug1.dll!701b0742()	Unknown
 	gkmedias.dll!mozilla::gfx::DrawTargetD2D::CreateGradientStops(mozilla::gfx::GradientStop * rawStops, unsigned int aNumStops, mozilla::gfx::ExtendMode aExtendMode) Line 1250	C++
 	xul.dll!mozilla::gfx::gfxGradientCache::GetOrCreateGradientStops(mozilla::gfx::DrawTarget * aDT, nsTArray<mozilla::gfx::GradientStop> & aStops, mozilla::gfx::ExtendMode aExtend) Line 195	C++
 	xul.dll!gfxPattern::CacheColorStops(mozilla::gfx::DrawTarget * aDT) Line 125	C++
 	xul.dll!nsSVGPaintServerFrame::SetupPaintServer(gfxContext * aContext, nsIFrame * aSource, nsStyleSVGPaint * aFillOrStroke, float aOpacity) Line 32	C++
 	xul.dll!nsSVGUtils::SetupCairoFillPaint(nsIFrame * aFrame, gfxContext * aContext, gfxTextContextPaint * aContextPaint) Line 1475	C++
 	xul.dll!nsSVGPathGeometryFrame::Render(nsRenderingContext * aContext, unsigned int aRenderComponents, nsIFrame * aTransformRoot) Line 645	C++
 	xul.dll!nsSVGPathGeometryFrame::PaintSVG(nsRenderingContext * aContext, const nsIntRect * aDirtyRect, nsIFrame * aTransformRoot) Line 200	C++
 	xul.dll!nsDisplaySVGPathGeometry::Paint(nsDisplayListBuilder * aBuilder, nsRenderingContext * aCtx) Line 98	C++
 	xul.dll!mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem> & aItems, const nsIntRect & aRect, gfxContext * aContext, nsRenderingContext * aRC, nsDisplayListBuilder * aBuilder, nsPresContext * aPresContext, const nsIntPoint & aOffset, float aXScale, float aYScale, int aCommonClipCount) Line 3397	C++
 	xul.dll!mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer * aLayer, gfxContext * aContext, const nsIntRegion & aRegionToDraw, mozilla::layers::DrawRegionClip aClip, const nsIntRegion & aRegionToInvalidate, void * aCallbackData) Line 3561	C++
 	xul.dll!mozilla::layers::ThebesLayerD3D10::DrawRegion(nsIntRegion & aRegion, mozilla::layers::Layer::SurfaceMode aMode) Line 432	C++
 	xul.dll!mozilla::layers::ThebesLayerD3D10::Validate(mozilla::layers::ReadbackProcessor * aReadback) Line 246	C++
 	xul.dll!mozilla::layers::ContainerLayerD3D10::Validate() Line 277	C++
 	xul.dll!mozilla::layers::ContainerLayerD3D10::Validate() Line 280	C++
 	xul.dll!mozilla::layers::LayerManagerD3D10::Render(mozilla::layers::LayerManager::EndTransactionFlags aFlags) Line 691	C++
 	xul.dll!mozilla::layers::LayerManagerD3D10::EndTransaction(void (mozilla::layers::ThebesLayer *, gfxContext *, const nsIntRegion &, mozilla::layers::DrawRegionClip, const nsIntRegion &, void *) * aCallback, void * aCallbackData, mozilla::layers::LayerManager::EndTransactionFlags aFlags) Line 393	C++
 	xul.dll!nsDisplayList::PaintForFrame(nsDisplayListBuilder * aBuilder, nsRenderingContext * aCtx, nsIFrame * aForFrame, unsigned int aFlags) Line 1210	C++
 	xul.dll!nsDisplayList::PaintRoot(nsDisplayListBuilder * aBuilder, nsRenderingContext * aCtx, unsigned int aFlags) Line 1067	C++
 	xul.dll!nsLayoutUtils::PaintFrame(nsRenderingContext * aRenderingContext, nsIFrame * aFrame, const nsRegion & aDirtyRegion, unsigned int aBackstop, unsigned int aFlags) Line 2324	C++
 	xul.dll!PresShell::RenderDocument(const nsRect & aRect, unsigned int aFlags, unsigned int aBackgroundColor, gfxContext * aThebesContext) Line 4608	C++
 	xul.dll!mozilla::dom::CanvasRenderingContext2D::DrawWindow(nsGlobalWindow & window, double x, double y, double w, double h, const nsAString_internal & bgColor, unsigned int flags, mozilla::ErrorResult & error) Line 3479	C++
 	xul.dll!mozilla::dom::CanvasRenderingContext2DBinding::drawWindow(JSContext * cx, JS::Handle<JSObject *> obj, mozilla::dom::CanvasRenderingContext2D * self, const JSJitMethodCallArgs & args) Line 3514	C++
 	xul.dll!mozilla::dom::CanvasRenderingContext2DBinding::genericMethod(JSContext * cx, unsigned int argc, JS::Value * vp) Line 4737	C++
 	mozjs.dll!js::CallJSNative(JSContext * cx, bool (JSContext *, unsigned int, JS::Value *) * native, const JS::CallArgs & args) Line 220	C++
 	mozjs.dll!js::Invoke(JSContext * cx, JS::CallArgs args, js::MaybeConstruct construct) Line 463	C++
 	mozjs.dll!Interpret(JSContext * cx, js::RunState & state) Line 2505	C++
 	mozjs.dll!js::RunScript(JSContext * cx, js::RunState & state) Line 420	C++
 	mozjs.dll!js::Invoke(JSContext * cx, JS::CallArgs args, js::MaybeConstruct construct) Line 482	C++
 	mozjs.dll!js::Invoke(JSContext * cx, const JS::Value & thisv, const JS::Value & fval, unsigned int argc, JS::Value * argv, JS::MutableHandle<JS::Value> rval) Line 513	C++
 	mozjs.dll!JS_CallFunctionValue(JSContext * cx, JSObject * objArg, JS::Value fval, unsigned int argc, JS::Value * argv, JS::Value * rval) Line 4971	C++
 	xul.dll!nsFrameMessageManager::ReceiveMessage(nsISupports * aTarget, const nsAString_internal & aMessage, bool aIsSync, const mozilla::dom::StructuredCloneData * aCloneData, CpowHolder * aCpows, nsIPrincipal * aPrincipal, nsTArray<nsString> * aJSONRetVal) Line 1020	C++
 	xul.dll!nsInProcessTabChildGlobal::DoSendBlockingMessage(JSContext * aCx, const nsAString_internal & aMessage, const mozilla::dom::StructuredCloneData & aData, JS::Handle<JSObject *> aCpows, nsIPrincipal * aPrincipal, nsTArray<nsString> * aJSONRetVal, bool aIsSync) Line 49	C++
 	xul.dll!nsFrameMessageManager::SendMessage(const nsAString_internal & aMessageName, const JS::Value & aJSON, const JS::Value & aObjects, nsIPrincipal * aPrincipal, JSContext * aCx, unsigned char aArgc, JS::Value * aRetval, bool aIsSync) Line 566	C++
 	xul.dll!nsFrameMessageManager::SendSyncMessage(const nsAString_internal & aMessageName, const JS::Value & aJSON, const JS::Value & aObjects, nsIPrincipal * aPrincipal, JSContext * aCx, unsigned char aArgc, JS::Value * aRetval) Line 511	C++
 	xul.dll!nsInProcessTabChildGlobal::SendSyncMessage(const nsAString_internal & aMessageName, const JS::Value & aObject, const JS::Value & aRemote, nsIPrincipal * aPrincipal, JSContext * aCx, unsigned char aArgc, JS::Value * aRetval) Line 54	C++
 	xul.dll!NS_InvokeByIndex(nsISupports * that, unsigned int methodIndex, unsigned int paramCount, nsXPTCVariant * params) Line 71	C++
 	xul.dll!CallMethodHelper::Invoke() Line 2567	C++
 	xul.dll!CallMethodHelper::Call() Line 1907	C++
 	xul.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx, XPCWrappedNative::CallMode mode) Line 1873	C++
 	xul.dll!XPC_WN_CallMethod(JSContext * cx, unsigned int argc, JS::Value * vp) Line 1301	C++
 	mozjs.dll!js::CallJSNative(JSContext * cx, bool (JSContext *, unsigned int, JS::Value *) * native, const JS::CallArgs & args) Line 220	C++
 	mozjs.dll!js::Invoke(JSContext * cx, JS::CallArgs args, js::MaybeConstruct construct) Line 463	C++
 	mozjs.dll!Interpret(JSContext * cx, js::RunState & state) Line 2505	C++
 	mozjs.dll!js::RunScript(JSContext * cx, js::RunState & state) Line 420	C++
 	mozjs.dll!js::Invoke(JSContext * cx, JS::CallArgs args, js::MaybeConstruct construct) Line 482	C++
 	mozjs.dll!js_fun_apply(JSContext * cx, unsigned int argc, JS::Value * vp) Line 1046	C++
 	mozjs.dll!js::CallJSNative(JSContext * cx, bool (JSContext *, unsigned int, JS::Value *) * native, const JS::CallArgs & args) Line 220	C++
 	mozjs.dll!js::Invoke(JSContext * cx, JS::CallArgs args, js::MaybeConstruct construct) Line 463	C++
 	mozjs.dll!js::Invoke(JSContext * cx, const JS::Value & thisv, const JS::Value & fval, unsigned int argc, JS::Value * argv, JS::MutableHandle<JS::Value> rval) Line 513	C++
 	mozjs.dll!js::DirectProxyHandler::call(JSContext * cx, JS::Handle<JSObject *> proxy, const JS::CallArgs & args) Line 474	C++
 	mozjs.dll!js::CrossCompartmentWrapper::call(JSContext * cx, JS::Handle<JSObject *> wrapper, const JS::CallArgs & args) Line 455	C++
 	mozjs.dll!js::Proxy::call(JSContext * cx, JS::Handle<JSObject *> proxy, const JS::CallArgs & args) Line 2630	C++
 	mozjs.dll!proxy_Call(JSContext * cx, unsigned int argc, JS::Value * vp) Line 3043	C++
 	mozjs.dll!js::CallJSNative(JSContext * cx, bool (JSContext *, unsigned int, JS::Value *) * native, const JS::CallArgs & args) Line 220	C++
 	mozjs.dll!js::Invoke(JSContext * cx, JS::CallArgs args, js::MaybeConstruct construct) Line 456	C++
 	mozjs.dll!Interpret(JSContext * cx, js::RunState & state) Line 2505	C++
 	mozjs.dll!js::RunScript(JSContext * cx, js::RunState & state) Line 420	C++
 	mozjs.dll!js::Invoke(JSContext * cx, JS::CallArgs args, js::MaybeConstruct construct) Line 482	C++
 	mozjs.dll!js::Invoke(JSContext * cx, const JS::Value & thisv, const JS::Value & fval, unsigned int argc, JS::Value * argv, JS::MutableHandle<JS::Value> rval) Line 513	C++
 	mozjs.dll!JS_CallFunctionValue(JSContext * cx, JSObject * objArg, JS::Value fval, unsigned int argc, JS::Value * argv, JS::Value * rval) Line 4971	C++
 	xul.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper, unsigned short methodIndex, const XPTMethodDescriptor * info_, nsXPTCMiniVariant * nativeParams) Line 1414	C++
 	xul.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex, const XPTMethodDescriptor * info, nsXPTCMiniVariant * params) Line 477	C++
 	xul.dll!PrepareAndDispatch(nsXPTCStubBase * self, unsigned int methodIndex, unsigned int * args, unsigned int * stackBytesToPop) Line 85	C++
 	xul.dll!SharedStub() Line 113	C++
 	xul.dll!nsTimerImpl::Fire() Line 555	C++
 	xul.dll!nsTimerEvent::Run() Line 637	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait, bool * result) Line 634	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * thread, bool mayWait) Line 263	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate) Line 95	C++
 	xul.dll!MessageLoop::RunInternal() Line 227	C++
 	xul.dll!MessageLoop::RunHandler() Line 220	C++
 	xul.dll!MessageLoop::Run() Line 194	C++
 	xul.dll!nsBaseAppShell::Run() Line 163	C++
 	xul.dll!nsAppShell::Run() Line 113	C++
 	xul.dll!nsAppStartup::Run() Line 268	C++
 	xul.dll!XREMain::XRE_mainRun() Line 3978	C++
 	xul.dll!XREMain::XRE_main(int argc, char * * argv, const nsXREAppData * aAppData) Line 4046	C++
 	xul.dll!XRE_main(int argc, char * * argv, const nsXREAppData * aAppData, unsigned int aFlags) Line 4254	C++
 	firefox.exe!do_main(int argc, char * * argv, nsIFile * xreDirectory) Line 280	C++
 	firefox.exe!NS_internal_main(int argc, char * * argv) Line 648	C++
 	firefox.exe!wmain(int argc, wchar_t * * argv) Line 105	C++
 	firefox.exe!__tmainCRTStartup() Line 533	C
 	firefox.exe!wmainCRTStartup() Line 377	C
 	kernel32.dll!BaseThreadInitThunk() + 18 bytes	Unknown
 	ntdll.dll!RtlInitializeExceptionChain() + 99 bytes	Unknown
 	ntdll.dll!RtlInitializeExceptionChain() + 54 bytes	Unknown

Comment 76

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=1fe96a7a656b

(btw, I notice from that that the bug number for the last patch is wrong)

Comment 77

[Nick Cameron [:nrc]]

There seems to be problems with some of the drawImage reftests that need fixing. Possibly also with the browser chrome mochitests, although they seem less obviously due to the patches. It might be worth fixing the reftests, rebasing, and pushing again to see if they go away before spending a lot of time trying to fix them.

Comment 78

[Nick Cameron [:nrc]]

Oh, and just needinfo me if you need to test on Windows again or want another try push.

Comment 79

[James Kolb]

Attachment: Patch 1: Refactor nsLayoutUtils to expose imgIContainer (v6)

Big rebase, but no logic really changed.

Comment 80

[James Kolb]

Attachment: Patch 2: Draw vector images using imgIContainer::Draw() (v10)

Same

Comment 81

[James Kolb]

Attachment: Patch 3: Modified DrawTargetSkia to accept non-skia sourcesurfaces (v2)

This used to be patch 4. The old patch 3 is now obsolete (see comment 73), so this is patch 3 now. r=nrc

Comment 82

[James Kolb]

Attachment: reftest.patch

Added fail for android on canvas scaling.

Comment 83

[James Kolb]

Hey Nick, thanks for testing my patches! :)

When you get a chance, can you test these new ones? Either on XP or with a try push?

Comment 84

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll do a try push for you.

Comment 85

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=bbf931015d79

Comment 86

[Robert O'Callahan (:roc) (email my personal email if necessary)]

WinXP crash:
https://tbpl.mozilla.org/php/getParsedLog.php?id=32629454&tree=Try&full=1#error1
I think this is related to part 3. I think the new code has a bug, which is that aSurface->GetDataSurface() can create a new DataSourceSurface, in which case it gets deleted at the end of GetBitmapForSurface, which returns an SkBitmap pointing to potentially freed memory.

Probably it's a SourceSurfaceCairo, whose underlying cairo surface is a cairo_win32_surface. Not only are we crashing but we're making a copy, probably unnecessarily since most cairo_win32_surfaces are DIBs that we can work with directly. Need to think a little about how to fix this.

Looks like canvas-drawImage-scale-2a.html needs some fuzz on Android.

Comment 87

[James Kolb]

Thanks, that makes sense.

I'm guessing the problem is that it isn't a DIB. It looks like GetDataSurface() passes the buffer directly for CAIRO_SURFACE_TYPE_IMAGE, but if we send it CAIRO_SURFACE_TYPE_SVG (which seems likely to me), it'll make a copy, then that copy will go out of scope and crash like you said.

I still need to wrap my head around it, but maybe the solution is to rasterize cairo svgs earlier if they're about to be drawn onto an uncompatible surface like Skia.

Comment 88

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We're not using any of the cairo SVG surface functionality here.

I'm written a new part 3 and pushed it to try; https://tbpl.mozilla.org/?tree=Try&rev=c82f1a7ffdd1. My patch just ensures we retain the temporary DataSourceSurface as long as we need the SkBitmap.

Comment 89

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That seems to have worked.

Comment 90

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3 v2

Comment 91

[Nick Cameron [:nrc]]

Comment on attachment 8356946 [details] [diff] [review]
Part 3 v2

Review of attachment 8356946 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/DrawTargetSkia.cpp
@@ +150,5 @@
>  }
>  
>  #endif
>  
> +struct TempBitmap

Could you add a comment for why we need this TempBitmap/mTmpSurface dance?

@@ +157,5 @@
> +  RefPtr<SourceSurface> mTmpSurface;
> +};
> +
> +static void
> +GetBitmapForSurface(SourceSurface* aSurface, TempBitmap* aResult)

why use an out param here instead of returning the TempBitmap?

@@ +173,5 @@
> +  aResult->mBitmap.setConfig(GfxFormatToSkiaConfig(surf->GetFormat()),
> +                                 surf->GetSize().width, surf->GetSize().height,
> +                                 surf->Stride());
> +  aResult->mBitmap.setPixels(surf->GetData());
> +  aResult->mTmpSurface = surf.forget();

don't need forget() (I think)

@@ +437,5 @@
>    uint32_t blurFlags = SkBlurMaskFilter::kHighQuality_BlurFlag |
>                         SkBlurMaskFilter::kIgnoreTransform_BlurFlag;
> +  TempBitmap bitmap;
> +  GetBitmapForSurface(aSurface, &bitmap);
> +  SkShader* shader = SkShader::CreateBitmapShader(bitmap.mBitmap, SkShader::kClamp_TileMode, SkShader::kClamp_TileMode);

line is too long

Comment 92

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: part 3 v3

Comment 93

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2fe87335a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/c92ffdf59f17
https://hg.mozilla.org/integration/mozilla-inbound/rev/a956cec79aad
https://hg.mozilla.org/integration/mozilla-inbound/rev/c305df97cbc6
!!!

Comment 94

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec2fe87335a9
https://hg.mozilla.org/mozilla-central/rev/c92ffdf59f17
https://hg.mozilla.org/mozilla-central/rev/a956cec79aad
https://hg.mozilla.org/mozilla-central/rev/c305df97cbc6

Comment 95

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Congratulations James! Sorry it was such a difficult road. Well done!

Comment 96

[James Kolb]

Thanks! I'm so glad that it's working now. It certainly gave me a better appreciation for cross-platform software. :)

Comment 97

[Jeff Muizelaar [:jrmuizel]]

This caused bug 1028288. The globalalpha state is not propagated into DrawDirectlyToCanvas.