-
Notifications
You must be signed in to change notification settings - Fork 195
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
WebM Video Builder #2200
base: master
Are you sure you want to change the base?
WebM Video Builder #2200
Conversation
…s backgrounded though
… Also doc and test updates
modules/video/src/video-builder.ts
Outdated
source instanceof WebGL2RenderingContext | ||
) { | ||
const gl = source; | ||
// const {width, height} = gl.canvas; |
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.
Either one is fine with me. Preference?
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.
Either should be fine if they're both the buffer size. I've assumed at this point const {width, height} = gl.canvas;
was the way to do it, but I only have limited theoretical knowledge of how it differs from GL.VIEWPORT
.
Do you know if GL.VIEWPORT
is meaningfully different? The spec suggests they are synced on the application level.
Pulling the correct size has been a source of issues in the past due to subtle differences in how libs handle it. Luma.gl uses gl.canvas
. Mapbox did something different. Maplibrae has diverged now I think. I tried to sort it out in this issue and rfc, but I wouldn't be surprised if it comes up again in some corner case.
I say go with what you know. I only know enough to flag it might not be straight forward.
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.
Oh interesting! And good to know, thanks for doing the legwork here.
Seems like pulling from gl.canvas
is the move. I had no idea this was such a hot button issue!
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.
- loaders.gl is generally WebGL agnostic - it doesn't depend on WebGL but works with typed arrays which are optimized for WebGL use.
- Also loaders.gl doesn't use Node.js constructs like
Buffer
, preferring to use typed arrays in all interfaces. Simply mentioningBuffer
tends to pull in a 50KB polyfill in browser bundles...
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 would have liked it more if this function just accepted a typed array and if necessary we offered some utility function for getting hold of a typed array from different input types.
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 think in ever case here this function reduces buffer to an Uint8Array
, right?
If so, the Buffer
block be switched to a block that only accepts source instanceof Uint8Array
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.
Yeah so one way or another, we have to send an ArrayBuffer instead of Uint8Array due to the way Web Workers handle shared memory. I think it's a bit cleaner for the implementing side (one less helper function that is required for all calls) if we handle the conversion on the library level for canvas, gl, and Buffers, but I also understand more generally wanting to work exclusively in the land of typed data (and, to Ib's point, not needing to pull in a huge node polyfill).
Ib, what are you expecting for the common use case for this library? Would this helper live in Loaders? Luma?
|
||
async onFinalize({gl}) { | ||
const videoDataUrl = await videoBuilder.finalize(); | ||
t.ok(videoDataUrl, 'finalize() returns WebM video URL'); |
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.
What should the two videos look like? Encoding is a newer frontier so I'm not sure if we've established how to verify it worked in a unit. We don't need to test webm-wasm
, but maybe there a way to verify the video isn't blank/undefined, and the resolution is as expected?
cc @ibgreen
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.
Yeah my thought here was to use the SnapshotTestRunner and then visually compare a couple of the frames (say, first, middle, last?). What do you think?
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.
That sounds solid. Very curious if compression is deterministic or flaky.
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.
My understanding is that since ScreenshotTest is perceptual, ideally it would return the same thing even if the bytes are different (though the two ImageDatas should have the same data with the same parameters, and from what I can tell from libvpx - the lib web-wasm wraps - it looks like there's an option to pass a debug mode flag to make the output deterministic).
modules/video/src/video-builder.ts
Outdated
source instanceof WebGL2RenderingContext | ||
) { | ||
const gl = source; | ||
// const {width, height} = gl.canvas; |
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.
Either should be fine if they're both the buffer size. I've assumed at this point const {width, height} = gl.canvas;
was the way to do it, but I only have limited theoretical knowledge of how it differs from GL.VIEWPORT
.
Do you know if GL.VIEWPORT
is meaningfully different? The spec suggests they are synced on the application level.
Pulling the correct size has been a source of issues in the past due to subtle differences in how libs handle it. Luma.gl uses gl.canvas
. Mapbox did something different. Maplibrae has diverged now I think. I tried to sort it out in this issue and rfc, but I wouldn't be surprised if it comes up again in some corner case.
I say go with what you know. I only know enough to flag it might not be straight forward.
Regarding the open questions,
|
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.
Nice addition! Sorry for delay, I am sure I reviewed this a few days ago but somehow my comments didn't make it in.
modules/video/src/video-builder.ts
Outdated
} | ||
|
||
export default class VideoBuilder { | ||
worker: Worker = new Worker('node_modules/webm-wasm/dist/webm-worker.js'); |
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.
We typically include such workers in our own dist, rather than import them from installed packages. Given that it is apache licensed there are no concerns with copying it, especially if we give some attribution.
The libs
folder in each module is the place to put such "binaries". There are a bunch of modules that do this.
The downside is we don't automatically get access to patches but I think it may cause problems for users if we depend on an npm install tree.
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.
Done. It seems like they don't update all that regularly, so we should be fine for the time being.
That said, I'm having trouble with the webpack config here... doesn't seem to be finding the worker code when I try to import (I see there's a rule for test: /\.worker\.js$/,
but it doesn't seem to be working). From what I can tell from the parquet-wasm and draco loader, it looks like they're loaded from third party CDNs. Any pointers on what to do here?
modules/video/src/video-builder.ts
Outdated
source instanceof WebGL2RenderingContext | ||
) { | ||
const gl = source; | ||
// const {width, height} = gl.canvas; |
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.
- loaders.gl is generally WebGL agnostic - it doesn't depend on WebGL but works with typed arrays which are optimized for WebGL use.
- Also loaders.gl doesn't use Node.js constructs like
Buffer
, preferring to use typed arrays in all interfaces. Simply mentioningBuffer
tends to pull in a 50KB polyfill in browser bundles...
modules/video/src/video-builder.ts
Outdated
source instanceof WebGL2RenderingContext | ||
) { | ||
const gl = source; | ||
// const {width, height} = gl.canvas; |
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 would have liked it more if this function just accepted a typed array and if necessary we offered some utility function for getting hold of a typed array from different input types.
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.
Updated with feedback. Worker URL situation is still a little wonky, would love some help or pointers here.
modules/video/src/video-builder.ts
Outdated
source instanceof WebGL2RenderingContext | ||
) { | ||
const gl = source; | ||
// const {width, height} = gl.canvas; |
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.
Oh interesting! And good to know, thanks for doing the legwork here.
Seems like pulling from gl.canvas
is the move. I had no idea this was such a hot button issue!
modules/video/src/video-builder.ts
Outdated
source instanceof WebGL2RenderingContext | ||
) { | ||
const gl = source; | ||
// const {width, height} = gl.canvas; |
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.
Yeah so one way or another, we have to send an ArrayBuffer instead of Uint8Array due to the way Web Workers handle shared memory. I think it's a bit cleaner for the implementing side (one less helper function that is required for all calls) if we handle the conversion on the library level for canvas, gl, and Buffers, but I also understand more generally wanting to work exclusively in the land of typed data (and, to Ib's point, not needing to pull in a huge node polyfill).
Ib, what are you expecting for the common use case for this library? Would this helper live in Loaders? Luma?
|
||
async onFinalize({gl}) { | ||
const videoDataUrl = await videoBuilder.finalize(); | ||
t.ok(videoDataUrl, 'finalize() returns WebM video URL'); |
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.
My understanding is that since ScreenshotTest is perceptual, ideally it would return the same thing even if the bytes are different (though the two ImageDatas should have the same data with the same parameters, and from what I can tell from libvpx - the lib web-wasm wraps - it looks like there's an option to pass a debug mode flag to make the output deterministic).
modules/video/src/video-builder.ts
Outdated
} | ||
|
||
export default class VideoBuilder { | ||
worker: Worker = new Worker('node_modules/webm-wasm/dist/webm-worker.js'); |
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.
Done. It seems like they don't update all that regularly, so we should be fine for the time being.
That said, I'm having trouble with the webpack config here... doesn't seem to be finding the worker code when I try to import (I see there's a rule for test: /\.worker\.js$/,
but it doesn't seem to be working). From what I can tell from the parquet-wasm and draco loader, it looks like they're loaded from third party CDNs. Any pointers on what to do here?
realtime: false | ||
}; | ||
|
||
type VideoBuilderOptions = Partial<typeof VIDEO_BUILDER_OPTIONS>; |
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.
WebMVideoBuilderOptions
@@ -0,0 +1,93 @@ | |||
const VIDEO_BUILDER_OPTIONS = { |
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.
WEBM_VIDEO_BUILDER_OPTIONS
Any thoughts about loading the worker file? Right now it depends on a specific hard coded path exposed through the server. I couldn't get the Webpack loader to require the file (I gave up debugging and didn't look too deeply into the config). |
How does the rest of loaders handle this, @ibgreen? It's not clear to me if this will this cause any issues as-is for users? |
Add a VideoBuilder class
The web runs primarily realtime and interactive but there are a number of cases where you might want the content in the canvas to run out of realtime to render to a video. A very fast or very slow draw call, e.g.
This builds on the work of the GifBuilder class and adds a bit more info to the encoding-rfc. Please let me know if I should make any edits.
cc @chrisgervang and visgl/hubble.gl#236
Open questions
Is it right to wrap the various APIs or should the client be responsible for passing in a buffer? I'm leaning towards ease-of-use for common cases, but the client can also pass in a buffer.
Should this use the frameBuffer instead of using readPixels? Should it use readPixelsToBuffer?
Alternatives considered