Closed Bug 1367251 Opened 7 years ago Closed 3 years ago

Wrong results with UNPACK_FLIP_Y_WEBGL and createImageBitmap imageOrientation

Categories

(Core :: Graphics: CanvasWebGL, defect, P2)

Product:
Core
Component:
Graphics: CanvasWebGL
Version:
55 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
93 Branch
Tracking Flags:
Tracking Status
firefox93 --- fixed

People

(Reporter: the.spite, Assigned: angus.sawyer)

References

Details

(Keywords: dev-doc-complete, Whiteboard: gfx-noted)

Attachments

(3 files, 4 obsolete files)

Bug 1367251 - ignore gl.UNPACK_FLIP_Y_WEBGL & UNPACK_PREMULTIPLY_ALPHA for textures uploading ImageBitmap data as per spec. r=jgilbert
47 bytes, text/x-phabricator-request
Details | Review
Bug 1367251 - add bindings for options to createImageBitmap and support flipY r=bzbarsky,aosmond
47 bytes, text/x-phabricator-request
angus.sawyer
: checkin+
Details | Review
Bug 1367251 - Revert changeset b1cdccfb383d - now-working-test. r=jgilbert
47 bytes, text/x-phabricator-request
Details | Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36
Firefox for Android

Steps to reproduce:

When using texImage2D/texSubImage2D providing an ImageBitmap, the combinations of UNPACK_FLIP_Y_WEBGL and imageOrientation specified on createImageBitmap are inconsistent.

Here's a test case with all combinations: https://www.clicktorelease.com/tmp/texImage2D/
Code is here: https://github.com/spite/texImage2D


Actual results:

"ImageBitmap, UNPACK_FLIP_Y_WEBGL false, imageOrientation flipY" should be upright, it's upside down.
"ImageBitmap, UNPACK_FLIP_Y_WEBGL true, imageOrientation default" should be upside down, it's upright.


Expected results:

The combination of flipY on texture and imagebitmap should yield correct results.
Note that this is in the context of trying to integrate ImageBitmap support into the popular Three.js library.

Support for the imageOrientation ImageBitmap creation option is needed in order to do this -- because it's using WebGL under the scenes.
Status: UNCONFIRMED → NEW
Component: Untriaged → Canvas: 2D
Ever confirmed: true
Product: Firefox → Core
See Also: → 1335594
Flags: needinfo?(jgilbert)
Whiteboard: gfx-noted
I can confirm this problem and it prevents us from using createImageBitmap.

We do not support createImageBitmap options yet...

Attached file IGNORE SUBMITTED IN ERROR (obsolete) —
Attached file IGNORE SUBMITTED IN ERROR (obsolete) —
Attached file IGNORE SUBMITTED IN ERROR (obsolete) —
Attachment #9061851 - Attachment description: Bug 1367251 - ignore gl.UNPACK_FLIP_Y_WEBGL for textures uploading ImageBitmap data as per spec. r=lsalzman → Bug 1367251 IGNORE SUBMITTED IN ERROR
Attachment #9061849 - Attachment description: Bug 1367251 - ignore gl.UNPACK_FLIP_Y_WEBGL for textures uploading ImageBitmap data as per spec. r=lsalzman → IGNORE SUBMITTED IN ERROR
Attachment #9061848 - Attachment description: Bug 1367251 - ignore gl.UNPACK_FLIP_Y_WEBGL for textures uploading ImageBitmap data as per spec. r=lsalzman → IGNORE SUBMITTED IN ERROR
Attachment #9061851 - Attachment description: Bug 1367251 IGNORE SUBMITTED IN ERROR → IGNORE SUBMITTED IN ERROR
Attachment #9061851 - Attachment is obsolete: true
Attachment #9061849 - Attachment is obsolete: true
Attachment #9061848 - Attachment is obsolete: true

Add optional options Dictionary. Later patches will add
implementation of imageOrientation: none|flipY.

Component: Canvas: 2D → Canvas: WebGL
Flags: needinfo?(jgilbert)

We need to be sure we aren't running too far ahead of where the spec is:
https://github.com/whatwg/html/pull/4248

Presumably this blocks 1442632 because that will need ImageBitmapOptions too?

That said, I'd like to understand whether our plan is to implement all the currently-specified options, or only some of them, or something else?

https://github.com/whatwg/html/pull/4248

Hmm. The spec already has an ImageBitmapOptions argument for createImageBitmap. That PR adds a new API for creating options objects that lets one easily detect which dictionary members on ImageBitmapOptions are supported, but doesn't affect the createImageBitmap() behavior, right?

I guess if https://github.com/whatwg/html/pull/4248#issuecomment-449221598 (and hence making ImageBitmapOptions an interface) is still on the table then we need to worry about that, but given that Chrome is shipping ImageBitmapOptions as a dictionary already that would still need to be supported not matter what, right?

Blocks: 1442632

Add optional options Dictionary. Later patches will add
implementation of imageOrientation: none|flipY.

Attachment #9063164 - Attachment is obsolete: true
Attachment #9062007 - Attachment description: Bug 1367251 - add bindings for options to createImageBitmap r=bzbarsky,aosmond → Bug 1367251 - add bindings for options to createImageBitmap and support flipY r=bzbarsky,aosmond
No longer blocks: 1533680

Andrew, do you plan to get to this review?

