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

Detach codec inputs (where feasible) #104

Open
chcunningham opened this issue Nov 9, 2020 · 14 comments
Open

Detach codec inputs (where feasible) #104

chcunningham opened this issue Nov 9, 2020 · 14 comments
Assignees
Labels
extension Interface changes that extend without breaking. PR exists A PR has been submitted that addresses this issue tag-tracker Group bringing to attention of the TAG, or tracked by the TAG but not needing response.

Comments

@chcunningham
Copy link
Collaborator

ImageDecoder isn't in the draft spec yet, but see here for latest interface in Chromium.

This is issue is a reminder that we should consider specifying that inputs are detached after the call to decode returns (neutering the buffer from JS pov). Similar to WebAudio's decodeAudio(). Detaching the inputs transfers ownership for the UA to safely perform asynch decoding without requiring a copy.

@sandersdan
Copy link
Contributor

Previously we decided against this for two reasons:

  1. It's not compatible with SharedArrayBuffer. In retrospect, it's probably fine to fall back to copying in this case, but that would mean inconsistent behavior.

  2. It doesn't allow the input buffers to be reused, and therefore creates extra GC pressure. In practice it doesn't seem like this garbage is consequential.

The primary alternative to transferring inputs is to always copy inputs immediately.

@chcunningham chcunningham changed the title Detach ImageDecoder inputs Detach codec inputs (where feasible) Nov 9, 2020
@chcunningham
Copy link
Collaborator Author

Title update: problem isn't unique to ImageDecoder. Lets fix this everywhere.

Agree with points 1 and 2 above.

@chcunningham
Copy link
Collaborator Author

Dropping some questions for TAG

  • is it better to always transfer when possible (e.g. yes for ArrayBuffer and ArrayBufferView, no for SAB) or to make it user configurable (e.g. transfer list)? For reference, inputs to WebAudio's decodeAudioData() are automatically transferred.
  • if configurable, is a transfer list the recommended approach? We've also considered dual static create methods like EncodedVideoChunk::TransferFrom() and EncodedVideoChunk::CopyFrom().

@padenot
Copy link
Collaborator

padenot commented Mar 24, 2021

2\. It doesn't allow the input buffers to be reused, and therefore creates extra GC pressure. In practice it doesn't seem like this garbage is consequential.

There are evidences in the Web Audio issue tracker that folks in the video game industry find this problematic and like "zero-garbage" paths.

I don't have a solution however, but a chat with representatives from this industry is planned.

@padenot
Copy link
Collaborator

padenot commented Mar 31, 2021

An update after talking with a couple of people from different industries: it's getting increasingly clear that detaching is needed, but that also internal copy is useful. We'll probably have to support both.

@padenot
Copy link
Collaborator

padenot commented May 5, 2021

A related issue is #212, where the detached input is reattached to another array buffer on output, so that authors can reuse it. This means we can have the best of both world: zero-copy input, but not a lot of malloc/free pairs.

@chcunningham
Copy link
Collaborator Author

I discussed this with @domenic (html and transferable expert) last week but haven't found time to send a PR. Here's a quick outline:

  • transfer list (like in postMessage) are more recently moving to a PostMessageOptions dict, containing a 'transfer' member
  • we add that dict as an argument to our constructors. folks that wish to transfer (say the ArrayBuffer used in VideoFrame PlaneInit) would then simply list the buffer like so:
    let frame = new VideoFrame(planeInit, {transfer: [ buffer ]} );
  • alternatively, we could incorporate the transfer argument into our *Init dictionaries directly. Same behavior as above.

Either way, we can do this without making a breaking change.

@chcunningham chcunningham added the extension Interface changes that extend without breaking. label May 12, 2021
@chcunningham
Copy link
Collaborator Author

triage note: marking extension, as we're converging on non-breaking proposals that simply add new arguments/keys.

Similarly, removing from launch blockers. This was only blocking when we thought it might be breaking. Offering a transfer ability is important, but hasn't blocked anyone currently experimenting with the API.

@chcunningham chcunningham removed this from the V1 Launch Blockers milestone May 12, 2021
@chcunningham
Copy link
Collaborator Author

Discussed in editors call. We like the last option

alternatively, we could incorporate the transfer argument into our *Init dictionaries directly. Same behavior as above.

@padenot
Copy link
Collaborator

padenot commented May 17, 2021

See also #212, that could also use the design mentioned in #104 (comment).

@dalecurtis
Copy link
Contributor

Transfer lists inside the dicts seem like a nice easy improvement. Will mark this ready for PR if someone wants to put that together. @Djuffin

@Djuffin Djuffin self-assigned this May 11, 2023
@Djuffin
Copy link
Contributor

Djuffin commented May 16, 2023

Proposal to add transfer field to the following *Init dictionaries.
This will allow detaching array buffers and using corresponding buffers inside VideoFrame, ImageDecoder, EncodedVideoChunk, EncodedAudioChunk, AudioData without a copy.
All buffers listed in the transfer field should be detached even if they are not actually used as a data source.
This is similar to behavior of structuredClone()

dictionary VideoFrameBufferInit {
   .......
  sequence<ArrayBuffer> transfer = [];  
};
dictionary ImageDecoderInit {
   .....

  sequence<ArrayBuffer> transfer = [];  
};
dictionary EncodedVideoChunkInit {

   .....
  sequence<ArrayBuffer> transfer = [];  
};
dictionary AudioDataInit {
   .....
  sequence<ArrayBuffer> transfer = [];  
}
dictionary  EncodedAudioChunkInit {
   .....
  sequence<ArrayBuffer> transfer = [];  
}

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 17, 2023
Implementation for w3c/webcodecs#104

Change-Id: Ifbc1fb1739a0bc19dc6db27943fff0cecae0156d
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 17, 2023
Implementation for w3c/webcodecs#104

Change-Id: Ifbc1fb1739a0bc19dc6db27943fff0cecae0156d
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 18, 2023
Implementation for w3c/webcodecs#104

Change-Id: Ifbc1fb1739a0bc19dc6db27943fff0cecae0156d
@chrisn
Copy link
Member

chrisn commented May 30, 2023

Discussed in Media WG meeting 30 May 2023, minutes.

@erights
Copy link

erights commented Dec 23, 2024

From #104 (comment) above

It's not compatible with SharedArrayBuffer. In retrospect, it's probably fine to fall back to copying in this case, but that would mean inconsistent behavior.

Would proposed Immutable ArrayBuffers enable you to avoid this copy?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension Interface changes that extend without breaking. PR exists A PR has been submitted that addresses this issue tag-tracker Group bringing to attention of the TAG, or tracked by the TAG but not needing response.
Projects
None yet
Development

No branches or pull requests

7 participants