Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 Video Uploading with WebCodecs sample #247
Add Video Uploading with WebCodecs sample #247
Changes from 1 commit
245d806
acf4635
693600b
606cec2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know this API at all so can't really review this. Seem reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I used similar code to https://github.com/gpuweb/cts/blob/0ac5d46/src/webgpu/web_platform/util.ts#L223
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem rather complex given that VideoFrame has a constructor that takes an HTMLVideoElement. But maybe this is necessary. It would be nice to document why it's done this way, because it's in a sample that people will reference and copy from. @shaoboyan could you help with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kainino0x (OOO last Friday). Sry for late reply. Since we use pre-recorded video file as the input for VideoFrame, we need to use HTMLVideoElement to play it and call
captureStream
to get the VideoFrame. I considered constructing video frame in another way by using canvas and captureStream to canvas but it seems in that way we cannot cover multiple color space.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to remove the
getVideoFrameFromVideoElement
function and simply useconst videoFrame = new VideoFrame(video);
for this sample if it helps.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it works for this sample I think we should.
@shaoboyan the color space issue is just due to the canvas, right? What about the
new VideoFrame(video)
codepath, does that work?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, due to canvas constraint color space.
That would be ideal!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! See #251