-
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
DVB Font Downloads #4338
DVB Font Downloads #4338
Conversation
I'd like community input on the Font isolation is good...The good thing about modifying the font family name is that it provides some degree (see below) of isolation between any font file downloaded specifically for a particular caption adaptation set and any other DOM elements that might coincidentally use the same font. For example, if the UI uses a font called "uifont" and it loads an MPD with a captions adaptation set that specifies a font descriptor to download a specific font file and also associates it with the name "uifont", should the UI font change? In this pull request, it would not, but the subtitles would use that downloaded font. That isolation seems on the surface to be a good thing, but I don't think it was considered during the standardisation process. (please correct me if I'm wrong about that!) ...or is it?It also has a significant downside: if the font descriptor is a "supplemental" property and download fails, and there happens to be a font with the same name that is already available to the user agent, then expected behaviour would be to use that font. However, by renaming the font in the TTML, it will no longer be matched and the captions/subtitles will show in a different font. An alternative way to think about this: if subtitles show in one font when there's no font descriptor in the MPD, then merely adding a supplemental property pointing to a font resource that gives a 404 should not change the font that's actually used, but with this font renaming approach, it would do. Font name clashes can still happenWhen I wrote "some degree of isolation" above... Another downside is that this approach does not guarantee to avoid font name clashes. The recipe for changing the font name means that it is possible to get clashes between fonts in separate MPDs loaded in multiple players in the same page, if they happen to point to different font resources and are given the same font family name; there can also be clashes with fonts whose names happen to begin Other views?I think my view at the moment is to simplify this PR by removing the font renaming code, but I'd like to know if there are other views. Are there other better approaches to the same problem, like making the captions div host a web component and delegating to the UA do whatever isolation is specified in the CSS domain? |
…sh.js into dvb-font-download
Regarding the fontFamily prefixing issues raised by @nigelmegitt, I have discussed this with him directly and also consulted other colleagues and ultimately decided to remove the prefixing as it was not required by the standards and was significantly adding complexity for a scenario that is unlikely to arise in practice. |
As a note, the above commit adds some test streams to the reference player to test (/confirm) this functionality. They are subset from a wider list of test streams [1][2] but we thought having some basic on-demand and live test cases in the reference player would be useful. |
All good, works well with this PR thanks. |
As a note on this commit with the addition of This is problematic when a native text track with It seems as though the user agent retains the cues added when a track is disabled (even though I'm not sure this is how it should work). The most sensible way to mitigate against this was to not call |
* Initial dvb font download work * Continued dvb font download functionality * Seperate DVBFontUtils and TTML prefixing * Essential Property descriptors working and refactor DVB font handling * Ensure fonts are removed and handled with essential property descriptors, tidy up * Comment tidy up * Clean up and function commenting * Team feedback corrections * Non dvbFonts unit tests * Add DVBFonts unit tests * Update types * Update BSD-3 header in accordance with contributor guide * Remove font prefixing functionality * Remove references to prefixing fontFamilies in TTMLParser * Move events from CoreEvents to MediaPlayerEvents * Address PR comments on DashAdapter, DashManifestModel and TextSourceBuffer * Address PR comments for DVBFonts * Missing semicolon removal * Add DVB Font Download test streams to reference player * Use camelCase for DescriptorType dvb extensions * Handle disabled tracks correctly in the reference player controlbar * Fix controlbar text track and native track matching * Fix issue with disabled track cues * Add DVB font download test streams to functional tests * Update imscJS version and remove now unneeded fix
This is an initial proposal to add downloadable font functionality for subtitles in dash.js based on what is described in the DVB Dash specification ETSI TS 103 285 V1.4.1 - Section 7.2 Downloadable Fonts.
Changes/Overview:
schemeIdUri
andvalue
. For DVB font downloadsdvb:fontFamily
,dvb:url
anddvb:mimeType
will be presented, but these wont pollute theMediaInfo
for other descriptors without these.schemeIdUri
do not get filtered out in capabilities filtering.Add a prefix to theresolve the font download URL (against BaseUrl where necessary).fontFamily
value to prevent clashes with other browser/local fonts and alsocheck for any relevant downloaded fonts for the associated track. Reapply the sameapply a fix for a bug in imscJS v1.1.4.fontFamily
prefix to the parsed IMSC documentsTesting
The added functionality has been tested against the font download test streams made available by BBC Research & Development. These streams are all VOD and this has not yet been tested against live streams yet.
Things to Note:
Supplemental/Essential Properties as Objects:
As further attributes of supplemental/essential property descriptors need to be considered the handling of these changes. Where these descriptors are represented in MediaInfo objects they go from:
to:
However, there could be a scenario where multiple descriptors with the same schemeIdUri causes information to be overwritten. This does not occur where these are represented in their
_AsArray
form.Additionally, not all supplemental properties descriptors have value attributes, such as where
schemeIdUri="urn:mpeg:dash:preselection:2016"
is used.Disabling Tracks:
Where font downloads fail for fonts given in Essential Property descriptor tags, the adaptation set should then be ignored. To implement this, the track is disabled until a font download is successful.
Trying to remove a textTrack from the textTracks of a video element isn’t possible without error where the textTrack is not represented in the DOM.
TextTracks.deleteTextTrack(idx)
was unused previously and did not work when trying to use it.Prefixing Fonts:Where a fontFamily is listed in an MPD a prefix is added so this is unique to the AdaptationSet it is described in. When the same unprefixed font appears in the TTML documents (linked to the AdapataionSet) the same prefix is added so the fontFamily is interpreted correctly.This gives us some reassurance that the font earmarked for subtitles display will not clash with any local/browser fonts causing subtitles to be shown in the wrong and equally to not affect other elements on the page. There isn’t fine-grained control of the FontFaceSets in the browser and indexing is done by FontFamily name, so reducing clashes here is beneficial.IMSC:
An outstanding issue within the imscJS library used by dash.js was causing font family values to be handled correctly. This has been addressed in this PR, until this PR in imscJS makes its way into a release.
DVBFonts not per stream:
The TextController creates TextTracks and TextSourceBuffers per streamId. This didn’t seem necessary to do for DVBFonts, as font downloading interfaces with the browser (which is consistent throughout) and is reset when a stream switches.
Font Support:
Code added has been written with the intention of covering the DVB Dash specification. This does not specify mimeTypes/support for woff2 fonts so this has not been added.
TODO:
Unit testingTypescript typesPossible change to ControlBar.js to only show tracks that do not have amode
value ofdisabled
.