See bug 589558 comment 6 & bug 589558 comment 7

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

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

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

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

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

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.

Careful what you wish for :P

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.

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

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

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 on attachment 756357 [details] [diff] [review] Patch 1: Refactor nsLayoutUtils to expose imgIContainer (v3) Review of attachment 756357 [details] [diff] [review]: ----------------------------------------------------------------- yeah!

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.

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

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?

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.

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.

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

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.

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.

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

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.

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.

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

Changed patch 3 to work with the new MaskSurface version.

Comment on attachment 775337 [details] [diff] [review] Patch 3: Fix gfxContext::mask bug (v2) Review of attachment 775337 [details] [diff] [review]: ----------------------------------------------------------------- Hurrah!!!

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

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

Android too

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

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

Is there anything holding this patch up then?

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.

Updated patches to work for current firefox build.

Updated patches to work with newest firefox.

Updated patches to work with newest version of firefox.

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.

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

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

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

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.

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

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)

This one really is the same.

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

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?

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

Fixed typo

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.

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

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

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

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.

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

Can somebody do a try push?

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!

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

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?

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.

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

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

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.

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

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)

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.

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

Big rebase, but no logic really changed.

Same

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

Added fail for android on canvas scaling.

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?

I'll do a try push for you.

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.

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.

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.

That seems to have worked.

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

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 !!!

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

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

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

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