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

[WebCodecs VideoFrame metadata registry] Introduce VideoFrame metadata #559

Merged
merged 11 commits into from
Oct 20, 2022

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Sep 14, 2022

@youennf youennf marked this pull request as draft September 14, 2022 01:45
@youennf
Copy link
Contributor Author

youennf commented Sep 14, 2022

@sandersdan FYI.

index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
index.src.html Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
@sandersdan
Copy link
Contributor

We just discussed this among the Chrome media team, and it turns out we're okay leaving out the user metadata. This would allow us to sidestep most of the questions about serialization.

The reasoning here is that if we're not storing metadata also on chunks, and we're not copying metadata between chunks and frames, then users will have to map their own metadata to frames separately anyway (presumably by timestamp). Therefore the user metadata is just a convenience rather than a critical feature in the short term.

@youennf
Copy link
Contributor Author

youennf commented Sep 14, 2022

it turns out we're okay leaving out the user metadata.

FWIW, one use case I can see is the AR/VR use case where metadata is retrieved by a worker through WebRTC encoded transform, then metadata is passing through decoder to MediaStreamTrack VideoFrames. In that case there might be two contexts so it seems nice to attach application metadata to VideoFrames directly.

That said, I am more than happy to leave this to another day/another PR.

@youennf
Copy link
Contributor Author

youennf commented Sep 15, 2022

I removed user metadata from this PR.
I kept dictionary/method for now.
There is a slight benefit for interface/attribute as we would return the same object every time, which might be handy if the metadata becomes a potentially big object.

@youennf youennf marked this pull request as ready for review September 15, 2022 00:34
index.src.html Outdated Show resolved Hide resolved
index.src.html Show resolved Hide resolved
@youennf
Copy link
Contributor Author

youennf commented Sep 15, 2022

One potential downsides of using a VideoFrameMetadata interface is that we would probably anyway need a VideoFrameMetadataInit dictionary.
Anytime a spec would extend VideoFrameMetadata with a new readonly attribute, it would also have to extend VideoFrameMetadataInit.

@youennf
Copy link
Contributor Author

youennf commented Sep 15, 2022

As a side note, VideoFrameMetadata name (even though dictionary names are not that important) conflicts with https://wicg.github.io/video-rvfc/#dictdef-videoframemetadata.
@tguilbert-google FYI.

@tguilbert-google
Copy link
Member

I'd be fine with renaming the rVFC VideoFrameMetadata to something else, to avoid collision. I think that VideoFrameMetadata makes more sense here.

Albeit verbose, rVFC VideoFrameMetadata could be renamed to VideoFrameCallbackMetadata?

@youennf
Copy link
Contributor Author

youennf commented Sep 16, 2022

rVFC VideoFrameMetadata could be renamed to VideoFrameCallbackMetadata?

Sounds good, dictionary names are not exposed to JS so verbosity is not a big issue anyway.

Copy link
Contributor

@sandersdan sandersdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach LGTM. The editors are more qualified to review the text of the changes.

index.src.html Outdated Show resolved Hide resolved
index.src.html Outdated Show resolved Hide resolved
@dalecurtis
Copy link
Contributor

Thanks, lgtm. @aboba @padenot any objections?

Copy link
Contributor

@dalecurtis dalecurtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still lgtm; thanks @youennf -- though there is some PR build error:

FATAL ERROR: Couldn't find 'webcodecs-video-frame-metadata-registry' in bibliography data.

@chrisn
Copy link
Member

chrisn commented Oct 10, 2022

Should we have a registry entry to run the CfC or is it fine to keep it empty?

I think it's fine to be empty.

I'll raise another PR for the remaining procedural bits. We can probably also drop this item from the Media WG meeting agenda for tomorrow.

@chrisn
Copy link
Member

chrisn commented Oct 14, 2022

@padenot, @aboba, is this OK to merge, and we can raise a separate PR for any remaining procedural requirements?

@padenot
Copy link
Collaborator

padenot commented Oct 17, 2022

Yes.

@youennf
Copy link
Contributor Author

youennf commented Oct 19, 2022

@aboba, can you review again the PR and approve it if you think this is fine?

@dalecurtis
Copy link
Contributor

Still lgtm; thanks @youennf -- though there is some PR build error:

FATAL ERROR: Couldn't find 'webcodecs-video-frame-metadata-registry' in bibliography data.

Bump on this one before submitting though.

@youennf
Copy link
Contributor Author

youennf commented Oct 20, 2022

@tidoust, can you shed some light on the issue @dalecurtis is mentioning above?

The entry is needed until the registry gets published and can be added to
official databases of specs.
@tidoust
Copy link
Member

tidoust commented Oct 20, 2022

@tidoust, can you shed some light on the issue @dalecurtis is mentioning above?

I pushed an update to add the entry to the local biblio. The entry can be removed when the registry document comes to existence and gets added to official biblio databases.

@dalecurtis dalecurtis merged commit 3b714ac into w3c:main Oct 20, 2022
@dalecurtis
Copy link
Contributor

Great, looks like we're all good here. Thanks @youennf!

github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Oct 20, 2022
SHA: 3b714ac
Reason: push, by dalecurtis

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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 this pull request may close these issues.

None yet

8 participants