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.

Sign up for GitHub

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

Jump to bottom

refactor: Unify audioOnly mode and audioPoster mode #7678

Merged
merged 9 commits into from
Mar 17, 2022
Merged

refactor: Unify audioOnly mode and audioPoster mode #7678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a977284
unify audioOnlyMode and audioPosterMode
harisha-swaminathan Mar 11, 2022
f057c4d
remove console log statement
harisha-swaminathan Mar 11, 2022
c151709
rewrite audioPoterMode method and add tests
harisha-swaminathan Mar 14, 2022
f1c279b
add tests
harisha-swaminathan Mar 14, 2022
6331f2d
update tests, add comments and replace currentSource with currentType
harisha-swaminathan Mar 15, 2022
2f17c4a
Fix typo in comment
harisha-swaminathan Mar 15, 2022
b7e4e4e
Merge branch 'videojs:main' into feat/audioOnlyModes
harisha-swaminathan Mar 15, 2022
6edb802
fix assert.equal/assert.ok in tests
harisha-swaminathan Mar 15, 2022
1cecdfe
add comment for audio detection
harisha-swaminathan Mar 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/js/control-bar/picture-in-picture-toggle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,21 @@ class PictureInPictureToggle extends Button {
this.on(player, ['enterpictureinpicture', 'leavepictureinpicture'], (e) => this.handlePictureInPictureChange(e));
this.on(player, ['disablepictureinpicturechanged', 'loadedmetadata'], (e) => this.handlePictureInPictureEnabledChange(e));

this.on(player, ['loadedmetadata', 'audioonlymodechange', 'audiopostermodechange'], () => {
// This audio detection will not detect HLS or DASH audio-only streams because there was no reliable way to detect them at the time
const isSourceAudio = player.currentType().substring(0, 5) === 'audio';
Copy link
Contributor

Choose a reason for hiding this comment

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

This wouldn't work for audio-only HLS

Copy link
Member

Choose a reason for hiding this comment

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

It's not meant to, but perhaps that's worth adding a comment on.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure whether anyone actually uses it, but audio/mpegurl is a valid type for HLS. I believe it's valid for HLS even if it contains video.

Copy link
Member

Choose a reason for hiding this comment

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

the-more-you-know.gif

Yeah, I should be more clear here: the intent is merely to detect cases where we know the media is audio. For HLS or DASH audio-only streams, they may have a blank video track or no video track. Either way, VHS currently does not have a way to detect whether a stream is audio only.

So, we decided that the best course of action for the time being is to do the simple detection we can do.

Copy link
Contributor

Choose a reason for hiding this comment

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

In theory videoHeight() would be enough to know there's no video - but I can see getting into a situation where there seems to be no video initially when there eventually will be. Fair enough if it doesn't cover all cases, but comment/doc would be good so that's clearer down the line.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @harisha-swaminathan!


if (isSourceAudio || player.audioPosterMode() || player.audioOnlyMode()) {
if (player.isInPictureInPicture()) {
player.exitPictureInPicture();
}
this.hide();
} else {
this.show();
}

});

// TODO: Deactivate button on player emptied event.
this.disable();
}
Expand Down
97 changes: 77 additions & 20 deletions src/js/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,9 @@ class Player extends Component {
// Init state audioOnlyMode_
this.audioOnlyMode_ = false;

// Init state audioPosterMode_
this.audioPosterMode_ = false;

// Init state audioOnlyCache_
this.audioOnlyCache_ = {
playerHeight: null,
Expand Down Expand Up @@ -583,7 +586,15 @@ class Player extends Component {

this.breakpoints(this.options_.breakpoints);
this.responsive(this.options_.responsive);
this.audioOnlyMode(this.options_.audioOnlyMode);

// Calling both the audio mode methods after the player is fully
// setup to be able to listen to the events triggered by them
this.on('ready', () => {
// Calling the audioPosterMode method first so that
// the audioOnlyMode can take precedence when both options are set to true
this.audioPosterMode(this.options_.audioPosterMode);
this.audioOnlyMode(this.options_.audioOnlyMode);
});
}

/**
Expand Down Expand Up @@ -4303,11 +4314,6 @@ class Player extends Component {
return !!this.isAudio_;
}

updateAudioOnlyModeState_(value) {
this.audioOnlyMode_ = value;
this.trigger('audioonlymodechange');
}

enableAudioOnlyUI_() {
// Update styling immediately to show the control bar so we can get its height
this.addClass('vjs-audio-only-mode');
Expand All @@ -4334,7 +4340,7 @@ class Player extends Component {

// Set the player height the same as the control bar
this.height(controlBarHeight);
this.updateAudioOnlyModeState_(true);
this.trigger('audioonlymodechange');
}

disableAudioOnlyUI_() {
Expand All @@ -4345,7 +4351,7 @@ class Player extends Component {

// Reset player height
this.height(this.audioOnlyCache_.playerHeight);
this.updateAudioOnlyModeState_(false);
this.trigger('audioonlymodechange');
}

/**
Expand All @@ -4366,6 +4372,8 @@ class Player extends Component {
return this.audioOnlyMode_;
}

this.audioOnlyMode_ = value;

Copy link
Contributor Author

@harisha-swaminathan harisha-swaminathan Mar 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Moved this here because this.audioOnlyMode_ was only getting updated after the promise was resolved and so outdated values were retuned when audioOnlyMode() was called in the audioPosterMode method

const PromiseClass = this.options_.Promise || window.Promise;

if (PromiseClass) {
Expand All @@ -4382,6 +4390,10 @@ class Player extends Component {
exitPromises.push(this.exitFullscreen());
}

if (this.audioPosterMode()) {
exitPromises.push(this.audioPosterMode(false));
}

return PromiseClass.all(exitPromises).then(() => this.enableAudioOnlyUI_());
}

Expand All @@ -4404,35 +4416,80 @@ class Player extends Component {
}
}

enablePosterModeUI_() {
// Hide the video element and show the poster image to enable posterModeUI
const tech = this.tech_ && this.tech_;

tech.hide();
this.addClass('vjs-audio-poster-mode');
this.trigger('audiopostermodechange');
}

disablePosterModeUI_() {
// Show the video element and hide the poster image to disable posterModeUI
const tech = this.tech_ && this.tech_;

tech.show();
this.removeClass('vjs-audio-poster-mode');
this.trigger('audiopostermodechange');
}

/**
* Get the current audioPosterMode state or set audioPosterMode to true or false
*
* @param {boolean} [value]
* The value to set audioPosterMode to.
*
* @return {boolean}
* True if audioPosterMode is on, false otherwise.
* @return {Promise|boolean}
* A Promise is returned when setting the state, and a boolean when getting
* the present state
*/
audioPosterMode(value) {

if (this.audioPosterMode_ === undefined) {
this.audioPosterMode_ = this.options_.audioPosterMode;
}

if (typeof value !== 'boolean' || value === this.audioPosterMode_) {
return this.audioPosterMode_;
}

this.audioPosterMode_ = value;

if (this.audioPosterMode_) {
this.tech_.hide();
this.addClass('vjs-audio-poster-mode');
const PromiseClass = this.options_.Promise || window.Promise;

if (PromiseClass) {

if (value) {

if (this.audioOnlyMode()) {
const audioOnlyModePromise = this.audioOnlyMode(false);

return audioOnlyModePromise.then(() => {
// enable audio poster mode after audio only mode is disabled
this.enablePosterModeUI_();
});
}

return PromiseClass.resolve().then(() => {
// enable audio poster mode
this.enablePosterModeUI_();
});
}

return PromiseClass.resolve().then(() => {
// disable audio poster mode
this.disablePosterModeUI_();
});
}

if (value) {

if (this.audioOnlyMode()) {
this.audioOnlyMode(false);
}

this.enablePosterModeUI_();
return;
}
// Show the video element and hide the poster image if audioPosterMode is set to false
this.tech_.show();
this.removeClass('vjs-audio-poster-mode');

this.disablePosterModeUI_();
}

/**
Expand Down
17 changes: 17 additions & 0 deletions test/unit/controls.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,23 @@ QUnit.test('Picture-in-Picture control enabled property value should be correct
pictureInPictureToggle.dispose();
});

QUnit.test('Picture-in-Picture control is hidden when the source is audio', function(assert) {
const player = TestHelpers.makePlayer({});
const pictureInPictureToggle = new PictureInPictureToggle(player);

player.src({src: 'example.mp4', type: 'video/mp4'});
player.trigger('loadedmetadata');

assert.notOk(pictureInPictureToggle.hasClass('vjs-hidden'), 'pictureInPictureToggle button is not hidden initially');

player.src({src: 'example1.mp3', type: 'audio/mp3'});
player.trigger('loadedmetadata');
assert.ok(pictureInPictureToggle.hasClass('vjs-hidden'), 'pictureInPictureToggle button is hidden whenh the source is audio');

player.dispose();
pictureInPictureToggle.dispose();
});

QUnit.test('Fullscreen control text should be correct when fullscreenchange is triggered', function(assert) {
const player = TestHelpers.makePlayer({controlBar: false});
const fullscreentoggle = new FullscreenToggle(player);
Expand Down
Loading