Bug 42176 – Double transformation in CanvasRenderingContext2D::drawImage()

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: New Bugs

[Jakob Petsovits]

CanvasRenderingContext2D::drawImage() contains calls to GraphicsContext::roundToDevicePixels() to transform src and dst rectangles, passing the transformed coordinates to CanvasRenderingContext2D::willDraw() and GraphicsContext::drawImage(). This is wrong, because both willDraw() and drawImage() handle transformations by themselves; especially painting a smaller source area of the image because the final device transformation scales pixels is a really bad idea. The reason it doesn't cause most browsers to break is that most browsers don't have a user-to-device transformation other than the identity matrix.

It does break for Android, and for our RIM port too. Other drawing calls in CanvasRenderingContext2D don't perform this kind of transformation before passing coordinates to willDraw() and the respective drawing function. drawImage() should work alike.

Patch coming below.

[Jakob Petsovits]

Created attachment 61393 [details]
Don't transform srcRect/dstRect twice

[Darin Adler]

Comment on attachment 61393 [details]
Don't transform srcRect/dstRect twice

Can we create a test case? We normally require test cases for all bug fixes.

[Jakob Petsovits]

Yeah, I'm never stoked about opportunities to write test cases. Basically, you'd have a canvas element, set a context transformation with scale factor e.g. 0.25 and draw an img element in it that's 4 times the size of the canvas. On affected platforms, the image will appear to be cropped even though it should cover the whole thing.

[Jakob Petsovits]

Hm, I notice the same transform is also in drawImage(HTMLVideoElement* video, ...).

[Ojan Vafai]

Comment on attachment 61393 [details]
Don't transform srcRect/dstRect twice

Sounds like you have a testcase in mind. Please add one and upload a new patch for review.

[Ahmad Saleem]

Current code looks this:

https://searchfox.org/wubkat/rev/93c3eb8ac99a6b6e3894deaf14e5d38304898b48/Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp#1851

Don't know whether this was done or not.

CCing - few people for their input whether this is still applicable or not.

[Kimmo Kinnunen]

Yes, I think the code has been fixed already.

[Kimmo Kinnunen]

ExceptionOr<void> CanvasRenderingContext2DBase::drawImage(CanvasImageSource&& image, float dx, float dy)
{
    return WTF::switchOn(image,
        [&] (RefPtr<HTMLImageElement>& imageElement) -> ExceptionOr<void> {
            LayoutSize destRectSize = size(*imageElement, ImageSizeType::AfterDevicePixelRatio);
            LayoutSize sourceRectSize = size(*imageElement, ImageSizeType::BeforeDevicePixelRatio);
            return this->drawImage(*imageElement, FloatRect { 0, 0, sourceRectSize.width(), sourceRectSize.height() }, FloatRect { dx, dy, destRectSize.width(), destRectSize.height() });


So here the sourceRect is not adjucted with the device pixel ratio, where as at the time of writing the bug report, it was.