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

API for conversion between pixel formats #92

Closed
chcunningham opened this issue Oct 16, 2020 · 20 comments
Closed

API for conversion between pixel formats #92

chcunningham opened this issue Oct 16, 2020 · 20 comments
Assignees
Labels
extension Interface changes that extend without breaking. p1 Ready for PR

Comments

@chcunningham
Copy link
Collaborator

chcunningham commented Oct 16, 2020

Currently the spec lists one format: I420. We'll want more, including some format to indicate "opaque", which will be used for GPU backed frames to avoid having to always copy to CPU memory.

Particularly for "opaque", we want an API to convert to another format where the planes can be read (performing the copy to CPU along the way). This API could also be used to convert between various non-opaque formats. Something like...

let frame = await gpu_frame.convertTo(I420);

@chcunningham
Copy link
Collaborator Author

chcunningham commented Jan 21, 2021

The urgency of this API is reduced. Previously we expected this would be primary means to read back pixel data from GPU backed frames into the CPU space. Since then we've managed to add support (in chromium, not yet in the spec) for exposing pixel data from GPU backed video frames coming off of the users camera in the NV12 pixel format.

A conversion API is still interesting to pursue, but there are no urgent use cases that we are aware of (feel free to share some).

@padenot
Copy link
Collaborator

padenot commented May 5, 2021

(in chromium, not yet in the spec)

#208, for reference, but this would still be good to have.

@chcunningham
Copy link
Collaborator Author

triage note: marking extension, as this would be a new API.

@chcunningham chcunningham added the extension Interface changes that extend without breaking. label May 12, 2021
@alvestrand
Copy link

this means, of course, that any application that handles only a limited set of formats will be broken every time a new format is encountered. Seems a rough deal for app developers.

@sandersdan
Copy link
Contributor

any application that handles only a limited set of formats will be broken every time a new format is encountered

There is a fallback path; you can createImageBitmap(videoFrame), then read back that ImageBitmap (presumably as RGBA).

This isn't great if the frame was mappable in the first place, but for GPU-backed frames it actually has acceptable performance.

@alvestrand
Copy link

So this means that we have a fallback (effectively "read as RGBA"). Apps can choose between using a fast path if they support the current format and a slow path if they don't. For a lot of cases, this will be sufficient.

@chcunningham
Copy link
Collaborator Author

Options for potential chromium impl in https://bugs.chromium.org/p/chromium/issues/detail?id=1139119

@Kaiido
Copy link

Kaiido commented Nov 8, 2022

There is a fallback path; you can createImageBitmap(videoFrame), then read back that ImageBitmap (presumably as RGBA).

Is there anything that would enforce that conversion? Currently the HTML specs for createImageBitmap(VideoFrame) ask to

Set imageBitmap's bitmap data to a copy of image's visible pixel data.

I guess the term "visible pixel data" is open to interpretation, but there doesn't seem to be anything there asking for a conversion of the pixel format.
As I mentioned in whatwg/html#4785 (comment) it seems that current Chrome does convert most formats to RGB[A|X], but doesn't for instance convert BGRA, so authors still have to handle at least that case.
Having a path to get a standard output format would be great and might become a viable alternative to the often asked ImageBitmap.getImageData().

@dalecurtis
Copy link
Contributor

We've had a few requests for this in different forms, so going to mark as p1 to at least add support for converting to RGBA/BGRA which is already required for canvas interactions.

@dalecurtis dalecurtis added the p1 label Apr 4, 2023
@Djuffin
Copy link
Contributor

Djuffin commented May 3, 2023

API Change Proposal

  1. Add VideoPixelFormat format field to VideoFrameCopyToOptions, if it's not specified VideoFrame's format will be used.
dictionary VideoFrameCopyToOptions {
  VideoPixelFormat format;
  DOMRectInit rect;
  sequence<PlaneLayout> layout;
};
  1. VideoFrame.allocationSize(options) takes options.format into account when calculating the size of the buffer.
    At the same time it checks if pixel format conversion is supported by the browser, and if it's not, it throws a
    NotSupportedError DOMException.
  2. VideoFrame.copyTo(options) takes options.format into account and if necessary converts pixel formats during copying. If pixel format conversion is not supported, it synchronously throws a NotSupportedError DOMException.
    If VideoFrame.format == option.format conversion is not needed and this error is never thrown.

