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

Should there be dedicated VideoFrame ReadableStream/WritableStream constructs #1187

Open
youennf opened this issue Nov 16, 2021 · 4 comments

Comments

@youennf
Copy link
Contributor

youennf commented Nov 16, 2021

As seen in various threads, using VideoFrame requires explicit closing, for instance when aborting streams.
Tee might be an issue, requiring to use structured cloning and a realtime mode to ensure limited buffering and branch independence.
If we start adding features for VideoFrame, we can wonder whether dedicated stream constructs for VideoFrame, like BYOB for array buffers, could be more ergonomic.

@youennf
Copy link
Contributor Author

youennf commented Nov 18, 2021

Following on last meeting discussion, a potential approach would be something like:

  • Add a new type to ReadableStream/WritableStream/TransformStream, say 'transfer'
  • If type is 'transfer', we apply the API contract we discussed for VideoFrame: controller.enqueue and writer.write take ownership of the object. In case of aborting a stream, enqueued values get cleaned up.

This makes the API contract very clear.
And there are additional good ergonomics (proper clean-up, tee would by default use structured cloning for this type).

Taking ownership could be implemented in various ways:

  • If object is transferable, use transfer steps
  • If object has clone/close methods, use these for transferring ownership and cleaning up.

The added complexity to the streams implementation seems mild.
We already have the type/readableType/writableType infrastructure in place.
We would have to store the 'transfer' type and update the enqueue/write/abort/transfer algorithms if type is 'transfer'.
One question might be whether this type should be queryable by JS.

One potential disadvantage happens for transferred streams.
In that case, if we allow random objects having custom close/clone methods, we lose the ability to call these methods in the transferred stream.
Maybe we should only allow streams of 'transfer' type to handle specific objects, say Transferable, with the possibility for a 'close-able' trait concept that would need to be defined.

Thoughts?

@dontcallmedom
Copy link
Contributor

Maybe relevant (re close-able trait) https://github.com/tc39/proposal-explicit-resource-management (not suggesting we wait for this in any fashion, but it may provide useful directions)

@MattiasBuelens
Copy link
Collaborator

MattiasBuelens commented Nov 18, 2021

Transferable solves the problem of moving ownership, but it doesn't solve explicit cleanup.

The explicit resource management proposal seems problematic when combined with transferable objects. What happens if user-land code overrides VideoFrame.prototype[Symbol.dispose], then transfers a ReadableStream containing VideoFrames, and then cancels that transferred stream? Should we call [Symbol.dispose] in the new realm, or should we transfer those VideoFrames back to the original realm and then call dispose on them?

We could add a concept for closeable objects, on the same level as transferable objects. Closeable IDL interfaces must be annotated with [Closeable], and must implement "close steps". We could even add a close() method to expose these steps to JavaScript, replacing VideoFrame.close(). If VideoFrame is [Transferable, Closeable], then we have all the tools we need to support proper ownership transfer and explicit cleanup.

Of course, the disadvantage is that regular JavaScript objects cannot be made closeable, so they cannot benefit from this cleanup mechanism. But perhaps we can still fix this in the future? If [Symbol.dispose] becomes a thing, we could do something like:

To close a chunk, perform the following steps:

  1. If chunk is closable (i.e. it implements a [Closeable] interface), then perform its close steps.
  2. Otherwise, if chunk has a [Symbol.dispose] method, then call this method.
  3. Otherwise, do nothing.

That way, platform objects like VideoFrame would always use their spec-defined close steps, so we don't have to worry about VideoFrame.prototype[Symbol.dispose]. But if user-land code introduces a [Symbol.dispose] method for certain objects, then we'll still call it.

We probably shouldn't assume that every stream containing closeable chunks wants those chunks to be closed in this way. If the chunks aren't transferable (which will be the case for user-land objects), then they may still be used outside the stream. So I would make this an opt-in with some sort of closeableChunks: true option on the underlying source/transformer/sink.

When type: "transfer", then ReadableStream is guaranteed to be the exclusive owner of its chunks, so closeableChunks: true is implied. Hmm, we still have to be careful about chunks that are [Transferable] but not [Closeable], since that would still call [Symbol.dispose]... Unless we make type: "transfer" require [Transferable, Closeable]? 🤔

@dontcallmedom
Copy link
Contributor

https://tc39.es/proposal-explicit-resource-management/ has reached stage 3
also probably relevant to this, #1212

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

No branches or pull requests

3 participants