Skip to content

Conversation

@StaehliJ
Copy link
Contributor

@StaehliJ StaehliJ commented Aug 14, 2024

Pull request

Description

The goal of this PR is to improve QoSCoordinator.

Changes made

  • Remove QoSEventDispatcher as it does nothing special.
  • MetrcisCollector collect durations when an error occurs during loading.
  • Remove QoS stall event.

Checklist

  • Your branch has been rebased onto the main branch.
  • APIs have been properly documented (if relevant).
  • The documentation has been updated (if relevant).
  • New unit tests have been written (if relevant).
  • The demo has been updated (if relevant).
  • All pull request status checks pass.

@StaehliJ StaehliJ changed the base branch from main to qos August 14, 2024 13:11
@StaehliJ StaehliJ force-pushed the improve-qos-coordinator branch from edec2a0 to 97338ca Compare August 14, 2024 13:12
@StaehliJ StaehliJ force-pushed the improve-qos-coordinator branch from 97338ca to f553433 Compare August 14, 2024 13:12
@github-actions
Copy link

github-actions bot commented Aug 14, 2024

Code Coverage

Overall Project 53.3% -0.2% 🟢
Files changed 87.68% 🟢

Module Coverage
:pillarbox-player 60.4% -0.37% 🟢
Files
Module File Coverage
:pillarbox-player QoSCoordinator.kt 90.79% -9.03% 🟢
PillarboxExoPlayer.kt 82.73% 🟢
MetricsCollector.kt 68.7% -0.16% 🟢

@MGaetan89 MGaetan89 added this to the QoS milestone Aug 15, 2024
@StaehliJ StaehliJ requested a review from MGaetan89 August 15, 2024 15:00
@StaehliJ
Copy link
Contributor Author

The player position of the stop event after a item transition is wrong. The player position is the position of the next item (near 0ms).

Copy link
Contributor

@MGaetan89 MGaetan89 left a comment

Choose a reason for hiding this comment

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

Just a small question, otherwise good for me

Comment on lines 95 to 97
override fun onSessionFinished(session: PlaybackSessionManager.Session) {
// Already handled by onMetricSessionFinished
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why override it, then, since it has a default implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I like to write a lot of things, and to keep in mind that it is handled elsewhere. But I agree we could remove that.

@StaehliJ StaehliJ merged commit 91d1f28 into qos Aug 19, 2024
@StaehliJ StaehliJ deleted the improve-qos-coordinator branch August 19, 2024 11:48
MGaetan89 added a commit that referenced this pull request Aug 20, 2024
Co-authored-by: Gaëtan Muller <m.gaetan89@gmail.com>
StaehliJ added a commit that referenced this pull request Aug 28, 2024
Co-authored-by: Gaëtan Muller <m.gaetan89@gmail.com>
StaehliJ added a commit that referenced this pull request Sep 11, 2024
Co-authored-by: Gaëtan Muller <m.gaetan89@gmail.com>
MGaetan89 added a commit that referenced this pull request Sep 12, 2024
Co-authored-by: Gaëtan Muller <m.gaetan89@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants