Simplify VideoFrame Readback #208

chcunningham · 2021-04-30T08:22:54Z

Define new concepts of Regions and Layout as described in #157.

Regions make it easier to articulate what part of the frame is desired for reading out (e.g. only the "visible" (non padded) region).
Layouts make it easier to describe the position and padding of a plane within a buffer. Useful during Plane construction as well as when reading out Planes to a BufferSource.

chcunningham · 2021-04-30T08:31:04Z

@padenot @aboba This makes the interface changes but does not yet include spec steps. Uploaded as-is for discussion during (or prior) to tomorrow's meeting.

chcunningham · 2021-04-30T16:27:54Z

Editors call:
@padenot generally looks good, going to loop in graphics folks shortly
@bernard, no objection

padenot · 2021-05-03T14:08:08Z

@padenot generally looks good, going to loop in graphics folks shortly

(I've contacted them).

chcunningham

I added some additional comments for design discussion.

index.src.html

chcunningham

I added some additional comments for design discussion.

On second thought, lets move the design discussion to #157 for better visibility/readability. Please see this recent comment.

@padenot generally looks good, going to loop in graphics folks shortly

(I've contacted them).

Thanks. Standing by. Please give this issue priority. Of all our v1 launch blockers, this one has the most open questions.

index.src.html

padenot · 2021-05-10T11:59:15Z

Thanks. Standing by. Please give this issue priority. Of all our v1 launch blockers, this one has the most open questions.

I'm relaying the comments I've received, paraphrasing to add a bit more context:

The changes look like a good idea overall. A few thoughts:

Most graphics APIs use Region to mean a complex shape (like a set of rectangles, or polygons). Can we just use the more-common Rect for top/left/width/height? Could we just use the existing DOMRect instead of defining a new dictionary type?

PlaneInit now takes a BufferSource and an offset, what happens if someone constructs a VideoFrame with a set of planes that each refer to different buffers? The idea of having a per-plane offset suggests that we expect all planes to be within a single buffer, so I think PlaneInit should be a single BufferSource plus a Sequence of PlaneLayouts.

Alternatively we should truly allow for separate buffers per-plane, which in theory should be possible (and describable by our internal C++ descriptors in Gecko's implementation), but doesn't seem to happen in practice (from the existing decoders in Gecko).

As is being discussed, adding an option to do an async readback seems like a really good idea.

chcunningham · 2021-05-10T23:11:28Z

Thanks @padenot

Most graphics APIs use Region to mean a complex shape (like a set of rectangles, or polygons). Can we just use the more-common Rect for top/left/width/height? Could we just use the existing DOMRect instead of defining a new dictionary type?

The main thing we disliked about DOMRect was that it's values are floating point. There are media APIs that handle the crop as floating point values, but this only makes sense for rendering, not for copying raw bytes.

We definitely aren't attached to the name "Region". VideoFrameRect or similar would be fine. We also slightly prefer the top/left naming to DOMRect's x/y, but that's not too important.

PlaneInit now takes a BufferSource and an offset, what happens if someone constructs a VideoFrame with a set of planes that each refer to different buffers? The idea of having a per-plane offset suggests that we expect all planes to be within a single buffer, so I think PlaneInit should be a single BufferSource plus a Sequence of PlaneLayouts.

Alternatively we should truly allow for separate buffers per-plane, which in theory should be possible (and describable by our internal C++ descriptors in Gecko's implementation), but doesn't seem to happen in practice (from the existing decoders in Gecko).

The current shape is flexible: each plane can use the same buffer or a different buffer. The offset refers to whatever they give (should default to zero).

But, we agree that using the same buffer will be more common. We're open to taking that path if you prefer. If so, we propose the following single-buffer constructor
VideoFrame(BufferSource buffer, VideoFramePlaneInit init)

And we would add a new sequence<PlaneLayout> layout; member to VideoFramePlaneInit to describe the offset/stride of each plane in the single buffer.

As is being discussed, adding an option to do an async readback seems like a really good idea.

Awesome.

- s/crop/visbile/ - s/region/rect/ - s/readInto/copyTo/

chcunningham · 2021-06-03T09:28:22Z

@padenot I still need to define some spec steps to tie everything together, but this should now at least be at a point where the definitions are all there and crop stuff is fully removed. I'll finish it up Thursday afternoon.

... and VideoFrame codedRect and visibleRect attributes. Still TODO: define steps for copyTo and allocationSize. Probably a *big* TODO.

chcunningham · 2021-06-04T08:02:55Z

@padenot

The latest commit does everything except impl steps for vf.allocationSize() and vf.copyTo(). Listing those steps is so far looking like a large amount of work. Still pondering the approach.

Re: #165, I think we at least want each VF pixel format to specify

num planes
sample size of plane[i]
bytes per sample of plane[i]

Maybe more than that, but those were the dfns I found myself wanting so far. Chat tomorrow.

chcunningham · 2021-06-08T07:26:06Z

@padenot @aboba this is now completely specified and ready for review.

@sandersdan you might double check the new VF algos.

@padenot, above I wrote:

Re: #165, I think we at least want each VF pixel format to specify

num planes

sample size of plane[i]

bytes per sample of plane[i]

Having now written the algorithms, I would add to that list

"plane"
stride

padenot

Looks good, a few questions though.

Makefile

padenot · 2021-06-11T10:09:28Z

index.src.html

+            3. Increment |row| by `1`.
+
+    4. Let |resourceCodedWidth| be the coded width of |resource|.
+    5. Let |resourceCodedHeight| be the coded height of |resource|.


Link those two.

padenot · 2021-06-11T10:09:45Z

index.src.html

        |resource|.
-    8. Let |resourceCropTop| be the top offset of the crop origin of
+    7. Let |resourceVisibleTop| be the top offset for the visible rectangle of


Link as well.

padenot · 2021-06-11T10:10:59Z

index.src.html

+        2. Assign {{VideoFrame/[[coded width]]}} and
+            {{VideoFrame/[[coded height]]}} to {{VideoFrameRect/width}} and
+            {{VideoFrameRect/height}} respectively.
+    2. Return |rect|.


A note that (0,0) is top left and that this matches canvas?

I think this one is pretty clear since we called our attributes "top" and "left", but I'm happy to add a note if you still like.

index.src.html

padenot · 2021-06-11T11:02:01Z

index.src.html

-    3. Otherwise (|resource| does not use a recognized {{PixelFormat}}):
-        1. Assign `""` to {{VideoFrame/format}}.
-        2. Assign `null` to {{VideoFrame/planes}}.
+    2. If |resource| uses a recognized {{PixelFormat}}, assign the


We'll link this after my PR.

padenot · 2021-06-11T11:04:06Z

index.src.html

+
+        1. Let |plane| be the Plane identified by |planeIndex| as defined by
+            {{VideoFrame/[[format]]}}.
+        2. Let |sampleWidth| be the horizontal pixel size of each subsample


We'll link this as well.

index.src.html

chcunningham · 2021-06-11T13:22:51Z

Editors call: good to merge % comments

chcunningham · 2021-06-11T13:31:11Z

VideoFrame(BufferSource buffer, sequence layout, VFPlaneInit init)

but this drops ability to add alpha without a copy. options to extend (if needed):

VideoFrame((BufferSource or sequence) buffer, sequence layout, VFPlaneInit init)

or We could add it to VFPlaneInit

chcunningham

Thanks for reviewing! I left the link comments unresolved so it would be easy to find them later. A few minor comments also still open w/ reply questions, but I'll send a quick follow up patch for those if needed.

Makefile

index.src.html

chcunningham · 2021-06-11T22:13:33Z

index.src.html

-    3. Otherwise (|resource| does not use a recognized {{PixelFormat}}):
-        1. Assign `""` to {{VideoFrame/format}}.
-        2. Assign `null` to {{VideoFrame/planes}}.
+    2. If |resource| uses a recognized {{PixelFormat}}, assign the


chcunningham · 2021-06-11T22:13:53Z

index.src.html

+
+        1. Let |plane| be the Plane identified by |planeIndex| as defined by
+            {{VideoFrame/[[format]]}}.
+        2. Let |sampleWidth| be the horizontal pixel size of each subsample


chcunningham · 2021-06-11T22:18:41Z

index.src.html

-      6. If |displayAspectWidth| and |displayAspectHeight| are provided,
+  1. Let |frame| be a new {{VideoFrame}}, constructed as follows:
+      1. Assign `false` to {{VideoFrame/[[detached]]}}.
+      2. Let |resource| be the [=media resource=] described by |output|.


I'd say it as "resource ownership is shared by all the frames that reference the resource. it may additionally be shared by a codec that is using/emitting the resource"

But I worry I maybe misunderstand your intent? We document the sharing in the Memory Model section. We also have this note in decode():

NOTE: Authors should call close() on output VideoFrames immediately when frames are no longer needed. The underlying media resources are owned by the VideoDecoder and failing to release them (or waiting for garbage collection) may cause decoding to stall.

LMK if more is needed, will send a quick follow up.

index.src.html

chcunningham · 2021-06-11T22:29:35Z

index.src.html

+        2. Assign {{VideoFrame/[[coded width]]}} and
+            {{VideoFrame/[[coded height]]}} to {{VideoFrameRect/width}} and
+            {{VideoFrameRect/height}} respectively.
+    2. Return |rect|.


I think this one is pretty clear since we called our attributes "top" and "left", but I'm happy to add a note if you still like.

@chcunningham

SHA: a96a6d7 Reason: push, by @chcunningham Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@chcunningham

SHA: a96a6d7 Reason: push, by @chcunningham Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@chcunningham

SHA: a96a6d7 Reason: push, by @chcunningham Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@tguilbert-google

SHA: a96a6d7 Reason: push, by @tguilbert-google Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@tguilbert-google

SHA: a96a6d7 Reason: push, by @tguilbert-google Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

@tguilbert-google

SHA: a96a6d7 Reason: push, by @tguilbert-google Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Define IDL for simpler regions-based VideoFrame API

2462a91

chcunningham changed the base branch from main to videoframe_clone_close April 30, 2021 08:23

Remove crop* dfn's in favor of visibleRegion

f8a720b

chcunningham requested review from padenot and aboba April 30, 2021 08:31

chcunningham commented May 4, 2021

View reviewed changes

index.src.html Outdated Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

chcunningham mentioned this pull request May 4, 2021

Make Encoded*Chunk interfaces immutable. #174

Merged

chcunningham commented May 4, 2021

View reviewed changes

index.src.html Show resolved Hide resolved

chcunningham mentioned this pull request May 4, 2021

Simplifying and optimizing VideoFrame readback #157

Closed

chcunningham commented May 4, 2021

View reviewed changes

index.src.html Outdated Show resolved Hide resolved

index.src.html Outdated Show resolved Hide resolved

padenot mentioned this pull request May 5, 2021

API for conversion between pixel formats #92

Closed

sandersdan mentioned this pull request May 19, 2021

Remove crop* from VideoDecoderConfig #250

Merged

chcunningham changed the base branch from videoframe_clone_close to main June 2, 2021 03:49

chcunningham added 5 commits June 1, 2021 21:08

Merge remote-tracking branch 'origin/main' into videoframe_regions

0ff1d88

Merge fixes

1d27d0a

Various renames

e8ce1f4

- s/crop/visbile/ - s/region/rect/ - s/readInto/copyTo/

Remove VideoFrame/planes

0dc00be

Merge remote-tracking branch 'origin/main' into videoframe_regions

893b046

Added dfns for VideoFrameRect, PlaneLayout, VFCopyToOptions, ...

6c2dd54

... and VideoFrame codedRect and visibleRect attributes. Still TODO: define steps for copyTo and allocationSize. Probably a *big* TODO.

Fill in VideoFrame copyTo() and allocationSize()

371529f

padenot mentioned this pull request Jun 8, 2021

Use slots instead of directly referencing the attributes #271

Merged

Fix PlaneInit, remove rows, add offset

72f69cd

Merge remote-tracking branch 'origin/main' into videoframe_regions

955111c

padenot reviewed Jun 11, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into videoframe_regions

f84adf3

chcunningham commented Jun 11, 2021

View reviewed changes

Address feedback

9591101

chcunningham merged commit a96a6d7 into main Jun 11, 2021

chcunningham deleted the videoframe_regions branch June 11, 2021 22:34

github-actions bot added a commit that referenced this pull request Jun 11, 2021

Merge pull request #208 from w3c/videoframe_regions

b5e01bd

SHA: a96a6d7 Reason: push, by @chcunningham Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Jun 11, 2021

Merge pull request #208 from w3c/videoframe_regions

dab7a17

SHA: a96a6d7 Reason: push, by @chcunningham Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

github-actions bot added a commit that referenced this pull request Jun 11, 2021

Merge pull request #208 from w3c/videoframe_regions

8a09f03

SHA: a96a6d7 Reason: push, by @chcunningham Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

tidoust mentioned this pull request Jun 12, 2021

Invalid Web IDL for VideoFrame: attributes cannot accept dictionary types #278

Closed

This was referenced Jul 12, 2021

Unit of the stride attribute #224

Closed

Update VideoFrame ctor to take single buffer #303

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify VideoFrame Readback #208

Simplify VideoFrame Readback #208

chcunningham commented Apr 30, 2021 •

edited by pr-preview bot

Loading

chcunningham commented Apr 30, 2021

chcunningham commented Apr 30, 2021

padenot commented May 3, 2021

chcunningham left a comment

chcunningham left a comment

padenot commented May 10, 2021

chcunningham commented May 10, 2021

chcunningham commented Jun 3, 2021

chcunningham commented Jun 4, 2021

chcunningham commented Jun 8, 2021

padenot left a comment

padenot Jun 11, 2021

padenot Jun 11, 2021

padenot Jun 11, 2021

chcunningham Jun 11, 2021

padenot Jun 11, 2021

chcunningham Jun 11, 2021

padenot Jun 11, 2021

chcunningham Jun 11, 2021

chcunningham commented Jun 11, 2021

chcunningham commented Jun 11, 2021

chcunningham left a comment

chcunningham Jun 11, 2021

chcunningham Jun 11, 2021

chcunningham Jun 11, 2021

chcunningham Jun 11, 2021

Simplify VideoFrame Readback #208

Simplify VideoFrame Readback #208

Conversation

chcunningham commented Apr 30, 2021 • edited by pr-preview bot Loading

chcunningham commented Apr 30, 2021

chcunningham commented Apr 30, 2021

padenot commented May 3, 2021

chcunningham left a comment

Choose a reason for hiding this comment

chcunningham left a comment

Choose a reason for hiding this comment

padenot commented May 10, 2021

chcunningham commented May 10, 2021

chcunningham commented Jun 3, 2021

chcunningham commented Jun 4, 2021

chcunningham commented Jun 8, 2021

padenot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chcunningham commented Jun 11, 2021

chcunningham commented Jun 11, 2021

chcunningham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chcunningham commented Apr 30, 2021 •

edited by pr-preview bot

Loading