Bug 209849 – CanvasRenderingContext2D.drawImage should ignore the EXIF orientation if the image-orientation is none

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Renaud Chaput]

When you draw an image with EXIF orientation tags on a Canvas, the image is always drawn according to the EXIF orientation tags, even with `imageOrientation="none"`.

Reproduction URL: https://codesandbox.io/s/safari-canvas-exif-bug-m3u2w

Expected : both images are displayed upside-down (EXIF rotation 3, image-orientation disabled).
On Safari 13.1 (and TP), the bottom image (canvas) is not upside-down, as EXIF orientation was taken into account

Other browsers (and Safari before 13.1?) do not show this behaviour even if they support image-orientation: from-image.

A test should also be added to ensure the image is drawn upside down when not adding the <img> element to the DOM (comment line 6) to get the same behaviour as other browsers.


I suspect it comes from https://trac.webkit.org/changeset/254841/webkit/ so I cc-ed Said.

[Said Abou-Hallawa]

Created attachment 395189 [details]
Patch

[Darin Adler]

Comment on attachment 395189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395189&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1467
> +    ImageOrientation imageOrientation = ImageOrientation::FromImage;

Seems like in the context of this function we don’t have to keep repeating the word image so much. I would have written:

    auto orientation = ImageOrientation::FromImage;

[Simon Fraser (smfr)]

Comment on attachment 395189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395189&action=review

> LayoutTests/ChangeLog:9
> +        * fast/images/image-orientation-none-canvas-expected.html: Added.
> +        * fast/images/image-orientation-none-canvas.html: Added.

Can these be web platform tests?

[Said Abou-Hallawa]

Created attachment 395226 [details]
Patch

[Said Abou-Hallawa]

Created attachment 395247 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 395189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395189&action=review

>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1467
>> +    ImageOrientation imageOrientation = ImageOrientation::FromImage;
> 
> Seems like in the context of this function we don’t have to keep repeating the word image so much. I would have written:
> 
>     auto orientation = ImageOrientation::FromImage;

Fixed.

>> LayoutTests/ChangeLog:9
>> +        * fast/images/image-orientation-none-canvas.html: Added.
> 
> Can these be web platform tests?

https://github.com/web-platform-tests/wpt/pull/22663/commits/06baf13e9911b89d5571548fb0c940794045380b

[Said Abou-Hallawa]

Created attachment 395290 [details]
Patch

[Said Abou-Hallawa]

(In reply to Said Abou-Hallawa from comment #7)
> Created attachment 395290 [details]
> Patch

I made a change which deserves a second review.

The HTMLImageElement.width and HTMLImageElement.height were respecting the image-orientation only if the HTMLImageElement is visible. If there is no renderer we were falling back to ImageOrientation::FromImage. I think we should fallback to the computedStyle to get the image orientation. 

I found this problem while writing the test and I did look carefully to see the real problem. Instead I wrote code like this:

    canvas.width = Math.max(image.width, image.height);
    canvas.height = Math.min(image.width, image.height);

This code was hiding the problem. As long as the CSS image-orientation: none is specified, the image should not look at its EXIF image orientation when calculating the size. So the above code was rewritten in this patch like this:

    canvas.width = image.width;
    canvas.height = image.height;

[Said Abou-Hallawa]

Created attachment 395545 [details]
Patch

[Said Abou-Hallawa]

(In reply to Said Abou-Hallawa from comment #9)
> Created attachment 395545 [details]
> Patch

I will address the issue of hidden image size in a separate bug.

[EWS]

Committed r259567: <https://trac.webkit.org/changeset/259567>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395545 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61333665>

[Emilio Cobos Álvarez (:emilio)]

Shouldn't this update style first? How does this deal with changing .style.imageOrientation, then calling drawImage?

[Said Abou-Hallawa]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)
> Shouldn't this update style first? How does this deal with changing
> .style.imageOrientation, then calling drawImage?

Are you referring to the layout test in the patch? Or are you talking about the change I did in CanvasRenderingContext2DBase::drawImage() where I get the imageOrientation from the computedStyle?

Please click "Review Patch" in the attachment above, click the line which is related to your question, add your comment and then click "Publish".

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 395545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395545&action=review

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1469
> +        orientation = computedStyle->imageOrientation();

I mean this line, yeah. computedStyle() may return an outdated style, unless you call document.updateStyleIfNeeded(). See similar computedStyle() callers in CanvasRenderingContext2D.cpp