Bug 209849

Summary: CanvasRenderingContext2D.drawImage should ignore the EXIF orientation if the image-orientation is none
Product: WebKit Reporter: Renaud Chaput <renchap>
Component: CanvasAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, dino, emilio, esprehn+autocc, ews-watchlist, gyuyoung.kim, japhet, sabouhallawa, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 13   
Hardware: Unspecified   
OS: Unspecified   
URL: https://codesandbox.io/s/safari-canvas-exif-bug-m3u2w
See Also: https://bugs.webkit.org/show_bug.cgi?id=210364
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Renaud Chaput
Reported 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.
Attachments
Patch (12.36 KB, patch)
2020-04-01 11:18 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (12.44 KB, patch)
2020-04-01 16:54 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (12.97 KB, patch)
2020-04-02 00:58 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (19.51 KB, patch)
2020-04-02 13:11 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Patch (12.74 KB, patch)
2020-04-05 22:34 PDT, Said Abou-Hallawa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1 2020-04-01 11:18:29 PDT
Created attachment 395189 [details] Patch
Darin Adler
Comment 2 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;
Simon Fraser (smfr)
Comment 3 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?
Said Abou-Hallawa
Comment 4 2020-04-01 16:54:02 PDT
Created attachment 395226 [details] Patch
Said Abou-Hallawa
Comment 5 2020-04-02 00:58:05 PDT
Created attachment 395247 [details] Patch
Said Abou-Hallawa
Comment 6 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
Said Abou-Hallawa
Comment 7 2020-04-02 13:11:00 PDT
Created attachment 395290 [details] Patch
Said Abou-Hallawa
Comment 8 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;
Said Abou-Hallawa
Comment 9 2020-04-05 22:34:08 PDT
Created attachment 395545 [details] Patch
Said Abou-Hallawa
Comment 10 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.
EWS
Comment 11 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].
Radar WebKit Bug Importer
Comment 12 2020-04-06 01:04:16 PDT
<rdar://problem/61333665>
Emilio Cobos Álvarez (:emilio)
Comment 13 2020-04-06 02:06:44 PDT
Shouldn't this update style first? How does this deal with changing .style.imageOrientation, then calling drawImage?
Said Abou-Hallawa
Comment 14 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".
Emilio Cobos Álvarez (:emilio)
Comment 15 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
Note You need to log in before you can comment on or make changes to this bug.