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

EncodedVideoFrame needs to be marked serializable #181

Closed
alvestrand opened this issue Jun 1, 2023 · 4 comments · Fixed by #182
Closed

EncodedVideoFrame needs to be marked serializable #181

alvestrand opened this issue Jun 1, 2023 · 4 comments · Fixed by #182

Comments

@alvestrand
Copy link
Contributor

The algorithm specified seems to indicate that EncodedVideoFrame (and audio) is serializable, since it uses the serialization steps to move it around.

This means that "structuredClone(frame)" should give sensible results.

We should mark the IDL accordingly.

(Related to #161)

@youennf
Copy link
Collaborator

youennf commented Jun 7, 2023

The spec is using serialise with transfer as a way to copy the detach the data ownership and ensure there is no TOCTOU issue. I do not think the intent was to enable serialisation of frames.
We can rewrite the spec to detach the array buffer and copy the necessary information to make things clearer.
We can discuss making frames serialisable in parallel.

@jan-ivar
Copy link
Member

jan-ivar commented Jun 16, 2023

The spec is using serialise with transfer as a way to copy the detach the data ownership and ensure there is no TOCTOU issue. I do not think the intent was to enable serialisation of frames.
We can rewrite the spec to detach the array buffer and copy the necessary information to make things clearer.

Thanks for explaining. I agree that unless the WG decides frames should be serializable, we need to rewrite that since neither RTCEncodedAudioFrame nor RTCEncodeVideoFrame contain the prerequisite serialization or deserialization steps necessary to have "Let serializedFrame be StructuredSerializeWithTransfer(frame, « data »)." and "Let frameCopy be StructuredDeserialize(serializedFrame, frame’s relevant realm)." work.

We can discuss making frames serialisable in parallel.

To save time, can this be that issue?

  • If we decide NO, we do a PR with the above rewrite
  • If we decide YES, we do a PR with the missing (de)serialization steps, and [Serializable] in the WebIDL

Does that work?

@jan-ivar
Copy link
Member

jan-ivar commented Jun 16, 2023

@alvestrand's request seems based on need in the #160 use case, and now that structuredClone(value[, { transfer }]) exists in the web platform, inventing custom frame.clone() methods like in #161 seems wrong to me.

This would also solve whether to clone the frame buffer or not, by leaving it up to JS:

const frameCopy = structuredClone(frame); // clones the data as well

const frameMove = structuredClone(frame, {transfer: [frame.data]}); // transfers the data

@dontcallmedom-bot
Copy link

This issue was mentioned in WEBRTCWG-2023-09-12 (Page 49)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants