Bug 231063 – ImageBitmap should honor EXIF orientation

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

[Cameron McCormack (:heycam)]

.

[Radar WebKit Bug Importer]

<rdar://problem/83753956>

[Cameron McCormack (:heycam)]

Filed https://github.com/whatwg/html/issues/7210 to get clarity in the HTML spec about exactly how EXIF orientation and the image-orientation property should influence createImageBitmap.

[Cameron McCormack (:heycam)]

Created attachment 441558 [details]
Patch

[Cameron McCormack (:heycam)]

Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/31287

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Cameron McCormack (:heycam)]

Created attachment 441560 [details]
Patch

[Cameron McCormack (:heycam)]

Created attachment 441561 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 441561 [details]
Patch

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

> Source/WebCore/html/ImageBitmap.cpp:391
> +    auto orientation = imageForRender->orientation();
> +    if (orientation == ImageOrientation::FromImage)
> +        orientation = ImageOrientation::None;

I've had to write similar code to this elsewhere. I think we should try to push this complexity somewhere lower level. I'm not sure where, though.

> Source/WebCore/platform/graphics/ImageOrientation.h:111
> +    ImageOrientation flipY() const

Can this be named something like "ImageOrientationWithFlippedY()"? It sounds like it would be a non-const function.

[Said Abou-Hallawa]

Comment on attachment 441561 [details]
Patch

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

> Source/WebCore/html/ImageBitmap.cpp:238
> +static ImageOrientation imageOrientationForOrientation(ImageBitmapOptions::Orientation imageBitmapOrientation, ImageOrientation imageOrientation = ImageOrientation::None)

This function is confusing:
1. The name does not reveal what it converts from and what it converts to. We have ImageBitmapOptions::Orientation and ImageOrientation. So which one is "imageOrientation" and which one is "Orientation"?
2. The second name of the second parameter "imageOrientation" does not reveal what it represents. Can it be something "defaultImageOrientation"?

> Source/WebCore/html/ImageBitmap.cpp:389
> +    auto orientation = imageForRender->orientation();

imageForRender -> imageForRenderer

[Cameron McCormack (:heycam)]

(In reply to Myles C. Maxfield from comment #8)
> I've had to write similar code to this elsewhere. I think we should try to
> push this complexity somewhere lower level. I'm not sure where, though.

It might be an artifact of Image::orientation() having a default implementation that returns FromImage.  Maybe it should be returning None.  (I'll file a followup.)

[Cameron McCormack (:heycam)]

(In reply to Said Abou-Hallawa from comment #9)
> > Source/WebCore/html/ImageBitmap.cpp:238
> > +static ImageOrientation imageOrientationForOrientation(ImageBitmapOptions::Orientation imageBitmapOrientation, ImageOrientation imageOrientation = ImageOrientation::None)
> 
> This function is confusing:

I think you're right that the name is no longer appropriate.  I'll rename.  (But I guess I'll leave the other two similarly named functions, interpolationQualityForResizeQuality and alphaPremultiplicationForPremultiplyAlpha, since they're pure conversion functions.)

[Cameron McCormack (:heycam)]

Created attachment 441678 [details]
Patch for landing

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #10)
> (In reply to Myles C. Maxfield from comment #8)
> > I've had to write similar code to this elsewhere. I think we should try to
> > push this complexity somewhere lower level. I'm not sure where, though.
> 
> It might be an artifact of Image::orientation() having a default
> implementation that returns FromImage.  Maybe it should be returning None. 
> (I'll file a followup.)

Filed bug 231934.

[EWS]

Committed r284436 (243198@main): <https://commits.webkit.org/243198@main>

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

[Micah]

Anyone know when this will land in Safari?

Currently, in Safari 15, EXIF orientations 5-8 are not handled correctly. Oddly enough, 1-4 are handled just fine.

We still have to use ImageElement in Safari (instead of ImageBitmap) when rendering images to Canvas / WebGL texture to work around this bug. I'm not positive if this exact bug is tracked by this issue, but it seems to be related.

[Cameron McCormack (:heycam)]

(In reply to Micah from comment #15)
> Anyone know when this will land in Safari?

This change was included in Safari 15.2.

> Currently, in Safari 15, EXIF orientations 5-8 are not handled correctly.
> Oddly enough, 1-4 are handled just fine.
> 
> We still have to use ImageElement in Safari (instead of ImageBitmap) when
> rendering images to Canvas / WebGL texture to work around this bug. I'm not
> positive if this exact bug is tracked by this issue, but it seems to be
> related.

Either sounds like a related issue, or the fix for this bug wasn't complete. Could you file a new bug with a description of the issue you're seeing and CC me? Thank you.

[Micah]

(In reply to Cameron McCormack (:heycam) from comment #16)
> (In reply to Micah from comment #15)
> > Anyone know when this will land in Safari?
> 
> This change was included in Safari 15.2.
> 
> > Currently, in Safari 15, EXIF orientations 5-8 are not handled correctly.
> > Oddly enough, 1-4 are handled just fine.
> > 
> > We still have to use ImageElement in Safari (instead of ImageBitmap) when
> > rendering images to Canvas / WebGL texture to work around this bug. I'm not
> > positive if this exact bug is tracked by this issue, but it seems to be
> > related.
> 
> Either sounds like a related issue, or the fix for this bug wasn't complete.
> Could you file a new bug with a description of the issue you're seeing and
> CC me? Thank you.

Thanks Cameron! I created a new issue here:
https://bugs.webkit.org/show_bug.cgi?id=242137

Turns out I was confused about EXIF orientations 1-4 working. Looks like none of them are working (except 1 of course).