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.

Sign up for GitHub

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

Jump to bottom

Explainer for proposed stream changes related to VideoFrame processing #1193

Merged
merged 15 commits into from
Dec 16, 2021
Merged

Explainer for proposed stream changes related to VideoFrame processing #1193

merged 15 commits into from
Dec 16, 2021

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Dec 2, 2021

As discussed at last WebRTC/WHATWG Streams meeting, we are converging on a potential solution for VideoFrame handling in streams. This explainer is the first step towards a more formal solution.

@youennf
Copy link
Contributor Author

youennf commented Dec 2, 2021

@MattiasBuelens, this is a first draft of the explainer, could you take a look?

MattiasBuelens
MattiasBuelens requested changes Dec 2, 2021
View reviewed changes
Copy link
Collaborator

@MattiasBuelens MattiasBuelens left a comment

Choose a reason for hiding this comment

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

Thanks for kicking this off! Looks pretty good already. 👍

streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
streams-for-raw-video-explainer.md Show resolved Hide resolved
streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
youennf and others added 7 commits December 2, 2021 13:03
@youennf @MattiasBuelens
Update streams-for-raw-video-explainer.md
ff82dd4 
Co-authored-by: Mattias Buelens <649348+MattiasBuelens@users.noreply.github.com>
@youennf @MattiasBuelens
Update streams-for-raw-video-explainer.md
f9b80aa 
Co-authored-by: Mattias Buelens <649348+MattiasBuelens@users.noreply.github.com>
@youennf @MattiasBuelens
Update streams-for-raw-video-explainer.md
e1d7c46 
Co-authored-by: Mattias Buelens <649348+MattiasBuelens@users.noreply.github.com>
@youennf
Use variable in example
6f4a10b
@youennf
Restrict to Serializable+Transferable.
98aed35 
Mention TransformStream readableType/writableType.
Add more future work/open questions.
@youennf
Add a note about memory for detached ArrayBuffer
aeae4a2
@youennf
Add alternative to not use streams
78d0c5b
@youennf
Copy link
Contributor Author

youennf commented Dec 3, 2021

@alvestrand, @aboba, @jan-ivar, @ricea, PTAL.

@youennf
Copy link
Contributor Author

youennf commented Dec 8, 2021

@domenic, could you also take a look whether that makes sense to you and whether that is worth including in streams (instead of special casing VideoFrame handling).

@domenic
Copy link
Member

domenic commented Dec 13, 2021

Generally looks good to me, and makes sense as a concept. Lots of details to work out, but I'm happy for that to be done in future revisions or in a spec PR, instead of having a perfect explainer ahead of time.

jan-ivar
jan-ivar reviewed Dec 13, 2021
View reviewed changes
Copy link
Contributor

@jan-ivar jan-ivar left a comment

Choose a reason for hiding this comment

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

Content looks great! Just a question on mentioning WebIDL, and nits. I also think we should show workers in the example, since that's what we've spec'ed (see suggestion).

streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
youennf and others added 6 commits December 15, 2021 09:43
@youennf @jan-ivar
Update streams-for-raw-video-explainer.md
4f1eba3 
Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>
@youennf @jan-ivar
Update streams-for-raw-video-explainer.md
08a53c5 
Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>
@youennf @jan-ivar
Update streams-for-raw-video-explainer.md
4620b37 
Co-authored-by: Jan-Ivar Bruaroey <jan-ivar@users.noreply.github.com>
@youennf
Use chunk instead of object
34e2fb8
@youennf
Updating example
a220610
@youennf
Mention explicitly that closing a transferred chunk is a no-op
1499abb
@youennf
Copy link
Contributor Author

youennf commented Dec 15, 2021

Build error seems unrelated to this PR:
LINK ERROR: Multiple possible 'WebSocket' idl refs.

@MattiasBuelens
Copy link
Collaborator

Correct, #1198 should fix the spec build.

jan-ivar
jan-ivar reviewed Dec 16, 2021
View reviewed changes
Copy link
Contributor

@jan-ivar jan-ivar left a comment

Choose a reason for hiding this comment

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

LGTM

@youennf
Copy link
Contributor Author

youennf commented Dec 16, 2021

Discussed at WebRTC editor's meeting today, and was thought as good to go as a first version.
@MattiasBuelens , @domenic, do you think it is ready for merging?

domenic
domenic approved these changes Dec 16, 2021
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM too. @MattiasBuelens has a changes-requested review which we need to make sure it satisfied though.

MattiasBuelens
MattiasBuelens approved these changes Dec 16, 2021
View reviewed changes
Copy link
Collaborator

@MattiasBuelens MattiasBuelens left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

streams-for-raw-video-explainer.md Outdated Show resolved Hide resolved
@domenic @MattiasBuelens
Incorporaite Mattias's typo fix
a65cd0b 
Co-authored-by: Mattias Buelens <649348+MattiasBuelens@users.noreply.github.com>
@domenic domenic merged commit 7f7d68b into whatwg:main Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvestrand alvestrand alvestrand left review comments

@jan-ivar jan-ivar jan-ivar left review comments

@domenic domenic domenic approved these changes

@MattiasBuelens MattiasBuelens MattiasBuelens approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@youennf @domenic @MattiasBuelens @alvestrand @jan-ivar