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

Added MediaSession support (notification controls on mobile) #187

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

R0nd
Copy link

@R0nd R0nd commented Oct 3, 2020

Implemented MediaSession support (show playing track and expose playback controls in a notification on mobile). Works great in Chrome, somewhat buggy in Firefox Nightly for now (no prev/next buttons, incorrect playback state).
image

@R0nd
Copy link
Author

R0nd commented Oct 3, 2020

One ugly hack I had to add is an <audio> tag with a 5-second silent mp3 file, because the current MediaSession API spec draft requires an html audio/video to be playing for it to work, and audio files shorter than 5 seconds don't count? I hope the final version of the spec makes more sense.

import PlayerModel from "./player_model";
import silenceMp3 from "./5-seconds-of-silence.mp3";

export default class MediaSessionController {
Copy link
Owner

Choose a reason for hiding this comment

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

Formatting does not match other files

Copy link
Author

Choose a reason for hiding this comment

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

Changed to single quotes and tab width 4

);

this.dataSource.on("player", (player) => this.updateMetadata(player));
this.dataSource.watch("player", {
Copy link
Owner

@hyperblast hyperblast Oct 4, 2020

Choose a reason for hiding this comment

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

Only one type can watch on data source, this logic should be in PlayerModel instead.

Copy link
Author

Choose a reason for hiding this comment

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

Moved to PlayerModel, but it should probably be refactored later because the activeItem's columns are becoming messy

loader: 'url-loader',
options: {
name: '[name].[ext]',
limit: 1024
}
});

config.optimization = {
Copy link
Owner

Choose a reason for hiding this comment

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

This looks unrelated to this PR, besides that I'm not sure if there is a benefit of having vendors chunk.

Copy link
Author

Choose a reason for hiding this comment

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

It's out of scope of this PR, but the added code pushed the bundle over 300KB, breaking the build

Copy link
Owner

Choose a reason for hiding this comment

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

The limit is raised in other PR, probably you can do the same.

Copy link
Author

Choose a reason for hiding this comment

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

Removed code splitting and raised bundle limit

@hyperblast
Copy link
Owner

hyperblast commented Oct 4, 2020

@R0nd,
Thank you for the contribution, the idea looks very promising.
I've left few improvement suggestions.

@hyperblast
Copy link
Owner

hyperblast commented Oct 4, 2020

One ugly hack I had to add is an <audio> tag with a 5-second silent mp3 file, because the current MediaSession API spec draft requires an html audio/video to be playing for it to work, and audio files shorter than 5 seconds don't count? I hope the final version of the spec makes more sense.

This has possible unintended effect effect of draining mobile battery for playing silence.
I think it's fine to keep this as that for now, probably makes sense to add setting to disable this feature completely.
I could help you with that.

@R0nd
Copy link
Author

R0nd commented Oct 4, 2020

I think it's fine to keep this as that for now, probably makes sense to add setting to disable this feature completely.

I've added a setting to enable this feature. I've made it disabled by default, since there's no data on performance impact yet and browser support isn't 100% there.

@hyperblast
Copy link
Owner

@R0nd sorry for delay, I'm on vacation at the moment, would merge it when I'm back, have a nice day!

@hyperblast
Copy link
Owner

hyperblast commented Oct 13, 2020

I've added a setting to enable this feature. I've made it disabled by default, since there's no data on performance impact yet and browser support isn't 100% there.

Probably it would be better to avoid <audio> completely if setting is disabled.

Brace formatting still does not match other source files: { should be on the next line for control statements and declarations, for object expressions could be on the same line. Short example of the described style.

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.

2 participants