refactor: Unify audioOnly mode and audioPoster mode #7678
Conversation
Codecov Report
@@ Coverage Diff @@
## main #7678 +/- ##
==========================================
+ Coverage 80.74% 80.83% +0.08%
==========================================
Files 116 116
Lines 7418 7447 +29
Branches 1794 1803 +9
==========================================
+ Hits 5990 6020 +30
+ Misses 1428 1427 -1
Continue to review full report at Codecov.
|
| @@ -4366,6 +4368,8 @@ class Player extends Component { | |||
| return this.audioOnlyMode_; | |||
| } | |||
|
|
|||
| this.audioOnlyMode_ = value; | |||
|
|
|||
There was a problem hiding this comment.
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
Co-authored-by: Alex Barstow <alexander.barstow@gmail.com>
misteroneill
changed the title
| @@ -29,6 +29,20 @@ 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'], () => { | |||
| const isSourceAudio = player.currentType().substring(0, 5) === 'audio'; | |||
There was a problem hiding this comment.
This wouldn't work for audio-only HLS
There was a problem hiding this comment.
It's not meant to, but perhaps that's worth adding a comment on.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks @harisha-swaminathan!
Co-authored-by: Alex Barstow <alexander.barstow@gmail.com>
Modify the
audioPosterModemethod to return a promise and unifyaudioOnlyModeandaudioPosterModeby turning off one mode when the other mode is turned on.This change also disables/hides PIP for an audio source and when either of the two audio modes are true