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

Update player.js faster more reliable autoplayDisable #2272

Merged
merged 4 commits into from
May 16, 2024
Merged

Update player.js faster more reliable autoplayDisable #2272

merged 4 commits into from
May 16, 2024

Conversation

raszpl
Copy link
Contributor

@raszpl raszpl commented May 16, 2024

Pause immediately after play without relinquishing js event loop queue spot (previous settimeout gave browsers opportunity to delay pausing)
Call html video element pause() first, it seems faster?
Finally remove autoplayDeniedOnce. YT is autostarting most videos couple of times, autoplayDeniedOnce was breaking autoplayDisable.

Final result is our autoplayDisable no longer plays up to couple seconds before pausing. Added bonus of moving autoplayDisable below calling original.apply(this, arguments) is any problems in autoplayDisable can no longer prevent/break Play.

@raszpl raszpl marked this pull request as ready for review May 16, 2024 01:53
@ImprovedTube ImprovedTube merged commit 5aa518e into code-charity:master May 16, 2024
@raszpl raszpl deleted the patch-15 branch May 16, 2024 08:59
@raszpl
Copy link
Contributor Author

raszpl commented May 16, 2024

settimeout gave browsers opportunity to delay pausing

this function

This comment was about original setTimeout(function () { player.pauseVideo(); });

, retries, in case defining player fails

no it doesnt :)

, with no side-effects

with no effect at all

. responding to your commit for #2137. Did you actually see it's log happening? "autoplayOff is waiting for..." (if so we should watch timing for this and more features.

Have you tested your hyphotesis? Edit :

(function waitForPlayer(){if(player=this.elements.player||videoElement.closest('#movie_player')){return;}

to read
(function waitForPlayer(){if(player=this.elements.player1||videoElement.closest('#movie_player1')){return;}
this will always evaluate false because this.elements.player1 doesnt exist and neither any '#movie_player1')
now test this code in a debugger :) I already listed 4 problems with it in 2df3a5e

1 you cant block a functions progress by calling settimeout, it just skips it and continues
2 even if you made it work with clever async/await you should never block event handlers like that, it would freeze UI
3 we are already in a .play() event handler, that means VIDEO element already exists thus videoElement.closest('#movie_player') is sufficient. If .closest('#movie_player') doesnt exist we arent interested in pausing such play event anyway.
4

}else if(tries===4){console.error("resigning autoplayOff after 1.5s")}

if(tries++<4){}else if(tries===4) {} will never execute Else clause because var++ is evaluated pre increment, thus you execute (3<4) else (4===4) and on the next loop (4<4) else (5===4) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Increment

tldr: javascript doesnt work like that. This

(function waitForPlayer(){if(player=this.elements.player||videoElement.closest('#movie_player')){return;}
else if(tries++<4){
console.log("autoplayOff is waiting for ImprovedTube.elements.player or #movie_player");
setTimeout(waitForPlayer,500);
}else if(tries===4){console.error("resigning autoplayOff after 1.5s")}
})()

just executes player=this.elements.player||videoElement.closest('#movie_player') and nothing else there ever matters. subsequent waitForPlayer() calls are made after handler finished executing, video is already playing, and nothing beyond line 15 will ever be executed again.

breaking functions should not break other scripts

thats why its safer to move it after original play().

@raszpl
Copy link
Contributor Author

raszpl commented May 17, 2024

well this is bad, you pushed this bad code to both builds without testing it
(function waitForPlayer(){if(player=this.elements.player||videoElement.closest('#movie_player')){return;}

crashes because this of waitForPlayer() is not ImprovedTube, making this.elements undefined and this.elements.player crash

Despite changing order it randomly breaks pause, this is a good time to pull the update
fix #2282

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.

2 participants