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

Change createImageBitMaps's imageOrientation attributes #8687

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

yiyix
Copy link
Contributor

@yiyix yiyix commented Jan 6, 2023

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an initial review focused on wording stuff. Hope it's helpful!

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Contributor Author

@yiyix yiyix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the initial review!!

@aardgoose
Copy link

I assume the 'from-image' and 'none' imageOrientation options are intended to apply to ImageBitmaps created from HTMLVideo elements. If so are wpt tests for this needed?

This would clarify the following FF bug https://bugzilla.mozilla.org/show_bug.cgi?id=1593790

aarongable pushed a commit to chromium/chromium that referenced this pull request Feb 6, 2023
Per new spec proposed here, whatwg/html#8687, users can now use the option {imageOrientation: 'none'} in CreateImageBitmap to specify display the image with exif orientation. Note that all the implementation is under flag "CreateImageBitmapOrientationNone", which can also be enabled with flag "--enable-experimental-web-platform-features"

Bug: 1342260

Change-Id: I8d00c3592eec756b8465b1b32177db2147fd295d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4185242
Commit-Queue: Yi Xu <yiyix@google.com>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Justin Novosad <junov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1101774}
@yiyix
Copy link
Contributor Author

yiyix commented Feb 27, 2023

I assume the 'from-image' and 'none' imageOrientation options are intended to apply to ImageBitmaps created from HTMLVideo elements. If so are wpt tests for this needed?

This would clarify the following FF bug https://bugzilla.mozilla.org/show_bug.cgi?id=1593790

This mainly applies to images with specified orientation in EXIF (Exchangeable image file format) metadata. One setting ignore the orientation and one setting uses the orientation. I have added tests here: web-platform-tests/wpt#38654

@yiyix yiyix changed the title [WIP] Change createImageBitMaps's imageOrientation attributes Change createImageBitMaps's imageOrientation attributes Feb 27, 2023
@aardgoose
Copy link

The reason I mention it is that chrome follows image rotation settings* in imageBitmaps created from Video elements while Firefox doesn't. They also diverge when creating WebGL textures directly from Video elements.

Not EXIF but have the same effect.

@yiyix
Copy link
Contributor Author

yiyix commented Mar 6, 2023

The reason I mention it is that chrome follows image rotation settings* in imageBitmaps created from Video elements while Firefox doesn't. They also diverge when creating WebGL textures directly from Video elements.

Not EXIF but have the same effect.

This change of spec is supposed to clear that ambiguity. Chrome uses to use the option "none" to say no additional change follow the orientation in the metadata; where Firefox uses "none" as disregard the rotation settings.

I first introduced the new option "from-image" to describe what Chrome follows, which also matches the CSS spec.

In this change, I am hoping to repurpose the meaning of 'none'. I also submitted a bug for firefox to ask them to use the rotational setting for 'from-image' option.

@smaug----
Copy link

do we know how much this feature has been used, in other words, do we think the change is web compatible?

@zcorpan
Copy link
Member

zcorpan commented Mar 15, 2023

@smaug---- see #8085 (comment)

@aardgoose
Copy link

@yiyix
I understand the reason for the change. As you have confirmed that video should be included, wpt tests for the imageBitmap sourced from video are needed.

Incidentally I have submitted a patch to fix one of the FF imageBitmap from HTMLImage rotation bugs and have another patch ready to fix the HTMLVideo case for imageBitmaps.

@zcorpan zcorpan added the needs tests Moving the issue forward requires someone to write tests label May 29, 2023
@zcorpan
Copy link
Member

zcorpan commented Dec 11, 2023

As of Safari 17.2 the change from none to from-image seems to be implemented in Chromium/Gecko/WebKit. I think we should land this (but it needs a rebase).

I added a "needs tests" label in May, but I don't recall why. Tests are referenced above. Are more tests needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Moving the issue forward requires someone to write tests
Development

Successfully merging this pull request may close these issues.

Add createImageBitMaps's imageOrientation "none"
5 participants