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

Add API for changing metadata #161

Closed
sandersdan opened this issue Apr 1, 2021 · 7 comments
Closed

Add API for changing metadata #161

sandersdan opened this issue Apr 1, 2021 · 7 comments
Labels
editors-agenda Add to agenda for upcoming editors call extension Interface changes that extend without breaking.

Comments

@sandersdan
Copy link
Contributor

An app may have a frame (video or audio) but want to change its metadata (timestamp, duration, colorspace) before passing it to another component. I propose that we extend clone() to take an initialization dictionary that allows this metadata to be overwritten in the clone. It would probably make sense for any introduced copy() API to do the same.

@padenot
Copy link
Collaborator

padenot commented Apr 6, 2021

Having the capability to do this would be very useful. With copy-on-write semantics your API suggestion sounds fine in terms of performance, but maybe a bit unfamiliar to developers.

@chcunningham
Copy link
Collaborator

Triage note: marked 'extension' as the proposal is describing adding new arguments to a method.

@padenot
Copy link
Collaborator

padenot commented May 17, 2021

I tend to agree in practice, but this is breaking in theory (.length on the method changes).

@sandersdan
Copy link
Contributor Author

sandersdan commented May 21, 2021

Note: now that we support construction from a CanvasImageSource, and VideoFrame is a CanvasImageSource, this can be written as new VideoFrame(originalFrame, { /* new metadata */ }), and Chrome's implementation already supports overriding a timestamp in this way.

I think it may be surprising that this construction clones rather than copies, but the spec is already quite clear on this point.

Maybe we don't actually need clone() at all?

@chcunningham
Copy link
Collaborator

@sandersdan what are your feelings as of late? Do you want to see metadata overrides added to clone() or is the VideoFrame constructor option sufficient? I can see it either way (i.e. symmetry is nice, but argument could also be made that clone() is the "simple" option).

@sandersdan
Copy link
Contributor Author

I no longer feel strongly about copy() and clone() supporting these options. It would presumably be trivial to support that use, but the added complexity in teaching the API is probably not worth it.

I suppose there is also the inverse to consider, an option for new VideoFrame() to force copy semantics. This has the advantage of actually highlighting the default clone behavior, but of course is not async which likely is a showstopper.

@chcunningham
Copy link
Collaborator

Ok, lets close this for now. If we hear folks hate the current options we'll reconsider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editors-agenda Add to agenda for upcoming editors call extension Interface changes that extend without breaking.
Projects
None yet
Development

No branches or pull requests

3 participants