Example

const options = { format: 'RGBA' };
try {
  const bufSize = frame.allocationSize(options);
} catch (error) {
  if (error instanceof DOMException) {
    if (error.name === 'NotSupportedError') {
       console.log('RGBA conversion is not supported');
       return;
    }
  }
}
const buffer = new Uint8ClampedArray(size);
await frame.copyTo(buffer, options);
const image_data = new ImageData(buffer, frame.codedWidth, frame.codedHeight);

@dalecurtis
Copy link
Contributor

@padenot hasn't been a fan of exceptions for feature signaling in the past. So we could also have a simple canCopyTo() method if that's preferred.

@lubobill1990
Copy link

Thanks @Djuffin for proposing the API.

Can't wait to use the copyTo with specific format, it will definitely boost the performance when handling VideoFrame in different format.

Do we have any schedule on finalizing this API? And in practice, how long would it take to implement it in Chrome/Edge?

@youennf
Copy link
Contributor

youennf commented Jun 16, 2023

3. VideoFrame.copyTo(options) takes options.format into account and if necessary converts pixel formats during copying. If pixel format conversion is not supported, it synchronously throws a NotSupportedError DOMException.

A method that returns a promise cannot throw, or this would be a new pattern.
Also, conversion might happen out of process in which case it might not be easy to know all supported pixel format conversions ahead of time.
Rejecting with NotSupportedError seems ok.

Also, I am not a big fan of optional/open ended support as it might trigger compat issues.

@slightlyoff
Copy link

Agree that it should reject rather than throw. It is possible for a Promise-returning API to throw, but also terrible style w/o precedent in DOM AFAIK.

@aboba
Copy link
Collaborator

aboba commented Jun 26, 2023

Rejecting with NotSupportedError makes more sense. Not sure canCopyTo() is needed if it only returns a boolean.

@Djuffin
Copy link
Contributor

Djuffin commented Sep 14, 2023

At TPAC Media WG agreed on this way forward.

Two new members in VideoFrameCopyToOptions dictionary format and colorSpace

dictionary VideoFrameCopyToOptions {
  // only supports “RGBA” and “RGBX” and the format of the VideoFrame itself 
  VideoPixelFormat format; 
  PredefinedColorSpace colorSpace; //  "srgb", "display-p3" 
  DOMRectInit rect;
  sequence<PlaneLayout> layout;
};

With format specified, VideoFrame.copyTo() will be equivalent to rendering on a canvas with a given color space and calling getImageData() afterwards.

Example

const options = { 
  format: 'RGBA',
  colorSpace: 'display-p3'
};
const bufSize = frame.allocationSize(options);

const buffer = new Uint8ClampedArray(size);
await frame.copyTo(buffer, options);
const image_data = new ImageData(buffer, frame.codedWidth, frame.codedHeight,
                                 { colorSpace: 'display-p3' });

@chrisn
Copy link
Member

chrisn commented Oct 4, 2023

At TPAC we were aligned on overall direction, but there were some concerns about potentially confusing naming of the pixel formats and color spaces, that we should review; https://www.w3.org/2023/09/11-mediawg-minutes.html#t05

@Djuffin
Copy link
Contributor

Djuffin commented Oct 4, 2023

Nigel expressed concern about pixel format named RGB.
That it is in fact the name of a color space. Many people in the room pointed out that in the context of WebCodecs RGB is a pixel format that refers to the memory layout rather than RGB color space. And that the name RGB for the pixel format is used everywhere in the spec, and so far it didn't seem to produce any confusion.

@chrisn
Copy link
Member

chrisn commented Oct 4, 2023

I agree, but also wonder if we should also include the bpp in the pixel format names, as is commonly done in native APIs (example). This would help clarify the actual memory layout as well as further distinguish from the color space.

@Djuffin
Copy link
Contributor

Djuffin commented Oct 4, 2023

I agree that the pixel format enum values could benefit from bit depth in the name.
But at this point given that 2 UA have already shipped webcodecs I don't think we should rename existing VideoPixelFormat values. But when we add pixel formats with bpp!=8 we'll certainly need to add it to the name.

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. p1 Ready for PR
Projects
None yet
Development

No branches or pull requests