-
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
Feature/imsc js #1693
Feature/imsc js #1693
Conversation
…controller, repair abandon rule, rich buffer rule.
Currently if the end user provides any protection data for the Key System our library no longer retrieves the license server URL from the initData. The above combines with the fact that the ProtectionKeyController skips Key Systems without protection data if protection data is provided for any key system in a bad way. The end result is that if you want to provide some data for one key system you can't get the serverUrl from the initData for a different key system. This ensures that even if protection data was passed, if no serverURL or laURL is provided we try to get the url from the initData.
src/streaming/TextTracks.js
Outdated
cue.cueID = currentItem.cueID; | ||
cue.videoWidth = currentItem.videoWidth; | ||
cue.videoHeight = currentItem.videoHeight; | ||
cue.cellResolution = currentItem.cellResolution; |
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.
cellResolution is used at https://github.com/Dash-Industry-Forum/dash.js/blob/development/src/streaming/TextTracks.js#L295 when the video element is resized. Now this atttibute has been removed, that method either needs updating or, more likely, removing since it is no longer relevant. Currently, this causes an uncaught TypeError.
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.
According to my understanding, the scaleCue function needs to call each time the renderHTML function of imsc library....
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 thought that too but Tobbe implied scaling was already working in a comment on the other PR, so that's why I thought scaleCue might not be needed.
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.
ok, maybe it's better to let @TobbeEdgeware remove it.
src/streaming/TextTracks.js
Outdated
log('Cue enter id:' + this.cueID); | ||
imsc1Parser.renderHTML(this.isd, finalCue, null, captionContainer.clientWidth, captionContainer.clientHeight); | ||
finalCue.id = this.cueID; | ||
captionContainer.appendChild(finalCue); |
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.
Unfortunately, this doesn't work properly when line padding is to be applied because the parser uses getComputedStyle
to calculate the required padding, which only works when the element is attached to the DOM.
So finalCue
needs to be appended to captionContainer
before renderHTML
is called.
src/streaming/TextTracks.js
Outdated
@@ -71,6 +72,8 @@ function TextTracks() { | |||
displayCCOnTop = false; | |||
topZIndex = 2147483647; | |||
|
|||
imsc1Parser = require('imsc'); |
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.
Typically, we use the ES6 import statement for dependencies. See round10 and codem-isoboxer examples. Is there any reason why this pattern should not be used here too?
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.
no good reason indeed...
package.json
Outdated
"round10": "^1.0.3" | ||
"imsc": "^1.0.0-beta.0", | ||
"round10": "^1.0.3", | ||
"sax": "^1.2.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.
Is it necessary to add sax as a dependency here? Shouldn't imsc bring in this dependency? (Perhaps there is something I am missing ...)
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're right....thanks.
Looks like the branch needs rebasing ... |
src/streaming/utils/TTMLParser.js
Outdated
@@ -30,6 +30,8 @@ | |||
*/ | |||
import FactoryMaker from '../../core/FactoryMaker'; | |||
import Debug from '../../core/Debug'; | |||
import { fromXML } from 'imsc'; | |||
import { generateISD } from 'imsc'; |
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.
FYI, this can be refactored to import { fromXML, generateISD } from 'imsc';
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.
thanks, ...:-)
bd6bd79
to
1397894
Compare
|
@palemieux Thanks for the comments and the new feature. I'll fix the test sequences to be compliant with the more accurate implementation of IMSC-1. That should make the line padding visible on http://vm2.dashif.org/dash/vod/testpic_2s/xml_subs.mpd again. With the mechanism you have implemented in sandflow/imscJS#47, I think it should be relatively easy to get SMPTE-TT image subtitles working after including this PR. |
Ok. I will merge today. |
@nicosang @bbcrddave @wilaw @AkamaiDASH I think this is now ripe for merging. The only thing missing that we have now is support for smpte:image, but the mechanism needed for implementing it is in sandflow/imscJS#47 so we can add that once it is included in an imscJS release. What is your view? |
Yes, this looks good to me. Great work, guys! |
imscJS 1.0.0-beta.3 has been pushed to NPM. |
Sorry, I thought I just removed another obsolete branch. This should be restored. |
I'm working on restoring the imscJS-integration branch that I accidentally deleted. Once that is made, I should be able to Reopen the PR. |
I will merge this PR into imscJS-integration branch, add support for embedded SMPTE-TT image subtitles and make a new PR towards the development branch. |
start integration of the imscJS library