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

fix(seekbar): don't disable if live tracker's seekable is infinity #5721

Merged
merged 2 commits into from
Jan 8, 2019

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Jan 2, 2019

The seekbar is disabled if the seekable end is Infinity and re-enabled
when it isn't. Instead, we should only disable if seekable end is
Infinity and re-enable only if we disabled for the seekable end.

This auto disabling and re-enabling, without this fix, caused issues
with disabling the progress control since the seekbar got re-enabled.

The seekbar is disabled if the seekable end is Infinity and re-enabled
when it isn't. Instead, we should only disable if seekable end is
Infinity and re-enable only if we disabled for the seekable end.

This auto disabling and re-enabling, without this fix, caused issues
with disabling the progress control since the seekbar got re-enabled.
@gkatsev gkatsev added this to the 7.4.x milestone Jan 3, 2019
@gkatsev
Copy link
Member Author

gkatsev commented Jan 3, 2019

So, spoke with @ldayananda offline.
The issue is that the livetracker affects the seekbar whether we are playing back a live stream or not. Also, this happened every 30ms or so, meaning that if a user tried to disable the progress control, this would potentially come back in and re-enable it. In particular, live tracker defaults seekable end to be Infinity, a reasonable assumption, however, before we end up loading the video, we think that we're at seekable end of infinity and disable the seekbar, which is why just one flag for disableForLive_ wasn't enough. So, here are some potential directions:

  • keep track of what the previous state was and then go back to that state instead of always re-enabling. This likely still needs to keep track of disableForLive_ for us to be able to distinguish between a "user-generated" disable or a live tracker disable.
  • do what we have now and ignore any live tracker information until video metadata has loaded
  • ignore live tracker info if we're not in a live video by checking the duration
  • some combination of the above but only on android
  • move the disable/enable logic out of update_ and into a durationchange handler. Then specifically target live on android. This means that disable/enable happens once (as we'll get another durationchange if the duration goes from Infinity back to finite number) and a user could call progressControl.disable() and we won't disable/enable it from under them.

@gkatsev gkatsev added patch This PR can be added to a patch release. needs: updates labels Jan 3, 2019
@gkatsev
Copy link
Member Author

gkatsev commented Jan 4, 2019

Another option is to just remove this block as well.

@gkatsev
Copy link
Member Author

gkatsev commented Jan 8, 2019

So, we've decided to just remove the code in question so it doesn't cause any potential issues. We recommend users on Android overrideNative to use our MSE-based playback engine.

@gkatsev gkatsev added needs: LGTM Needs one or more additional approvals and removed needs: updates labels Jan 8, 2019
@misteroneill misteroneill removed the needs: LGTM Needs one or more additional approvals label Jan 8, 2019
@gkatsev gkatsev changed the title fix(seekbar): don't disable if readyState is 0 fix(seekbar): don't disable if live tracker's seekable is infinity Jan 8, 2019
@gkatsev gkatsev merged commit f02fb1b into master Jan 8, 2019
@gkatsev gkatsev deleted the progress-control-disable branch January 8, 2019 19:13
gkatsev added a commit that referenced this pull request Jan 8, 2019
…5721)

This was done to make the behavior on Android with HLS live streams better but the it's opening us up to too many potential issues, like a user not being able to properly disable the control bar, that we should just back it out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants