-
Notifications
You must be signed in to change notification settings - Fork 7
IMSC.js Styling #95
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
IMSC.js Styling #95
Conversation
package.json
Outdated
"localforage": "^1.7.1", | ||
"path-browserify": "^1.0.1", | ||
"ua-parser-js": "^1.0.2" | ||
}, | ||
"peerDependencies": { | ||
"imsc": "^1.1.4" |
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.
Why not use the bbc fork?
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.
Hi @nigelmegitt, we are using the latest BBC smp-imsc fork for this. However, it's swapped out in Bigscreen player rather than here — so I've reverted the change here to avoid it being in the bundle twice.
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.
LGTM! Left one comment on docs but I'm not going to block merging on that :)
src/core/Settings.js
Outdated
* When true, only those captions where itts:forcedDisplay="true" will be displayed. | ||
* @property {boolean} [imsc.enableRollUp=true] | ||
* Enable/disable rollUp style display of IMSC captions. | ||
* @property {number} [imsc.options.sizeAdjust=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.
What's thoughts on declaring a more generic "put your IMSC options here" type rather than calling out a few specific ones? My concern is as written this documentation suggests IMSC options mentioned are the only ones supported.
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.
Outcome of some pairing on this — we've tried to create a generic imscJS options object. However, Dash.js uses the default settings as a way of validating the settings. As a result, we opted to create a more comprehensive list of imscJS render options instead. There are a couple of options have been left out (that are not in use) that require further nested objects. If needed in the future, these could be passed via a new customiseSubtitles()
function on the media player rather than via updateSettings()
.
a9348fa
to
a162738
Compare
This PR exposes the
bbc/ImscJS
customisation options to the Dash.js settings in a way consistent with other subtitle settings already within Dash.js.bbc/ImscJS
customisation has been already been PR'd back to mainlinesandflow/imscJS
#257. However, there hasn't been any indication when this may be merged.The changes here are compatible with those proposed to mainline
sandflow/imscJS
should the PR be merged.These changes are required to allow customisation of subtitles on TV Platforms through Bigscreen Player for an upcoming low-latency trial. As the content is delivered in chunks for low-latency DASH it is would not be possible to use the existing side-chain mechanism in BSP without significant modification.