-
Notifications
You must be signed in to change notification settings - Fork 312
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
Conversation
1bbff25
to
c6c2ce6
Compare
c6c2ce6
to
acf4635
Compare
@kainino0x Please have another look. This PR is now ready for review. |
45f625c
to
693600b
Compare
minFilter: 'linear', | ||
}); | ||
|
||
function getVideoFrameFromVideoElement(video) { |
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 use const 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.
@shaoboyan the color space issue is just due to the canvas, right?
Yes, due to canvas constraint color space.
What about the new VideoFrame(video) codepath, does that work?
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
f9f13fe
to
81c8b1b
Compare
81c8b1b
to
606cec2
Compare
This PR adds a sample "Video Uploading with WebCodecs", similar to "Video Uploading", so that web developers can play with
importExternalTexture()
andVideoFrame
from Web Codecs API.