-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add mediaPlayer.addTextTrack()
#3573
base: development
Are you sure you want to change the base?
Add mediaPlayer.addTextTrack()
#3573
Conversation
This method allows for loading captions from a file not listed in the manifest.
Needed for MediaPlayer.addTextTrack()
4e0c4a0
to
b32c526
Compare
src/streaming/CaptionsLoader.js
Outdated
if (data.indexOf('WEBVTT') > -1) { | ||
return VTTParser(context).getInstance(); | ||
} | ||
// } else if (data.indexOf('MPD') > -1 || data.indexOf('Patch') > -1) { |
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.
Remove comments
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.
Done in 97006db
src/streaming/MediaPlayer.js
Outdated
captionsLoader.reset(); | ||
}; | ||
|
||
eventBus.on(Events.EXTERNAL_CAPTIONS_LOADED, handler, self); |
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.
Can this event be captured by the TextSourceBuffer instead and cause createTextTrackFromMediaInfo() to be called? Instead of duplicating the code?
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.
You are totally right, I think the current structure is an artifact of how I wrote the change initially.
Change done in 39773bb
} | ||
|
||
/********* END ***********/ | ||
let captionsLoader = createCaptionsLoader(); |
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.
Do we need to create a new instance of CaptionsLoader every time? Might consider creating a single instance as part of TextController and then call textController.getCaptionsLoader
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 you're right, but I'm not sure how to do it. Currently the CaptionsLoader
inherits requestModifier
and mssHandler
from the MediaPlayer
config. Those don't seem available from inside TextController
, so I'm not sure how pass those along if it's created in TextController.setup()
.
Move `EXTERNAL_CAPTIONS_LOADED`-event listener into TextSourceBuffer and to allow reusing `createTextTrackFromMediaInfo`. This requires passing `mediaInfo` to CaptionsLoader and adding the passed MediaInfo to `TextSourceBuffer.mediaInfos`. Also removes `TextTracks.getNumberOfTextTracks()` which is no longer needed.
@malthejorgensen Sorry for the delay on my end. We are currently working on version 4.0 including major changes to the general structure of dash.js. Once this is done I will get back to your PR |
Will this be merged? This is really useful when the backend doesn't support adding tracks in the manifest. :) |
2 years later.. any update on this? I'm now creating a HTMLTrack element as workaround. Thanks. |
This is a proof-of-concept for allowing adding captions/subtitles that are not listed in the manifest.
See issue: #3572