fix: validation for stalled and suspended event in iOS when headphones disconnect #6199

marcodeltorob · 2019-08-26T19:40:23Z

Description of the problem:
When playback is ongoing in iOS + Safari and the user disconnect headphones (wired or Bluetooth) the player will throw 'suspend' or 'stalled' after a couple of seconds and 'progress' events.

Solution: Validation for stalled and suspended event for iOS browser when headphones are disconnected

…ones are disconnected

welcome · 2019-08-26T19:40:25Z

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

Run npm run lint -- --errors locally to catch formatting errors earlier.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

misteroneill · 2019-08-27T15:38:34Z

src/js/tech/html5.js

+          (this.el_.buffered.end(this.el_.buffered.length - 1) + SAFE_TIME_DELTA >= this.el_.currentTime)) {
+        this.el_.pause();
+      }
+    };


I don't think there's much benefit in having this as a separate function. The body of it can probably go directly into the callback below.

misteroneill · 2019-08-27T15:41:21Z

src/js/tech/html5.js

+
+    const disconnectionIOSListener = () => {
+      if ((!this.el_.paused && window.navigator.onLine) &&
+          (this.el_.buffered.end(this.el_.buffered.length - 1) + SAFE_TIME_DELTA >= this.el_.currentTime)) {


It would be good to have some explanatory comments about this condition and the TIME_FUDGE_FACTOR and SAFE_TIME_DELTA (and that those come from VHS).

We should probably have a local variable here for buffered and check if buffered has length, before trying to get the end of the buffer. Also would it make sense to use tech methods instead of el_ methods here: IE this.pause() or this.buffered()

Also, it's possible that there are multiple buffered regions, and we're current in the first or a middle buffered region instead of the last one.

All good points!

@gkatsev Can you explain a little more, since we try to get the end time buffered. (length - 1) and if we have multiple regions it does not matter that means that we still have buffered time and we are not experiencing a Stalled or suspend due another problem

@marcodeltorob basically, we could be at the end of a buffered region which isn't the last buffered region available, so, currentTime will end up being less than the buffered region.
For example, we're at 10 seconds in (marked by |, and our buffer looks something like:

[0-10|----20-30----60-70----]

Reading the last buffered region, would give us 70 but our currentTime is 10. However, we still can't play because we've run out of buffer.
But, if the currentTime is say 5, and our buffered region has extra buffer, then we should be fine.

[0-5|5-10----20-30----60-70----]

…ody inside event funcion

Updates needed related to buffered regions.

misteroneill · 2019-09-09T14:43:10Z

src/js/tech/html5.js

+
+        // Establish if we have an extra buffer in the current time range playing.
+        for (let i = 0; i < buffered.length; i++) {
+          if (buffered.start(i) <= this.currentTime() <= buffered.end(i) + SAFE_TIME_DELTA) {


I think what we want is to check if there is anything in the forward buffer. It seems that if the currentTime is equal to the buffered.end, then we don't have anything in the forward buffer. Maybe just buffered.start <= currentTime < buffered.end?

Should we adjust the buffered.start using SAFE_TIME_DELTA as well? Maybe not.

We might as well break out of the loop when we set extraBuffer = true.

gkatsev · 2019-09-09T17:58:00Z

src/js/tech/html5.js

+
+        // Establish if we have an extra buffer in the current time range playing.
+        for (let i = 0; i < buffered.length; i++) {
+          if (buffered.start(i) <= this.currentTime() < buffered.end(i) + SAFE_TIME_DELTA) {


I just verified and buffered.start(i) <= this.currentTime() < buffered.end(i) doesn't work.
It'll get evaluated as if there are parenthesis around the buffered.start(i) <= this.currentTime() and evaluate it to true (potentially) which will end up being smaller than any buffered.end(i).

> [2, 4, 6, 8, 10].map((f) => 2 <= f < 8) < [true, true, true, true, true]

> [2, 4, 6, 8, 10].map((f) => 2 <= f && f < 8) < [true, true, true, false, false]

gkatsev · 2019-09-09T18:03:34Z

src/js/tech/html5.js

+        }
+
+        // if tech is not paused, browser has internet connection & player has extraBuffer inside the timeRange
+        if ((!this.paused() && window.navigator.onLine) && extraBuffer) {


I don't think the extra parens are necessary as the expression will just evaluate each item in turn.

Suggested change

if ((!this.paused() && window.navigator.onLine) && extraBuffer) {

if (!this.paused() && window.navigator.onLine && extraBuffer) {

Also, should we check extraBuffer first?

Make sense and we can decide quicker the state of the playback.

…, extraBuffer is compared first

brandonocasey · 2019-09-17T20:05:02Z

src/js/tech/html5.js

+    this.on(['stalled', 'suspend'], (e) => {
+      const buffered = this.buffered();
+
+      if (buffered.length > 0) {


we should do

if (!bufferred.length) { return; }

to eliminate the scope here.

…ave any buffer region.

welcome · 2019-10-04T17:31:43Z

Congrats on merging your first pull request! 🎉🎉🎉

… buffer is available (#6199)" This reverts commit c791cd8. Fixes #6330

… buffer is available (#6199)" (#6373) This reverts commit c791cd8. Fixes #6330

Validation for stalled and suspended event in iOS browser when headph…

8ea8e3c

…ones are disconnected

misteroneill requested changes Aug 27, 2019

View reviewed changes

Safe Time Delta commented and time fudge factor along with function b…

dcbd7b9

…ody inside event funcion

misteroneill previously approved these changes Aug 28, 2019

View reviewed changes

gkatsev changed the title ~~Validation for stalled and suspended event in iOS when HP disconnected~~ fix: validation for stalled and suspended event in iOS when headphones disconnect Aug 30, 2019

marcodeltorob added 2 commits August 30, 2019 14:33

using tech methods instead elements and buffered variable

9a78ffc

Establish if we have enough extra buffer in the current time range

10761f0

misteroneill reviewed Sep 9, 2019

View reviewed changes

Break in for when we have extra buffer and fix in the conditional

bea6615

gkatsev requested changes Sep 9, 2019

View reviewed changes

current Time comparition with end and start buffer time range using …

288fc4a

…, extraBuffer is compared first

brandonocasey reviewed Sep 17, 2019

View reviewed changes

Modified buffered.length comparition to return in case if we do not h…

684c4ae

…ave any buffer region.

brandonocasey approved these changes Sep 19, 2019

View reviewed changes

gkatsev approved these changes Oct 4, 2019

View reviewed changes

gkatsev merged commit c791cd8 into videojs:master Oct 4, 2019

klipstein mentioned this pull request Dec 12, 2019

Apply the IOS headphone disconnect patch only after successful playback #6354

Closed

2 tasks

gkatsev added a commit that referenced this pull request Dec 23, 2019

revert: Revert "fix(iOS): pause player on suspend or stalled if extra…

92acc56

… buffer is available (#6199)" This reverts commit c791cd8. Fixes #6330

gkatsev added a commit that referenced this pull request Dec 24, 2019

revert: Revert "fix(iOS): pause player on suspend or stalled if extra…

17d8046

… buffer is available (#6199)" (#6373) This reverts commit c791cd8. Fixes #6330

This was referenced Mar 19, 2020

[Snyk] Upgrade video.js from 7.4.1 to 7.7.5 videojs/videojs-playlist#173

Closed

[Snyk] Upgrade video.js from 7.6.6 to 7.7.5 videojs/videojs-errors#192

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: validation for stalled and suspended event in iOS when headphones disconnect #6199

fix: validation for stalled and suspended event in iOS when headphones disconnect #6199

marcodeltorob commented Aug 26, 2019

welcome bot commented Aug 26, 2019

misteroneill Aug 27, 2019

misteroneill Aug 27, 2019

brandonocasey Aug 29, 2019

gkatsev Aug 29, 2019

misteroneill Aug 30, 2019

marcodeltorob Aug 30, 2019

gkatsev Aug 30, 2019

misteroneill Sep 9, 2019

gkatsev Sep 9, 2019

gkatsev Sep 9, 2019

gkatsev Sep 9, 2019

marcodeltorob Sep 9, 2019

brandonocasey Sep 17, 2019

welcome bot commented Oct 4, 2019

	if ((!this.paused() && window.navigator.onLine) && extraBuffer) {
	if (!this.paused() && window.navigator.onLine && extraBuffer) {

fix: validation for stalled and suspended event in iOS when headphones disconnect #6199

fix: validation for stalled and suspended event in iOS when headphones disconnect #6199

Conversation

marcodeltorob commented Aug 26, 2019

welcome bot commented Aug 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

welcome bot commented Oct 4, 2019