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

feat: add listenable events for playback stall detection and gap jumping #4249

Conversation

JulianDomingo
Copy link
Contributor

An event stalldetected can be dispatched when Shaka Player detects a stall based on the value of stallThreshold through StreamingConfiguration.

A second event gapjumped could also be dispatched when Shaka performs a jump in a media gap.

Related to issue #4227.

@JulianDomingo JulianDomingo force-pushed the 4227-add-listenable-events-for-playback-stall-detection-gap-jumping branch from 018cd97 to 70ef1c5 Compare May 23, 2022 00:32
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

I don't see any breaking changes. I think you can drop the ! from the PR title, which indicates a breaking change and would push us to v5.0 on merge. It should just be feat: for a backward compatible feature.

Looks pretty good otherwise!

lib/player.js Outdated Show resolved Hide resolved
@avelad
Copy link
Member

avelad commented May 23, 2022

A good idea would be to compute in stats the number of times each of these events is launched

lib/player.js Outdated Show resolved Hide resolved
@avelad avelad added the type: enhancement New feature or request label May 23, 2022
@avelad avelad added this to the v4.1 milestone May 23, 2022
@JulianDomingo JulianDomingo changed the title feat!: add listenable events for playback stall detection and gap jumping feat: add listenable events for playback stall detection and gap jumping Jun 1, 2022
@JulianDomingo
Copy link
Contributor Author

A good idea would be to compute in stats the number of times each of these events is launched

I think this is a great idea, I've updated the Stats object to incorporate the occurrences of these events!

@JulianDomingo JulianDomingo force-pushed the 4227-add-listenable-events-for-playback-stall-detection-gap-jumping branch 2 times, most recently from a35f1d8 to f201cd0 Compare June 2, 2022 04:02
@JulianDomingo JulianDomingo marked this pull request as ready for review June 2, 2022 04:06
@avelad avelad requested review from theodab and joeyparrish June 2, 2022 06:34
Copy link
Member

@joeyparrish joeyparrish 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 overall. One small nit. Nice work!

lib/player.js Outdated Show resolved Hide resolved
Additionally:
- Occurrences of these new events will be propagated to the Stats object
- The player event names enum is moved to util/fake_event.js so classes
  outside of Player can require them.
@JulianDomingo JulianDomingo force-pushed the 4227-add-listenable-events-for-playback-stall-detection-gap-jumping branch from f201cd0 to 5675305 Compare June 2, 2022 19:12
@avelad avelad requested a review from joeyparrish June 2, 2022 19:17
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Next time, please push additional commits instead of amending. It makes PR review easier when you make updates. (It's the opposite of Gerrit.)

@joeyparrish joeyparrish merged commit 5987458 into shaka-project:main Jun 2, 2022
@JulianDomingo
Copy link
Contributor Author

Noted, thanks for the heads up!

theodab pushed a commit to theodab/shaka-player that referenced this pull request Jun 3, 2022
…ing (shaka-project#4249)

An event `stalldetected` can be dispatched when Shaka Player detects a stall based on the value of stallThreshold through [StreamingConfiguration](https://shaka-player-demo.appspot.com/docs/api/externs_shaka_player.js.html#line920).

A second event `gapjumped` could also be dispatched when Shaka performs a jump in a media gap.

Related to issue shaka-project#4227
@JulianDomingo JulianDomingo deleted the 4227-add-listenable-events-for-playback-stall-detection-gap-jumping branch June 15, 2022 23:29
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants