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

createImageBitmap(videoFrame) Semantics #159

Closed
chcunningham opened this issue Mar 29, 2021 · 18 comments
Closed

createImageBitmap(videoFrame) Semantics #159

chcunningham opened this issue Mar 29, 2021 · 18 comments
Assignees
Labels
extension Interface changes that extend without breaking. need-definition An issues where something needs to be specified normatively

Comments

@chcunningham
Copy link
Collaborator

The plan is to make VideoFrame a CanvasImageSource (#158) and then (quickly) deprecate our videoFrame.createImageBitmap() in favor of window.createImageBitmap(videoFrame). This issue explores how window.createImageBitmap(videoFrame) would be spec'ed (in HTML).

Full credit to @sandersdan for the following. I'm just turning his excellent doc into a bug. I support his proposals. @domenic, are you the right html spec reviewer? Thoughts on the proposals below?


Purpose

There are a variety of things that createImageBitmap() for VideoFrame could optionally do, at a performance cost. This document tries to clarify what those are, whether they should be done, and whether there is or should be a configuration parameter.

Background

For a long time, it was assumed that transferToImageBitmap() would be the most efficient way to use VideoFrame, and a lot of focus was put on making ImageBitmap creation efficient (zero-copy). It’s now clear that other APIs (texImage2D(), drawImage(), and WEBGL_webcodecs_video_frame) will offer better performance, so our assumptions should be re-evaluated.

Properties

Lifetime

In some cases we can create an ImageBitmap that references the VideoFrame contents to avoid making a copy. This could require that the ImageBitmap is closed to avoid stalling the decoder, which is atypical for Web APIs.

  • Proposal: use zero-copy only when it cannot cause a decoder stall. Apps can rely on GC for ImageBitmaps, as they typically already do.

Crop

It is unclear whether users only need the visible region or will sometimes want the whole coded region.

  • Proposal: simplify usage and provide only the visible region. Recommend lower-level APIs for coded region access.

Scale

Scaling the frame to its natural size may prevent zero-copy. It’s also unclear whether API users would prefer raw pixels or 1:1 pixels.

  • ImageBitmapOptions has resizeWidth and resizeHeight to control this behavior.
  • ImageBitmaps are specified to be measured in CSS pixels, which implies a 1:1 PAR.
  • VR360 videos may have a PAR that causes the natural width to exceed the maximum WebGL texture size. We can recommend those users to specify a natural size in the VideoDecoderConfig that does not result in scaling.
  • Proposal: simplify usage and scale to the natural size. Recommend lower-level APIs for raw access.

Rotation

Rotating the frame to match the requested orientation may prevent zero-copy. Most API users will prefer that orientation is handled for them, unless they are intentionally trying to pass through the raw stream.

ImageBitmapOptions has imageOrientation, but it is for flipY only.

  • Proposal: simplify usage and always apply image orientation. Recommend lower-level APIs for raw access.

Color Conversion

VideoFrames can be created in a wider variety of color spaces than ImageBitmaps. Converting to an ImageBitmap color space is likely to be lossy, but ImageBitmaps do not carry their own color space information.

  • ImageBitmapOptions has colorSpaceConversion to disable this behavior.
  • Proposal: default to applying color space conversion (to sRGB). Recommend lower-level APIs for raw access.
@domenic
Copy link

domenic commented Mar 29, 2021

@domenic, are you the right html spec reviewer? Thoughts on the proposals below?

I don't feel like I have the ImageBitmap or VideoFrame domain expertise necessary to make strong judgment calls here. (@annevk might?) In such scenarios my preferred working mode is to delegate to the multi-implementer consensus, while doing my best to ensure local consistency at a high level. At a quick glance, your suggestions seem to meet that criteria. So the natural next question is, do you have multi-implementer input on these questions?

@chcunningham
Copy link
Collaborator Author

So the natural next question is, do you have multi-implementer input on these questions?

Firming that up now :). @aboba @padenot, any objection to the proposals in comment 1?

@chcunningham chcunningham added this to the 2021 Q1 milestone Apr 5, 2021
@chcunningham
Copy link
Collaborator Author

Discussed in editors call. Generally no objections, but @padenot is taking a closer look.

@chrisn
Copy link
Member

chrisn commented Apr 7, 2021

On colour conversion, I want to point to discussion happening in the Color on the Web CG, which is looking at how to enable HDR support in HTMLCanvasElement. This includes potential changes to ImageBitmap to support compositing of HDR images with other content in the canvas. See slide deck (slides 33 to 41) presented by @ccameron-chromium at a recent meeting (notes).

@sandersdan
Copy link
Contributor

