Bug 209849 - CanvasRenderingContext2D.drawImage should ignore the EXIF orientation if the image-orientation is none
Summary: CanvasRenderingContext2D.drawImage should ignore the EXIF orientation if the ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: Safari 13
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL: https://codesandbox.io/s/safari-canva...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-01 02:41 PDT by Renaud Chaput
Modified: 2020-04-10 17:57 PDT (History)
11 users (show)

See Also:


Attachments
Patch (12.36 KB, patch)
2020-04-01 11:18 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (12.44 KB, patch)
2020-04-01 16:54 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (12.97 KB, patch)
2020-04-02 00:58 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (19.51 KB, patch)
2020-04-02 13:11 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (12.74 KB, patch)
2020-04-05 22:34 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Renaud Chaput 2020-04-01 02:41:39 PDT
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.
Comment 1 Said Abou-Hallawa 2020-04-01 11:18:29 PDT
Created attachment 395189 [details]
Patch
Comment 2 Darin Adler 2020-04-01 13:35:03 PDT
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;
Comment 3 Simon Fraser (smfr) 2020-04-01 16:41:55 PDT
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?
Comment 4 Said Abou-Hallawa 2020-04-01 16:54:02 PDT
Created attachment 395226 [details]
Patch
Comment 5 Said Abou-Hallawa 2020-04-02 00:58:05 PDT
Created attachment 395247 [details]
Patch
Comment 6 Said Abou-Hallawa 2020-04-02 11:53:40 PDT
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
Comment 7 Said Abou-Hallawa 2020-04-02 13:11:00 PDT
Created attachment 395290 [details]
Patch
Comment 8 Said Abou-Hallawa 2020-04-02 13:20:55 PDT
(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;
Comment 9 Said Abou-Hallawa 2020-04-05 22:34:08 PDT
Created attachment 395545 [details]
Patch
Comment 10 Said Abou-Hallawa 2020-04-06 00:58:54 PDT
(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.
Comment 11 EWS 2020-04-06 01:03:22 PDT
Committed r259567: <https://trac.webkit.org/changeset/259567>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395545 [details].
Comment 12 Radar WebKit Bug Importer 2020-04-06 01:04:16 PDT
<rdar://problem/61333665>
Comment 13 Emilio Cobos Álvarez (:emilio) 2020-04-06 02:06:44 PDT
Shouldn't this update style first? How does this deal with changing .style.imageOrientation, then calling drawImage?
Comment 14 Said Abou-Hallawa 2020-04-06 10:37:50 PDT
(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".
Comment 15 Emilio Cobos Álvarez (:emilio) 2020-04-06 12:25:40 PDT
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