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

Handle correctly seek events while paused #21

Merged
merged 2 commits into from
Jul 24, 2023

Conversation

oliviermartin08
Copy link
Contributor

📖 Description & Context

On iOS, a fix has been recently done to the video tracker in order to improve the seek events. What was done in #20:

  • Send CONTENT_SEEK_START and CONTENT_SEEK_END events.
  • Send CONTENT_BUFFER_START and CONTENT_BUFFER_END with their bufferType = seek rather than connection when seek occurs.

⚠️ However, this fix did not cover correctly when a user made a seek during the pause state.

  • The CONTENT_SEEK_END event was only sent once the playback was resumed. That behavior was corrupting the timeSinceSeekBegin timer.
  • Also, when paused, the CONTENT_BUFFER_START and CONTENT_BUFFER_END events were simply not sent, while it's still not possible to directly resume playback for a certain buffer time.

👷 Fix

Modified the state isSeekingDuringPlayback for isUserSeeking. With this solution, the lib integrators are now 100% handling when a seek event starts (on a user seek interaction). Then, with the KVO observers, the tracker is handling by itself when the seek ends as well as its buffer. So, right now:

  • CONTENT_SEEK_START and CONTENT_SEEK_END events are sent when paused (without the need to restart the playback).
  • CONTENT_BUFFER_START and CONTENT_BUFFER_END events are sent when the player stays in a pause state. The CONTENT_BUFFER_END is sent when it's possible for the user to resume playback without any loading time (which is when the observer playbackLikelyToKeepUp changes to true during the pause state while seeking).

📓 Notes about this solution

  • I first tried to handle manually when seek started AND when seek ended on the integrator side, but if the CONTENT_SEEK_END was not handled by the tracker on par with the buffer events, it was leading to certain race conditions and unwanted behaviors.
  • When a user spams the seek button many times, it may request some subsequent seek actions while a previous seek buffer is not completed. In that case, these subsequent seeks will be part of the latest SEND_SEEK_START, SEND_SEEK_END and its buffer start/end.

@oliviermartin08
Copy link
Contributor Author

For the question of Charles on the previous PR here: #20 (comment)

I thought we wouldn't need this condition and would simply rely on isSeekingDuringPlayback changes to call [self sendSeekStart];. Is making isSeekingDuringPlayback KVO compatible a solution that could work?

No, it doesn't really fix it for the case where a user spams the seek button very quickly as mentioned in this PR Notes. ☝️ Basically, I think that we still need an internal state inside the tracker to provide sending multiple seek events when the player is already seeking (goSeekStart function purpose). This basically regroups multiple seek interactions inside a single seek event when the buffering occurs in the same time frame.

Copy link
Contributor

@alarochelle alarochelle left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@miransar
Copy link
Contributor

miransar commented Jul 20, 2023

@oliviermartin08 PR looks good. As this is a major change due to function names changing, please bump up the version to 2.0.0 here. Thanks.

@oliviermartin08
Copy link
Contributor Author

Bumped version to 2.0.0, thanks @miransar 🙏

@miransar miransar merged commit b9aae80 into newrelic:master Jul 24, 2023
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.

7 participants