-
Notifications
You must be signed in to change notification settings - Fork 27
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
Fix for Shaka 2.2.x to be able to switch languages #43
base: master
Are you sure you want to change the base?
Changes from all commits
1294859
0bf13de
5197929
0703004
ca9d2d4
a10001a
726da91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
# editorconfig.org | ||
|
||
# top-most EditorConfig file | ||
root = true | ||
|
||
[*] | ||
charset = utf-8 | ||
end_of_line = lf | ||
insert_final_newline = true | ||
|
||
indent_style = space | ||
indent_size = 2 | ||
|
||
trim_trailing_whitespace = true | ||
|
||
[*.md] | ||
trim_trailing_whitespace = false | ||
|
||
[{package.json}] | ||
indent_style = space | ||
indent_size = 2 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,7 @@ class DashShakaPlayback extends HTML5Video { | |
this._levels = [] | ||
this._pendingAdaptationEvent = false | ||
this._isShakaReadyState = false | ||
this._activeAudioLanguage = null | ||
|
||
options.autoPlay && this.play() | ||
} | ||
|
@@ -124,6 +125,15 @@ class DashShakaPlayback extends HTML5Video { | |
} | ||
} | ||
|
||
/** | ||
* Determine if the playback does not contain video/has video but video should be ignored. | ||
* @property isAudioOnly | ||
* @type Boolean | ||
*/ | ||
get isAudioOnly() { | ||
return this.isReady ? this._player.isAudioOnly() : false | ||
} | ||
|
||
get textTracks () { | ||
return this.isReady && this._player.getTextTracks() | ||
} | ||
|
@@ -140,21 +150,44 @@ class DashShakaPlayback extends HTML5Video { | |
return (this.isReady && this._player.isLive() ? 'live' : 'vod') || '' | ||
} | ||
|
||
selectTrack (track) { | ||
if (track.type === 'text') { | ||
this._player.selectTextTrack(track) | ||
} else if (track.type === 'variant') { | ||
this._player.selectVariantTrack(track) | ||
if (track.mimeType.startsWith('video/')) { | ||
// we trigger the adaptation event here | ||
// because Shaka doesn't trigger its event on "manual" selection. | ||
this._onAdaptation() | ||
} | ||
} else { | ||
throw new Error('Unhandled track type:', track.type); | ||
selectTrack (track, clearBuffer) { | ||
if (!this.isReady) { | ||
return false | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we check if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice catch! didn't test selecting variant tracks here again. thanks!! my next aim on this codebase is to add some unit tests :) |
||
switch(track.type) { | ||
case 'text': | ||
this._player.selectTextTrack(track) | ||
return true | ||
case 'variant': | ||
this._player.selectVariantTrack(track, clearBuffer) | ||
if (track.mimeType.startsWith('video/')) { | ||
// we trigger the adaptation event here | ||
// because Shaka doesn't trigger its event on "manual" selection. | ||
this._onAdaptation() | ||
} | ||
return true | ||
default: | ||
throw new Error('Unhandled track type:', track.type) | ||
} | ||
} | ||
|
||
selectLanguage(language, role) { | ||
if (this.isReady) { | ||
this._activeAudioLanguage = language | ||
this._player.selectAudioLanguage(language, role) | ||
return true | ||
} | ||
return false | ||
} | ||
|
||
get audioLanguages() { | ||
return this.isReady && this._player.getAudioLanguages() | ||
} | ||
|
||
get activeAudioLanguage() { | ||
return this._activeAudioLanguage || this.audioLanguages[0] || null | ||
} | ||
|
||
/** | ||
* @override | ||
*/ | ||
|
@@ -261,6 +294,9 @@ class DashShakaPlayback extends HTML5Video { | |
this._options.shakaConfiguration && this._player.configure(this._options.shakaConfiguration) | ||
this._options.shakaOnBeforeLoad && this._options.shakaOnBeforeLoad(this._player) | ||
|
||
const preferredAudioLanguage = this._player.getConfiguration().preferredAudioLanguage | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this something that can be overridden via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just by now we need to initialize the state of otherwise me start out with saying language A is active because it's first in the list, but actually language B is because it has been configured as preferred. not sure i got the question right, but does that answer your question actually? :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so, this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. exactly |
||
this._activeAudioLanguage = preferredAudioLanguage.length ? preferredAudioLanguage : null | ||
|
||
let playerLoaded = this._player.load(this._options.src) | ||
playerLoaded.then(() => this._loaded()) | ||
.catch((e) => this._setupError(e)) | ||
|
@@ -281,7 +317,7 @@ class DashShakaPlayback extends HTML5Video { | |
} | ||
|
||
_loaded () { | ||
this._onShakaReady(); | ||
this._onShakaReady() | ||
this._startToSendStats() | ||
this._fillLevels() | ||
this._checkForClosedCaptions() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see too much value by changing an
if else if else
by a switch, there was another reason for this refactoring?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm well no there is no change in logic.
from a code-style pov i would say that a switch is preferrable here since the condition is solely around the value of a single variable and no other conditions, and there are more than two cases :) so perfect time for a "switch"
so my answer is: this shouldn't have been an if-else in the first place. although i had written this code right there, just correcting myself now ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I see your points :) Later I'll try to break this single class into at least 4 different components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's still quite maintainable like this.
we should however add tests + enforce JSdocs with the eslint plugin for that on each method :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, tests are way up on the priority list. 🥂