Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify how ImageBitmap interacts with EXIF orientation and image-orientation property #7210

Open
heycam opened this issue Oct 13, 2021 · 17 comments

Comments

@heycam
Copy link
Contributor

heycam commented Oct 13, 2021

Two things about how https://html.spec.whatwg.org/multipage/imagebitmap-and-animations.html#dom-createimagebitmap describes how an { imageOrientation } options object influences the image data it gets.

First, I find it odd that the createImageBitmap steps say "flipY" overrides the image's inherent orientation, instead of augmenting it with an additional vertical flip. Especially with other ~recent changes that treat correctly re-oriented images as the norm, and un-reoriented (e.g. using image-orientation: none) as being different from the norm. My guess (though I'm not sure) is that this imageOrientation option for createImageBitmap exists to make it easier to get image data into WebGL textures in the right orientation. If so, and if given an image with say a 90 degree EXIF orientation, it seems like it would better meet that goal by applying the vertical flip after applying the 90 degree rotation to correctly orient the image.

Second, it's not clear from the spec whether image-orientation: none on an <img> element passed to createImageBitmap should influence its result. We have some WPTs like html/canvas/element/manual/drawing-images-to-the-canvas/image-orientation/drawImage-from-element-swap-width-height.tentative.html, which tests that CanvasRenderingContext2D.drawImage() does take note of the property (and I think I may have asked @schenney-chromium to mark it as tentative because drawImage() also doesn't say anything about image-orientation), but from a very quick look I can't see a similar test for createImageBitmap.

@annevk
Copy link
Member

annevk commented Oct 13, 2021

I agree that this is weird and I guess it ends up exposing the original orientation, which we agreed to not do as part of w3c/csswg-drafts#5165, right?

@whatwg/canvas thoughts?

@schenney-chromium
Copy link

I also agree that we should be orienting before flipping for canvas, and always orienting tainted cross origin content.

I'll see what data we have for image-orientation: none. The plan was to deprecate it (I thought) but I'm guessing that's a long road.

@heycam
Copy link
Contributor Author

heycam commented Oct 18, 2021

I did some quick testing.

When {imageOrientation:"none"} is passed:

  • Firefox and Chrome both apply the EXIF orientation
  • Safari ignores the EXIF orientation

When {imageOrientation:"flipY"} is passed:

  • Firefox applies the EXIF orientation then performs the flipY
  • Chrome performs the flipY then applies the EXIF orientation
  • Safari performs the flipY and ignores the EXIF orientation

Chrome and Safari both have some bugs with EXIF orientation and a sub-region is passed in to createImageBitmap.

I plan to make WebKit follow Firefox's behavior and add a WPT for this in https://bugs.webkit.org/show_bug.cgi?id=231063.

