Constructor frame #223

guidou · 2024-01-25T20:14:31Z

dontcallmedom-bot · 2024-03-27T10:25:58Z

This issue was mentioned in WEBRTCWG-2024-03-26 (Page 40)

jan-ivar · 2024-03-28T15:47:39Z

index.bs

@@ -426,12 +426,34 @@ dictionary RTCEncodedVideoFrameMetadata {
 // re-use or extend the equivalent defined in WebCodecs.
 [Exposed=(Window,DedicatedWorker), Serializable]
 interface RTCEncodedVideoFrame {
+    constructor(RTCEncodedVideoFrame originalFrame, optional RTCEncodedVideoFrameMetadata newMetadata);


I think I'd prefer the second argument to be an options object, e.g.

new RTCEncodedVideoFrame(frame, {metadata: frame.getMetadata()});

youennf · 2024-03-28T15:47:56Z

index.bs

+            When called, run the following steps:
+
+            1. Set this.`[[type]]` to |originalFrame|.`[[type]]`.
+            1. Let this.`[[data]]` be a new ArrayBuffer object whose `[[ArrayBufferData]]` internal slot value is |originalFrame|.`[[data]]`.`[[ArrayBufferData]]`, and whose `[[ArrayBufferByteLength]]` internal slot value is |originalFrame|.`[[data]]`.`[[ArrayBufferByteLength]]`.


We probably want to clone the data and I am not sure it does that there.

Yes, this is meant to do deep clone the data. Does that work?

Changed it now to use CloneArrayBuffer definition so that it's more clear.

youennf · 2024-03-28T15:52:48Z

index.bs

+            1. Let this.`[[data]]` be a new ArrayBuffer object whose `[[ArrayBufferData]]` internal slot value is |originalFrame|.`[[data]]`.`[[ArrayBufferData]]`, and whose `[[ArrayBufferByteLength]]` internal slot value is |originalFrame|.`[[data]]`.`[[ArrayBufferByteLength]]`.
+            1. Let `[[metadata]]` represent the metadata associated with this newly constructed frame.
+                1. For each {`[[key]]`,`[[value]]`} pair of |originalFrame|.`[[getMetadata()]]`, set `[[metadata]]`.`[[key]]` to `[[value]]`.
+                1. For each {`[[key]]`,`[[value]]`} pair of |newMetadata|, if the `[[value]]` exists, set `[[metadata]]`.`[[key]]` to `[[value]]`.


We probably want to make sure that changing value does not change what metadata would be exposed in the newly constructed object. This seems fine as is now.
It might be good to add a [[metadata]] internal slot.

Can I add the [[metadata]] internal slot in a follow-up change because that would need using this internal slot in the whole encoded-transforms spec, which would be unrelated to this change?

Sounds fine, but let's add a comment that we want to do a deep copy here.

youennf · 2024-04-18T14:33:00Z

index.bs

+            1. Let this.`[[data]]` be the result of <a href="https://tc39.es/ecma262/#sec-clonearraybuffer">CloneArrayBuffer</a>(|originalFrame|.`[[data]]`, 0, |originalFrame|.`[[data]]`.`[[ArrayBufferByteLength]]`).
+            1. Let `[[metadata]]` represent the metadata associated with this newly constructed frame.
+                1. For each {`[[key]]`,`[[value]]`} pair of |originalFrame|.`[[getMetadata()]]`, set `[[metadata]]`.`[[key]]` to `[[value]]`.
+                1. For each {`[[key]]`,`[[value]]`} pair of |options|`[metadata]`, if the `[[value]]` exists, set `[[metadata]]`.`[[key]]` to `[[value]]`.


Suggested change

1. For each {`[[key]]`,`[[value]]`} pair of |options|`[metadata]`, if the `[[value]]` exists, set `[[metadata]]`.`[[key]]` to `[[value]]`.

1. For each {`[[key]]`,`[[value]]`} pair of |options|`[metadata]`, if `[[value]]` is not <code>undefined</code>, set `[[metadata]]`.`[[key]]` to `[[value]]`.

Or remove it

youennf · 2024-04-18T14:36:10Z

index.bs

+            When called, run the following steps:
+
+            1. Set this.`[[type]]` to |originalFrame|.`[[type]]`.
+            1. Let this.`[[data]]` be the result of <a href="https://tc39.es/ecma262/#sec-clonearraybuffer">CloneArrayBuffer</a>(|originalFrame|.`[[data]]`, 0, |originalFrame|.`[[data]]`.`[[ArrayBufferByteLength]]`).


There might be better bs way of referring to tc39, probably by adding the URL at the beginngin of index.bs

jan-ivar

Agree with other comments.

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

palak8669 added 4 commits January 16, 2024 16:05

Add constructor for encoded frames

03df57b

Update

4f2c438

deep clone of data

303d24f

update wording of metadata

dccb6f6

guidou marked this pull request as draft January 25, 2024 20:15

guidou requested review from alvestrand, jan-ivar and youennf February 8, 2024 11:26

guidou marked this pull request as ready for review February 8, 2024 11:26

alvestrand approved these changes Feb 26, 2024

View reviewed changes

aboba approved these changes Feb 26, 2024

View reviewed changes

guidou mentioned this pull request Mar 13, 2024

Need for modifying metadata #135

Open

jan-ivar reviewed Mar 28, 2024

View reviewed changes

youennf reviewed Mar 28, 2024

View reviewed changes

palak8669 and others added 3 commits April 2, 2024 16:01

Changes to address the comments

9739ea1

rm internal slot

3c35ee7

Use CLoneArrayBuffer

417f896

guidou mentioned this pull request Apr 3, 2024

Constructors for RTC encoded frames with custom metadata w3ctag/design-reviews#942

Open

1 task

rename the Options

ae86de4

This was referenced Apr 15, 2024

Constructors for RTC Encoded Frames with metadata mozilla/standards-positions#1009

Closed

Constructors for RTC Encoded Frames with metadata WebKit/standards-positions#334

Closed

youennf reviewed Apr 18, 2024

View reviewed changes

jan-ivar approved these changes Apr 18, 2024

View reviewed changes

alvestrand added the Editors can integrate label Apr 18, 2024

palak8669 added 4 commits April 19, 2024 11:31

address comments

8ae6738

remove conditions on [[value]]

ae0c342

add introduction

f229c33

nits

18c1b3e

add deep copy in the steps

429171d

guidou merged commit 4b61373 into w3c:main Apr 19, 2024
2 checks passed

github-actions bot added a commit that referenced this pull request Apr 19, 2024

Merge pull request #223 from palak8669/constructor_frame

de9c454

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

guidou mentioned this pull request Jul 18, 2024

IDL changes for setMetadata for 3.2.2 webrtc nv use case #202

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Constructor frame #223

Constructor frame #223

guidou commented Jan 25, 2024 •

edited by pr-preview bot

Loading

dontcallmedom-bot commented Mar 27, 2024

jan-ivar Mar 28, 2024

palak8669 Apr 2, 2024

youennf Mar 28, 2024

palak8669 Apr 2, 2024

palak8669 Apr 3, 2024

youennf Mar 28, 2024

palak8669 Apr 2, 2024

youennf Apr 18, 2024

palak8669 Apr 19, 2024

youennf Apr 18, 2024

youennf Apr 18, 2024

palak8669 Apr 19, 2024

youennf Apr 18, 2024

palak8669 Apr 19, 2024

jan-ivar left a comment

	1. For each {`[[key]]`,`[[value]]`} pair of \|options\|`[metadata]`, if the `[[value]]` exists, set `[[metadata]]`.`[[key]]` to `[[value]]`.
	1. For each {`[[key]]`,`[[value]]`} pair of \|options\|`[metadata]`, if `[[value]]` is not <code>undefined</code>, set `[[metadata]]`.`[[key]]` to `[[value]]`.

Constructor frame #223

Constructor frame #223

Conversation

guidou commented Jan 25, 2024 • edited by pr-preview bot Loading

dontcallmedom-bot commented Mar 27, 2024

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

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

jan-ivar left a comment

Choose a reason for hiding this comment

guidou commented Jan 25, 2024 •

edited by pr-preview bot

Loading