Skip to content
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

DRAFT: Potencial fix for VideoTexture #24758

Closed
wants to merge 5 commits into from
Closed

Conversation

eXponenta
Copy link
Contributor

@eXponenta eXponenta commented Oct 7, 2022

Related issue: #24757

Description

Check texture reference in update method safly, null supported.

DRAFT, this is as idea!

@eXponenta eXponenta marked this pull request as ready for review October 7, 2022 12:24
const hasVideoFrameCallback = 'requestVideoFrameCallback' in video;

if ( hasVideoFrameCallback && video !== this._callbackSource ) {
Copy link
Contributor

@LeviPesin LeviPesin Oct 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should stop the previous callback loop when creating a new one (by storing the callback ID and calling cancelVideoFrameCallback on it).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is problem.

We should store and callbackId and source reference, that add already 2 extra field to instance, what is undesirable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way to solve would be just to check in update function whether the callback source and the video are still the same, and request callback only if they still are.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check can be made in the start of update, so if the video changes the old callback would return before setting needsUpdate to true.

update is ticks with render FPS, and you can't requiest video callback from update each call.

But you do currently?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this will siplify code.

this._callbackSource = video;

const update = () => {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ( this._callbackSource !== video ) return;

A simpler alternative to 1469756.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 8, 2022

Sorry, but I think the PR introduces a complexity in VideoTexture which is no acceptable to address #24757. Right now, I think it's best to update the documentation and state that it's not possible to change the video after the creation of a VideoTexture. There is a similar restriction in place for the dimensions, format, and type of a texture.

@Mugen87 Mugen87 marked this pull request as draft October 10, 2022 07:47
@LeviPesin LeviPesin mentioned this pull request Oct 10, 2022
@Mugen87 Mugen87 closed this Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants