-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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 mediasession support #8309
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8309 +/- ##
==========================================
- Coverage 82.72% 82.69% -0.04%
==========================================
Files 113 113
Lines 7594 7563 -31
Branches 1827 1826 -1
==========================================
- Hits 6282 6254 -28
+ Misses 1312 1309 -3 ☔ View full report in Codecov by Sentry. |
position: 5 | ||
}), 'setPositionState called'); | ||
|
||
this.player.trigger('paused'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Referring to my comment above:
Shouldn't it be
pause
instead ofpaused
?
}; | ||
|
||
const updatePositionState = () => { | ||
const dur = parseFloat(this.duration()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question, is there a case that requires a parseFloat
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That can probably go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return; | ||
} | ||
const ms = window.navigator.mediaSession; | ||
const defaultSkipTime = 15; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question, do you think it would be a good idea to use skipButtons
values if they are available? This would allow a unified experience between the player
and the mediaSession
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about keeping this property as it is for now and adding this modification via another PR? Because it would be nice to have the titleBar
values as well.
@mister-ben everything works well, except for the minor issue related to the I found a small issue in videojs-playlist that prevents the artwork to be set, this means that when the next media is played, the poster is not displayed. |
Co-authored-by: André <34163393+amtins@users.noreply.github.com>
Thanks for reviewing! I think there may be a PR that would fix that issue, need to check. |
Description
Adds built-in, opt-in support for populating mediasession metadata and setting action handlers
Specific Changes proposed
mediaSession
player setup option. Since implementors may be using the API, not on by default. Possibly this could be made default in a major version.mediaSession.skipTime
(seconds) will override the default skip interval (if the browser/os doesn't request a time)getMedia()
(ifloadMedia()
has been used) or from playlist metadata.updatemetadata
with the resolved metadata object as its second argument. The metadata can be modified synchronously if needed, i.e. there is another source of truth for metadata.Requirements Checklist