VideoTexture: Allow changing image source

[LeviPesin]

Related issue: #25530 (comment) #24757 #24769

Description

Don't throw if video is null or undefined. Also allow changing image source.

[github-actions]

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev	Filesize PR	Diff
633.9 kB (157.1 kB)	634.2 kB (157.1 kB)	+299 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev	Filesize PR	Diff
425.1 kB (103 kB)	425.1 kB (103 kB)	+0 B

[sunag]

I think that this check should be in something like texture.video = video for example:

class VideoTexture extends Texture {
...
	set video( video = null ) {

		if ( ( video !== undefined ) && ( video !== null ) && ( video.requestVideoFrameCallback !== undefined ) ) {

			video.requestVideoFrameCallback( updateVideo );

		}

	}
...
}

Because it could be added implicitly.

[LeviPesin]

I think this should be changed in another PR.

[Mugen87]

Can you please explain why the additional check is now necessary? I'm afraid it is not clear from the comment you have linked in your first post.

[LeviPesin]

With this PR something like new VideoTexture() works (similar to new Texture(), new CubeTexture(), new DataTexture()).

[Mugen87]

It seems the PR does not work so far. When I use your version of VideoTexture and update webgl_materials_video such that the video is defined at a later point (via texture.image = video;), the texture stays black. And I think that happens because requestVideoFrameCallback() is never triggered.

[LeviPesin]

Updated with @sunag's suggestion that fixes this.

[Mugen87]

Can you please verify what happens when you replace a video after the initial use of a texture? Is requestVideoFrameCallback() still executed for the replaced video?

[LeviPesin]

Yes, but I think we have decided that image source shouldn't be changed afterwards -- see #24757, #24768, and #24769. But it can be fixed to allow this if desired.

[LeviPesin]

Updated.

I also moved updateVideo to an outer function (binding to this in the constructor) to make it slightly cleaner.

[LeviPesin]

This line sets needsUpdate to true when the video changes, which is correct behaviour, I think. (It has a side effect that it sets it after first created, but I think this shouldn't break anything?)

[Mugen87]

I have the feeling it is better to make the video to a mandatory parameter. In this case, we can avoid the additional code and keep the class more simple.

[LeviPesin]

One of the use cases for using null or undefined instead of a video is:

three.js/examples/jsm/renderers/webgpu/WebGPUTextures.js

Lines 59 to 61 in 03e4e05

	const video = document.getElementById( 'video' );
	const texture = new VideoTexture( video );

Also, all other textures support ommiting image parameter (and all parameters).

[LeviPesin]

@mrdoob @Mugen87 So what's your opinion about this PR?

[LeviPesin]

@mrdoob @Mugen87 Can this PR be merged, please? It both makes the code easier to understand, more compatible with other textures, and allows a few more use-cases.

[LeviPesin]

@mrdoob @Mugen87 What is your opinion about this PR?

[Mugen87]

The resulting complexity isn't satisfying. I think it's best to keep the code as it is.

[LeviPesin]

Well... But the original version of this PR wasn't that complex and you've requested to make it a bit more? But I don't think even the final version is so much more complex than it was originally...

[LeviPesin]

Also, I think multiple users (at least one issue linked in the original post and you can count me too) has requested in the past the possibility to change the VideoTexture source, which this PR also addresses (and which you've also requested above).

@Mugen87 So may the decision to close this PR be reconsidered, please? I still think having this possibility would be better.