sandersdan commented Apr 7, 2021

Yep, we can't expose HDR (except by readback) until <canvas> supports it. Regardless of the final form, <canvas> will support fewer color spaces than VideoFrame, and even within the ones that are supported they will be less configurable (VideoFrame will likely allow independent control of primaries, transfer function, and color matrix).

Given multiple choices for color spaces within an ImageBitmap does mean that we will need to pick the "right" one to convert to, but I expect that createImageBitmap() needs to solve that for non-WebCodecs cases anyway.

@annevk
Copy link
Member

annevk commented Apr 13, 2021

whatwg/html#6562 adds display-p3 support to ImageBitmap so you'll have to account for that. I'm not sure I understand the Lifetime proposal. Would an actual copy not result in a stall? Or would you throw if that were required?

@chcunningham
Copy link
Collaborator Author

Would an actual copy not result in a stall?

That's correct. We would copy from a buffer that is effectively "owned" by the decoder (they only own a handful) to a buffer that is owned by the ImageBitmap. This copy would be made silently (no throwing).

@chcunningham
Copy link
Collaborator Author

FYI, updates for createImageBitmap() are now proposed in whatwg/html#6589

Semantics match the what we proposed above.

@chcunningham
Copy link
Collaborator Author

chcunningham commented Apr 21, 2021

Edit: on second thought, still launch blocking. While we now have other better paths to render VideoFrame (e.g. drawImage()), we still need to define the behavior for createImageBitmap() since this takes a VideoFrame (CanvasImageSource) and changing the semantics may be observable.

@chcunningham
Copy link
Collaborator Author

The PR in html has approval from spec folks there
whatwg/html#6589

and I think we have consensus here as well. Given that, I'm removing the blocking label.

I'm leaving this open to track the landing of that PR (blocked on proper inheritance of the [[Detached]] slot for transferable objects - easy to fix as part of #210).

@chcunningham chcunningham removed this from the V1 Launch Blockers milestone May 5, 2021
@chcunningham chcunningham added the extension Interface changes that extend without breaking. label May 12, 2021
@chcunningham
Copy link
Collaborator Author

Triage note: marking 'extension' as we are extending createImageBitmap() and the consensus we've arrived at matches what Chrome's behind-flag implementation (so, not breaking for folks in the origin trial).

@padenot padenot added the need-definition An issues where something needs to be specified normatively label May 17, 2021
@padenot
Copy link
Collaborator

padenot commented May 17, 2021

This needs a definition if available in a shipping implementation.

@chcunningham
Copy link
Collaborator Author

@padenot, is that sufficiently covered in linked PR? whatwg/html#6589

@padenot
Copy link
Collaborator

padenot commented May 31, 2021

I see three things that are not addressed. [[detached]] vs [[Detached]] (= making VideoFrame transferable on our side), #184, which is a refactoring (which I can probably do if that would help, because it's independent of most other things), and the last comment, where a Promise is resolved in parallel, that's not really possible. It's also unclear to me why this is in parallel, this is (unfortunately) a sync API, I guess what should happen is that the implementation does its best.

@chcunningham
Copy link
Collaborator Author

[[detached]] vs [[Detached]] (= making VideoFrame transferable on our side), #184, which is a refactoring (which I can probably do if that would help, because it's independent of most other things)

Working on this now.

#184, which is a refactoring (which I can probably do if that would help, because it's independent of most other things),

Sure!

last comment, where a Promise is resolved in parallel, that's not really possible. It's also unclear to me why this is in parallel, this is (unfortunately) a sync API, I guess what should happen is that the implementation does its best.

I'll let them sort that out in the review. Seems easy to just update the other clauses and have everyone post a task to resolve if that's what's needed.

@annevk
Copy link
Member

annevk commented Jun 2, 2021

Queuing a task would probably give the most flexibility to implementations, but if we know it can never be done in parallel, I'm not sure there's a point to it as it would increase latency.

@chcunningham
Copy link
Collaborator Author

@domenic to opine.

My guess is that the current wording intended that we allow some parts of the API to run async. For example, when we createImageBitmap() from a <video> tag, steps like reading pixel data from the internal video frame may need to be async. The spec steps aren't too prescriptive about this, but that's probably fine since details may be impl specific and it shouldn't be observable. I think the bug in spec text is just that they forgot to post a task to resolve the promise.

@dalecurtis dalecurtis self-assigned this Jul 28, 2021
@dalecurtis
Copy link
Contributor

Closed by whatwg/html#6589

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension Interface changes that extend without breaking. need-definition An issues where something needs to be specified normatively
Projects
None yet
Development

No branches or pull requests

7 participants