Skip to content

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

}
});

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