Flags: needinfo?(aosmond)
Attachment #9062007 - Flags: checkin+
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/41c6b7e917fd
add bindings for options to createImageBitmap and support flipY r=bzbarsky,aosmond

The test failures are tests that previously were not attempted because createImageBitmap() didn't accept the options dictionary.
The failing tests are because the permultiplyAlpha option has not yet been implemented.

What is the best way forward? Either temporarily expect the failures or add the premultiplyAlpha support?

Flags: needinfo?(angus.sawyer)

The best way forward is to add expectations for the behavior we plan to have (in this case, expect the failures).

Angus, are generally you aiming to get this fixed to pass the tests and into the tree, or should I try to find someone else?

Also, what about the second (so far unreviewed?) changeset?

Flags: needinfo?(angus.sawyer)

Boris, I am looking at skipping the tests and also working on adding the missing options.

I'll look at the other changeset tomorrow.

Flags: needinfo?(angus.sawyer)
Attachment #9062007 - Attachment description: Bug 1367251 - add bindings for options to createImageBitmap and support flipY r=bzbarsky,aosmond → Bug 1367251 - add options to createImageBitmap and support flipY and premultiplyAlpha r=bzbarsky,aosmond

Is this ready to land?

Assignee: nobody → angus.sawyer
Flags: needinfo?(angus.sawyer)

waiting for re-review.

From jgilbert?

Attachment #9061852 - Attachment description: Bug 1367251 - ignore gl.UNPACK_FLIP_Y_WEBGL for textures uploading ImageBitmap data as per spec. r=lsalzman → Bug 1367251 - ignore gl.UNPACK_FLIP_Y_WEBGL & UNPACK_PREMULTIPLY_ALPHA for textures uploading ImageBitmap data as per spec. r=jgilbert

Angus is this ready to land?

Flags: needinfo?(aosmond)
Keywords: dev-doc-needed
Assignee: angus.sawyer → jgilbert
Priority: P3 → P2

Jeff, what's missing in the patch for it to land? This is causing pain for people trying to use the API with the new options, see https://github.com/mrdoob/three.js/commit/70aed3c8092392b73083e1d403ab9a90e4caa975 etc.

Flags: needinfo?(jgilbert)

Would like to add that we're also having to blanket ban ImageBitmap when running on Firefox in PlayCanvas. Fix would be much appreciated!

Attachment #9062007 - Attachment description: Bug 1367251 - add options to createImageBitmap and support flipY and premultiplyAlpha r=bzbarsky,aosmond → Bug 1367251 - add bindings for options to createImageBitmap and support flipY r=bzbarsky,aosmond

Reinstate Khronos WebGL tests for createImageBitmap options parameter

Assignee: jgilbert → angus.sawyer
Flags: needinfo?(jgilbert)
Attachment #9161587 - Attachment description: Bug 1367251 - Backout changeset b1cdccfb383d - createImageBitmap options available. r=jgilbert → Bug 1367251 - Revert changeset b1cdccfb383d - now-working-test. r=jgilbert

This failed to land for incorrect reviewer(s).
https://lando.services.mozilla.com/D82359/

Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a55cda2431fc
ignore gl.UNPACK_FLIP_Y_WEBGL & UNPACK_PREMULTIPLY_ALPHA for textures uploading ImageBitmap data as per spec. r=jgilbert
https://hg.mozilla.org/integration/autoland/rev/a76c2c98b33c
add bindings for options to createImageBitmap and support flipY r=bzbarsky,aosmond,baku
https://hg.mozilla.org/integration/autoland/rev/871379f9b194
Revert changeset b1cdccfb383d - now-working-test. r=jgilbert
Priority: P2 → P1
Severity: normal → S3
Priority: P1 → P2
See Also: → 1677183

Hi Jeff or Angus: Wondering if we might see some activity on this again soon? Still P2? FWIW, we also need the options parameter for some work we are doing at Adobe and it's blocking our team from implementing more optimal solution on Firefox.

Flags: needinfo?(jgilbert)

Hi,

I have just pushed a rebase diff for the first patch for this. It needs reviewing by Jeff.
I'll push the rebase second part that adds the options parameter itself shortly.

Flags: needinfo?(angus.sawyer)

Now waiting for review of both patches. Passes tests cleanly in WPT and Khronos WebGL tests. Remaining options (colour space and resizing still need completing)

Flags: needinfo?(jgilbert)
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b52784ddd18
ignore gl.UNPACK_FLIP_Y_WEBGL & UNPACK_PREMULTIPLY_ALPHA for textures uploading ImageBitmap data as per spec. r=jgilbert
https://hg.mozilla.org/integration/autoland/rev/426b58b5faa1
add bindings for options to createImageBitmap and support flipY r=bzbarsky,aosmond,baku
https://hg.mozilla.org/integration/autoland/rev/c153c24e5559
Revert changeset b1cdccfb383d - now-working-test. r=jgilbert

https://hg.mozilla.org/mozilla-central/rev/7b52784ddd18
https://hg.mozilla.org/mozilla-central/rev/426b58b5faa1
https://hg.mozilla.org/mozilla-central/rev/c153c24e5559

Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox93: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 93 Branch
Regressions: 1732681
Keywords: dev-doc-needed → dev-doc-complete
Regressions: 1740031
You need to log in before you can comment on or make changes to this bug.