VideoTexture: Fix error when detecting requestVideoFrameCallback

[ianpurvis]

Fixes TypeError: video is not an Object. regression in #19906

Related issues:

#19906

Description

When VideoTexture is constructed with an undefined video, the requestVideoFrameCallback detection on line 24 throws an error.

[Mugen87]

So to clarify: This issue only happens if a video texture is cloned. Notice that the first parameter of VideoTexture is already mandatory (which is not true for other texture types).

However, I'm not sure this change is correct. When a VideoTexture is now cloned, requestVideoFrameCallback() will be called twice by both instances of VideoTexture (which is obviously not correct, too).

[ianpurvis]

For more context- I ran into the issue updating three where a ShaderMaterial was calling UniformsUtils.merge:

video is not an Object. (evaluating ''requestVideoFrameCallback' in video')
VideoTexture (three.module.js:27130)
clone (three.module.js:1460)
cloneUniforms (three.module.js:11568)
mergeUniforms (three.module.js:11594)

Let me know if there is a better way... the intention is for each instance of VideoTexture to update itself, right?

[Mugen87]

Maybe it's better to do the following for now:

if ( video !== undefined && 'requestVideoFrameCallback' in video ) {

Besides, one could also investigate if it's possible to move the usage of requestVideoFrameCallback into WebGLTextures.

[ianpurvis]

if ( video !== undefined && 'requestVideoFrameCallback' in video ) {

Yeah, I was also thinking about that approach. One issue is that if the requestVideoFrameCallback update loop doesn't get created during construction, update will never satisfy this condition:

   if ( hasVideoFrameCallback === false && video.readyState >= video.HAVE_CURRENT_DATA ) {

The WebGLTextures approach sounds interesting... maybe this PR can be used to fix the regression while that optimization is developed

[Mugen87]

The additional undefined seems sufficient for now.

update will never satisfy this condition:

Not sure I understand this argument. VideoTexture.update() is a fallback for the case requestVideoFrameCallback is not available.

[ianpurvis]

Sorry, let me try to clarify- In my testing VideoTexture.update() gets called by WebGLTextures.updateVideoTexture regardless of if requestVideoFrameCallback is available. The fallback actually happens inside VideoTexture.update():

	update: function () {

		const video = this.image;
		const hasVideoFrameCallback = 'requestVideoFrameCallback' in video;

		if ( hasVideoFrameCallback === false && video.readyState >= video.HAVE_CURRENT_DATA ) {

			this.needsUpdate = true;

		}

	}

So in environments where hasVideoFrameCallback is true, the texture will never touch needsUpdate

[Mugen87]

Okay, that cleared things up. The missing assignment to needsUpdate makes it not possible to properly use the clone.

I'm still not sure it's safe to use requestVideoFrameCallback() like in this PR. If it's just a callback that is fired when a new frame is ready then fine. But it would be bad if the invocation triggers more under the hood. Hence, my doubts.

It seems this issue could be easier solved with #17949 merged. Keep in mind that when cloning a (video) texture right now, you have duplicate uploads to the GPU. Which is probably not something you want.

[ianpurvis]

I'm still not sure it's safe to use requestVideoFrameCallback() like in this PR. If it's just a callback that is fired when a new frame is ready then fine. But it would be bad if the invocation triggers more under the hood.

I agree... out of curiosity, I dug into the Chrome source. It looks like work is done once per frame and then shared by all callbacks:

• video_frame_callback_requester_impl.cc#114
• video_frame_request_callback_collection.cc#43

Everything appears to scale safely depending on how heavy the callbacks themselves are. In this test I'm able to add around 768 callback loops before things begin to exceed the frame window on a MacBook Pro. And around 512 loops on a Pixel 2 XL.

Keep in mind that when cloning a (video) texture right now, you have duplicate uploads to the GPU. Which is probably not something you want.

Yeah, I didn't realize what UniformsUtils.merge was doing there. Mmm, copypasta...

[Mugen87]

All right, then I guess it's okay to merge this for now. One might refactor this code when duplicate texture uploads are solved.

[mrdoob]

Thanks!