Media Session API: Add artwork #162
Conversation
Release 1.0.0-beta
Fix when the deploy-production action runs
Release 1.0.0-beta2
Release 1.0.0-beta3
Release 1.0.0-beta4
src/js/utils/generateCache.js
Outdated
| @@ -69,6 +69,9 @@ export default function generateCache() { | |||
| } else if (Object.prototype.toString.call(video.thumbnail) === '[object String]') { | |||
| assetsToCache.push(video.thumbnail); | |||
| } | |||
| if (Array.isArray(video['media-session-artwork'])) { | |||
There was a problem hiding this comment.
Is this needed to cache media session artwork images if the Media Session API is not supported?
There was a problem hiding this comment.
@beaufortfrancois I was wondering if we even want to cache those files. It's quite a lot of data – 400 kB currently – and the benefit of the artwork being available in the offline mode is marginal IMO. We can always fall back to the default artwork in those cases.
And if you're asking whether we should exclude browsers with no Media Session API support, then I'd say perhaps. With Safari 15 on iOS out the door, there is currently no mainstream evergreen browser that would not support it.
What is your opinion on the caching?
There was a problem hiding this comment.
What is your opinion on the caching?
Is it possible to cache them only when media session metadata are set?
In other words, only when the media starts playing.
There was a problem hiding this comment.
I think adding logic to the SW to cache this artwork when it's played, or dowloaded for offline playback, would be a better user experience instead of caching all the artwork during initialization.
There was a problem hiding this comment.
Good idea, guys, thank you. I've implemented the changes in d7ac820
The artwork is now cached when a video is played or downloaded.
@beaufortfrancois @derekherman Can you please take another look?
There was a problem hiding this comment.
QQ: Is this caching assets every time video is played and/or downloaded or only once?
There was a problem hiding this comment.
@beaufortfrancois Aha, good point, I did not realize cache.addAll would overwrite the cache entries. Updated the saveToCache implementation in 64ca172. Could you please take a look?
I believe it is sufficient to match the URL, because we're versioning cache. LMK if you think this needs to be more robust. Thanks!
| @@ -9,6 +9,26 @@ video-sources: | |||
| - src: https://storage.googleapis.com/kino-assets/single-video/video.mp4 | |||
| type: video/mp4; codecs="avc1.640032,mp4a.40.2" | |||
| thumbnail: https://storage.googleapis.com/kino-assets/single-video/thumbnail.png | |||
| media-session-artwork: | |||
There was a problem hiding this comment.
Do we already have linter warnings so that we don't forget to add media session artwork to future .md files?
There was a problem hiding this comment.
I can see how that would be useful, but I don't believe I've ever linted front-matter blocks in markdown files. What tool would you recommend for the task?
There was a problem hiding this comment.
https://www.npmjs.com/package/remark-frontmatter maybe. We use it for web.dev. See https://github.com/GoogleChrome/web.dev/search?q=remark-frontmatter
There was a problem hiding this comment.
I think adding a markdown linter is a great idea. However, it feels a bit out of scope for this PR.
There was a problem hiding this comment.
Noted. Do you mind filing a feature request to keep track of it?
| @@ -9,6 +9,26 @@ video-sources: | |||
| - src: https://storage.googleapis.com/kino-assets/single-video/video.mp4 | |||
| type: video/mp4; codecs="avc1.640032,mp4a.40.2" | |||
| thumbnail: https://storage.googleapis.com/kino-assets/single-video/thumbnail.png | |||
| media-session-artwork: | |||
There was a problem hiding this comment.
Noted. Do you mind filing a feature request to keep track of it?
| * @returns {string[]} URLs. | ||
| */ | ||
| getMediaSessionArtworkUrls() { | ||
| const artworkObjects = this.internal.videoData['media-session-artwork'] || []; |
There was a problem hiding this comment.
Shouldn't we have default artwork URL there as a fallback or is it safe to assume MEDIA_SESSION_DEFAULT_ARTWORK will always be in the cache?
There was a problem hiding this comment.
@beaufortfrancois You're right, we should have the default artwork pulled in here. Updated the implementation in ab8c104.
|
Tested against https://kinoweb-dev--staging-2ra3ji0i.web.app/
|
Summary
When
mediaSession.metadatais set, the app now uses artwork specific to the currently playing video. The video specific artwork is also cached by the SW.Fixes #161