(I didn't check image-orientation property influence.)

@Kaiido
Copy link
Member

Kaiido commented Oct 18, 2021

I find this sentence about EXIF very confusing...
Isn't this algorithm taking a bitmap data as input? If so, I would assume the EXIF info would already have been applied on the bitmap even before reaching this step.

Trying to trace back where the input comes from, for an HTMLImageElement it would have gone by CanvasImageSource which says that "the element's image must be used as the source image", followed by a no clearer bullet saying that "the original image data of the source image must be used, not the image as it is rendered" (apparently intended to avoid <img>'s width and height to interfere in the process).
None of these image, or original image data is clearly linked to any definition though, but if we go in HTMLImageElement we have in Updating the image data the step to prepare image request for presentation which does apply the transformations from EXIF.
So I would indeed expect the EXIF data to have been applied before the flipping, that this sentence about EXIF should be removed from createImageBitmap and that the same behavior be seen from drawImage() too and any consumer of CanvasImageSource.

@heycam
Copy link
Contributor Author

heycam commented Oct 18, 2021

I find this sentence about EXIF very confusing... Isn't this algorithm taking a bitmap data as input? If so, I would assume the EXIF info would already have been applied on the bitmap even before reaching this step.

Yes I think that's the right way to think about it. The HTMLImageElement can provide some bitmap data, and it's really its job to convert the JPEG or whatever to the bitmap data. And the normal way to do that is to apply the EXIF orientation data that's in the file.

[...] So I would indeed expect the EXIF data to have been applied before the flipping, that this sentence about EXIF should be removed from createImageBitmap and that the same behavior be seen from drawImage() too and any consumer of CanvasImageSource.

Agreed.

The only thing I would add is clarification on whether image-orientation: none on the <img> element has an effect on the bitmap data that is provided to drawImage() and createImageBitmap().

@Kaiido
Copy link
Member

Kaiido commented Oct 18, 2021

Yes, I didn't want to enter the image-orientation part because it is indeed even more confusing to me.
CSS specs say that "All CSS layout and rendering processes use the image after rotation, exactly as if the image were originally encoded in its rotated form", from that I would expect it also applies before we get the image bitmap, but the second bullet from HTML I quoted seems to imply that the <img> doesn't really matter (even if it was meant only to avoid width&height).
CSS doesn't hook on any HTML definition of what is actually updated, so it's complete gray area from what I can tell, and I'm not sure either how or where this should be specced.

@annevk
Copy link
Member

annevk commented Oct 18, 2021

cc @whatwg/css

@annevk
Copy link
Member

annevk commented Oct 18, 2021

@heycam will you also create a HTML PR?

The only way image-orientation can work I think is if the result of decoding also stores the EXIF information somewhere. That would allow for undoing it. Storing EXIF information should only be done for CORS-same-origin images.

So if the EXIF information is stored that would also allow this particular feature to undo it, but that doesn't seem like a good idea.

And if we want to remove image-orientation eventually I think the less features that account for it, the better. So if we can ignore it for drawImage() and createImageBitmap() (i.e., only look at the bitmap and not the EXIF and CSS side tables) that would be best.

@schenney-chromium
Copy link

I'm happy with this direction. I'll look for new WPT and file bugs in Chromium as necessary. Feel free to file any other bugs too.

@heycam
Copy link
Contributor Author

heycam commented Oct 19, 2021

Some further testing shows that image-orientation on the <img> has an effect on drawImage() in Firefox, but not in Chrome. I think we should just go ahead and not look at image-orientation in drawImage(), createImageBitmap(), etc.

I'll look at writing up a PR.

@heycam
Copy link
Contributor Author

heycam commented Oct 19, 2021

(I remember that in Chrome it used to be that image-orientation had an influence when it was put on the <canvas> but it looks like that's no longer the case.)

webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Oct 19, 2021
https://bugs.webkit.org/show_bug.cgi?id=231063
<rdar://problem/83753956>

Reviewed by Myles Maxfield and Said Abou-Hallawa.

LayoutTests/imported/w3c:

* web-platform-tests/html/canvas/element/manual/imagebitmap/createImageBitmap-exif-orientation-expected.txt: Added.
* web-platform-tests/html/canvas/element/manual/imagebitmap/createImageBitmap-exif-orientation.html: Added.
* web-platform-tests/html/canvas/element/manual/imagebitmap/resources/squares.jpg: Added.

Source/WebCore:

Test: imported/w3c/web-platform-tests/html/canvas/element/manual/imagebitmap/createImageBitmap-exif-orientation.html

This makes us treat {imageOrientation:"none"} as meaning "apply EXIF
orientation without any additional transformation", and
{imageOrientation:"flipY"} as meaning "apply EXIF orientation and then
apply an additional vertical flip". This behavior matches Firefox;
whatwg/html#7210 is open on clarifying this
behavior in the HTML spec.

* html/ImageBitmap.cpp:
(WebCore::ImageBitmap::createPromise):
(WebCore::ImageBitmap::createFromBuffer):
(WebCore::imageOrientationForOrientation): Deleted.
* html/ImageBitmapOptions.h:
(WebCore::ImageBitmapOptions::resolvedImageOrientation const):
* html/ImageBitmapOptions.idl:
* platform/graphics/ImageOrientation.h:
(WebCore::ImageOrientation::withFlippedY const):


Canonical link: https://commits.webkit.org/243198@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@284436 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@heycam
Copy link
Contributor Author

heycam commented Oct 19, 2021

Landed a WPT in web-platform-tests/wpt#31287.

bertogg pushed a commit to Igalia/webkit that referenced this issue Oct 19, 2021
https://bugs.webkit.org/show_bug.cgi?id=231063
<rdar://problem/83753956>

Reviewed by Myles Maxfield and Said Abou-Hallawa.

LayoutTests/imported/w3c:

* web-platform-tests/html/canvas/element/manual/imagebitmap/createImageBitmap-exif-orientation-expected.txt: Added.
* web-platform-tests/html/canvas/element/manual/imagebitmap/createImageBitmap-exif-orientation.html: Added.
* web-platform-tests/html/canvas/element/manual/imagebitmap/resources/squares.jpg: Added.

Source/WebCore:

Test: imported/w3c/web-platform-tests/html/canvas/element/manual/imagebitmap/createImageBitmap-exif-orientation.html

This makes us treat {imageOrientation:"none"} as meaning "apply EXIF
orientation without any additional transformation", and
{imageOrientation:"flipY"} as meaning "apply EXIF orientation and then
apply an additional vertical flip". This behavior matches Firefox;
whatwg/html#7210 is open on clarifying this
behavior in the HTML spec.

* html/ImageBitmap.cpp:
(WebCore::ImageBitmap::createPromise):
(WebCore::ImageBitmap::createFromBuffer):
(WebCore::imageOrientationForOrientation): Deleted.
* html/ImageBitmapOptions.h:
(WebCore::ImageBitmapOptions::resolvedImageOrientation const):
* html/ImageBitmapOptions.idl:
* platform/graphics/ImageOrientation.h:
(WebCore::ImageOrientation::withFlippedY const):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@284436 268f45cc-cd09-0410-ab3c-d52691b4dbfc
JonWBedard pushed a commit to WebKit/WebKit that referenced this issue Oct 25, 2021
    Make createImageBitmap() take EXIF orientation into account correctly
    https://bugs.webkit.org/show_bug.cgi?id=231063
    <rdar://problem/83753956>

    Reviewed by Myles Maxfield and Said Abou-Hallawa.

    LayoutTests/imported/w3c:

    * web-platform-tests/html/canvas/element/manual/imagebitmap/createImageBitmap-exif-orientation-expected.txt: Added.
    * web-platform-tests/html/canvas/element/manual/imagebitmap/createImageBitmap-exif-orientation.html: Added.
    * web-platform-tests/html/canvas/element/manual/imagebitmap/resources/squares.jpg: Added.

    Source/WebCore:

    Test: imported/w3c/web-platform-tests/html/canvas/element/manual/imagebitmap/createImageBitmap-exif-orientation.html

    This makes us treat {imageOrientation:"none"} as meaning "apply EXIF
    orientation without any additional transformation", and
    {imageOrientation:"flipY"} as meaning "apply EXIF orientation and then
    apply an additional vertical flip". This behavior matches Firefox;
    whatwg/html#7210 is open on clarifying this
    behavior in the HTML spec.

    * html/ImageBitmap.cpp:
    (WebCore::ImageBitmap::createPromise):
    (WebCore::ImageBitmap::createFromBuffer):
    (WebCore::imageOrientationForOrientation): Deleted.
    * html/ImageBitmapOptions.h:
    (WebCore::ImageBitmapOptions::resolvedImageOrientation const):
    * html/ImageBitmapOptions.idl:
    * platform/graphics/ImageOrientation.h:
    (WebCore::ImageOrientation::withFlippedY const):

    Canonical link: https://commits.webkit.org/243198@main
    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@284436 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Canonical link: https://commits.webkit.org/240672.270@safari-612-branch
git-svn-id: https://svn.webkit.org/repository/webkit/branches/safari-612-branch@284804 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@junov
Copy link
Member

junov commented Mar 28, 2022

The behavior verified by this test seems reasonable to me:

https://github.com/web-platform-tests/wpt/blob/master/html/canvas/element/manual/imagebitmap/createImageBitmap-exif-orientation.html

However, I find that the naming should be made more consistent with the image-orientation CSS attribute:
In CSS, "none" means don't apply EXIF orientation, and "from-image" means apply EXIF orientation.

Also, the option to flip the image seems orthogonal to whether or not we want to respect EXIF orientation.

I would like to propose this:

enum ImageOrientation {
  "from-image",
  "none",
};
(...)

dictionary ImageBitmapOptions {
  ImageOrientation imageOrientation = "from-image";
  boolean flipY = false;
  (...)
};

We could continue to support ImageOrientation="flipY" with a deprecation warning. This would change the meaning of "none", but since we don't currently have interoperable behavior, I think that would be acceptable.

Thoghts?

@kenrussell
Copy link
Member

@junov if we're convinced that a boolean flipY dictionary member will be eventually supported permanently by all browsers, then adding that and removing it from ImageBitmapOptions sounds fine to me.

@junov
Copy link
Member

junov commented Mar 28, 2022

More context on flipY:
The reason we have flipY at all here is for WebGL use cases. The Y-axis points upwards in the WebGL (and WebGPU?) coordinate system, so vertical flipping is common when loading images into WebGL textures. WebGL has the UNPACK_FLIP_Y_WEBGL pixel storage parameter that can be used for requesting a vertical flip. If I recall correctly, requesting the vertical flip at texture upload time triggers a slow path (@kenrussell : is this correct?). Therefore, requesting the flip at createImageBitmap time allows texture uploads to be faster (i.e. without undue latency).

WebGL's texImage2D() ignores EXIF orientation metadata. That is why, for convenience, the "flipY" option in createImageBitmap is spec'ed to cause createImageBitmap to ignore EXIF orientation metadata. Unfortunately we don't have interoperable behavior for this part. Admittedly, when taken out of context, it is awkward for flipY to have that side-effect.

I think having flipY and imageOrientation as separate options adds clarity and correctly supports important use cases.

When using {imageOrientation = "from-image", flipY = true}, I think the flip should be performed after applying the image orientation.

@sandstrom
Copy link

sandstrom commented May 17, 2022

If I've understood the discussion correctly, the desired behavior is that any EXIF rotation is respected and applied [image is rotated] before image processing begins (by createImageBitmap). Any output would hence be rotated according to EXIF, but the resulting image would not have any EXIF rotation metadata within it.

If this is indeed the aim, it would be good if the spec clarified that any output image should not contain any EXIF rotation metadata (i.e. no "pass-through").

I've read the thread and couldn't find anything, but I may have missed it.

@annevk
Copy link
Member

annevk commented Oct 7, 2022

As per #8118 (comment) there's agreement that EXIF should not be disregarded. Anyone up for writing a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

7 participants