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 the spec require copying init.data when constructing EncodedAudioChunk and EncodedVideoChunk? #127

Closed
OllieJones opened this issue Jan 29, 2021 · 3 comments

Comments

@OllieJones
Copy link

Sections 9.1.1 and 9.2.1 of the draft WebCodecs spec say:

  1. Assign a copy of init.data to chunk.data.

I suggest the words a copy of be removed from the spec. It seems like leaving those words in requires unnecessary copying of compressed bitstream data.

Leaving these words out would allow a client of these constructors to make the decision about copying that data. It's possible for a client to pass a .subarray() of some ArrayBuffer. If that ArrayBuffer is volatile for some reason, it should be up to the client to copy the data.

Of course, if a decoder implementation requires the compressed bitstream data to be copied, the implementation can do so. But promising to do so adds a required copying step.

How about this wording?

  1. Assign init.data to chunk.data. The client must not allow init.data to change after using this ctor.
@sandersdan
Copy link
Contributor

sandersdan commented Jan 29, 2021

The goal here is to reduce the total number of copies. We can't just use an ArrayBuffer directly as it could be mutated, and if a decoder is currently reading from it that could result in TOCTTOU issues.

@chcunningham previously we discussed removing data and providing copyInto() instead, thus making encoded chunks immutable.

An alternative to immutable chunks is to have our decoders always copy the data before decoding it, but I don't really see an advantage in having mutable chunks.

@chcunningham
Copy link
Collaborator

We can't just use an ArrayBuffer directly as it could be mutated, and if a decoder is currently reading from it that could result in TOCTTOU issues.

One option under consideration is to transfer the provided ArrayBuffer when constructing the encoded chunk. This would mitigate TOCTTOU issues by detaching the users reference to the data.

@chcunningham previously we discussed removing data and providing copyInto() instead, thus making encoded chunks immutable.

I think this is probably the best we can do without read-only ArrayBuffers (WICG/reducing-memory-copies#1). It does impose a new copy for users of the encoder interface (e.g. presently they could just pass our data directly to WebTransport).

An alternative to immutable chunks is to have our decoders always copy the data before decoding it, but I don't really see an advantage in having mutable chunks.

I lean toward immutable. The alternative doesn't resolve the issue #80.

@chcunningham chcunningham added this to the Q1 2021 milestone Feb 18, 2021
@chcunningham
Copy link
Collaborator

I'm closing this as I think its work is tracked in duplicate issues. The plan is:

  1. Make Encoded*Chunks immutable. Tracked by EncodedChunks confusingly provide access to constructed ".data" -- despite the decoder making an internal copy. #80. This avoids the confusion described in that issue and allows us to eliminate one of the copies described above as we feed the chunk into the decoder.
  2. Allow callers to optionally transfer ownership of their ArrayBuffer's when constructing an Encoded*Chunk. Tracked by Detach codec inputs (where feasible)  #104. This obviously eliminates the copy (assuming the site didn't need to make its own copy to continue to have access to the data).

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

No branches or pull requests

3 participants