Define video color space interfaces and members #306

chcunningham · 2021-07-16T08:23:28Z

Fixes #47

:)

chcunningham · 2021-07-16T08:25:03Z

For now it's just the basic IDL. Out of time tonight, but wanted to get this out for discussion tomorrow. The enums are just a start - more to come there (probably in a follow up) aligning with H.273.

@sandersdan FYI

chcunningham · 2021-07-16T14:47:14Z

@sandersdan - looking at VideoColorSpace.toJSON() - can you remind me what the precedent was for this?

For changing color space (getting dict version of the interface), @padenot mentioned the pattern in WebRTC has been to use Object.assign(), for example

sandersdan · 2021-07-16T19:07:59Z

Object.assign() is very similar to [Default] toJSON(), but it doesn't work as (may be) intended if the interface includes properties that shouldn't be turned into fields. For VideoColorSpace this could come up if we add H.273 getters, for example.

toJSON() also allows JSON.stringify() to work.

sandersdan · 2021-07-16T19:09:11Z

index.src.html

@@ -2685,6 +2695,9 @@
  // Default display dimensions match visibleRect.
  [EnforceRange] unsigned long displayWidth;
  [EnforceRange] unsigned long displayHeight;
+
+  // Defaults to Rec709 or SRGB for planar and interleaved formats respectively.


For YUV and RGB formats respectively.

I've deleted this comment now, but see how I've implemented the "Pick Color Space" algo (choose sRGB if it's an RGB format, otherwise choose rec709).

index.src.html

chcunningham

Mostly done. Just needs definitions for the enums. @sandersdan a few questions for you in here.

chcunningham · 2021-07-26T08:55:11Z

index.src.html

@@ -2685,6 +2695,9 @@
  // Default display dimensions match visibleRect.
  [EnforceRange] unsigned long displayWidth;
  [EnforceRange] unsigned long displayHeight;
+
+  // Defaults to Rec709 or SRGB for planar and interleaved formats respectively.


I've deleted this comment now, but see how I've implemented the "Pick Color Space" algo (choose sRGB if it's an RGB format, otherwise choose rec709).

index.src.html

chcunningham · 2021-07-26T08:57:13Z

index.src.html

+    1. If {{VideoEncoder/[[active color space]]}} is `null`:
+        1. If |frame|.{{VideoFrame/colorSpace}} is not `null`, assign it's value
+            to {{VideoEncoder/[[active color space]]}}.
+        2. ISSUE: what here? 709? Can we trust that frame.colorSpace is not null?


@sandersdan thoughts? I we discussed it being null in some cases, like if you were creating from an internal videoframe that had some non-compatible representation? I wasn't able to quickly find cases in our impl where this actually occurs.

That's correct, an implementation could produce a VideoFrame with no JS-exposed color space. In Chrome that can happen easily with the current implementation because the WebCodecs implementation only supports a subset of color spaces (BT. 601, BT. 709, and sRGB), but if/when it's expanded to all of H.273 it would only happen for frames that are in a non-H.273 color space (currently not possible in Chrome).

index.src.html

sandersdan · 2021-07-27T21:15:04Z

Merge conflicts resolved, minor fixes applied.

@padenot PTAL.

index.src.html

chcunningham · 2021-07-27T23:18:47Z

Review note: We could make this non-nullable and use a VideoColorSpace instance with null values instead.

I think I prefer the parent nullable, as-is, but I'm flexible.

Sorry, ran out time here. @sandersdan, here's an outline of todos:

Define the attribute getter steps (and associated slots) for the VideoColorSpace interface. Should be similar in size to https://w3c.github.io/webcodecs/#imagetrack
For "Initialize Frame With Resource and Size (with init, frame, resource, width and height)", delete step 12, instead write: Otherwise, assign null to [[color space]]. Per our convo that not every image source can be represented. Also, delete the Issue: block following this step
Fill out "Rendering Color" section under VideoColorSpace, replacing the placeholder Issue: block. The idea here is just to describe in english (not rigid numbered steps) the behavior of WebGL and createImageBitmap() wrt VideoFrame.
Add brief definitions for VideoColorSpace enum values, referencing table entries in h.273 (let h.273 do the heavy lifting here).

sandersdan · 2021-08-03T23:27:29Z

Sorry, ran out time here. @sandersdan, here's an outline of todos:

TODOs fixed, PTAL.

index.src.html

chcunningham

TODOs fixed, PTAL.

Thank you! LGTM.

I added a small note to address the comment from earlier.

@padenot @aboba let us know if this looks good.

chcunningham · 2021-08-11T16:09:17Z

EC: @aboba defers to Paul. LGTM

@chcunningham