Skip to content
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

Merged

Conversation

mattjuggins
Copy link
Contributor

@mattjuggins mattjuggins commented Dec 21, 2023

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:

  • Handle supplemental/essential property descriptors so that more attributes can be attached to them rather than just schemeIdUri and value. For DVB font downloads dvb:fontFamily, dvb:url and dvb:mimeType will be presented, but these wont pollute the MediaInfo for other descriptors without these.
  • Ensure essential properties with the DVB font download schemeIdUri do not get filtered out in capabilities filtering.
  • When text tracks are created, check for valid DVB font download attributes and collect the information on each font to be downloaded, including the text track it is relevant to. Add a prefix to the fontFamily value to prevent clashes with other browser/local fonts and also resolve the font download URL (against BaseUrl where necessary).
  • Keep track of fonts that were described in essential property descriptors and initially set that text track to be disabled, as the AdaptationSet should be ignored if the font download fails.
  • Download the fonts. Trigger events for successful or failed font downloads.
  • Where font downloads are successful for fonts from essential property descriptors enable the track.
  • As TTML documents are parsed, check for any relevant downloaded fonts for the associated track. Reapply the same fontFamily prefix to the parsed IMSC documents apply a fix for a bug in imscJS v1.1.4.
  • If all is well, subtitles are rendered on screen with the custom font download.
  • Remove fonts from the document when the stream changes.

Testing

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:

{
  'schemeIdUri': 1
}

to:

{
  'schemeIdUri': {
    value: 1,
    ...other attributes
  }
}

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 testing
  • Typescript types
  • Possible change to ControlBar.js to only show tracks that do not have a mode value of disabled.

@nigelmegitt
Copy link
Contributor

nigelmegitt commented Dec 22, 2023

I'd like community input on the tts:fontFamily renaming approach here. I haven't found any specification document that requires this behaviour, it adds complexity without always being desirable, and it could cause surprises.

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 happen

When 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 dashjs- by some other accident.

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?

@mattjuggins
Copy link
Contributor Author

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.
This could always be added back down the line if real-world experience suggests it is needed.

@mattjuggins mattjuggins marked this pull request as ready for review January 8, 2024 15:15
@dsilhavy dsilhavy self-requested a review January 8, 2024 17:12
@dsilhavy dsilhavy added this to the 4.7.4 milestone Jan 8, 2024
src/core/events/CoreEvents.js Outdated Show resolved Hide resolved
src/dash/DashAdapter.js Outdated Show resolved Hide resolved
src/dash/models/DashManifestModel.js Outdated Show resolved Hide resolved
src/dash/models/DashManifestModel.js Outdated Show resolved Hide resolved
src/dash/vo/DescriptorType.js Outdated Show resolved Hide resolved
src/streaming/text/DVBFonts.js Outdated Show resolved Hide resolved
src/streaming/text/TextController.js Show resolved Hide resolved
src/streaming/text/TextSourceBuffer.js Outdated Show resolved Hide resolved
src/streaming/text/TextTracks.js Show resolved Hide resolved
src/streaming/utils/TTMLParser.js Outdated Show resolved Hide resolved
@mattjuggins
Copy link
Contributor Author

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.
Hopefully, these are appropriate to be added. If they would be better placed in a separate PR I'd be happy to move them over.

@dsilhavy
Copy link
Collaborator

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. Hopefully, these are appropriate to be added. If they would be better placed in a separate PR I'd be happy to move them over.

All good, works well with this PR thanks.

@mattjuggins
Copy link
Contributor Author

As a note on this commit with the addition of mode: 'disabled' native text tracks I'd introduced a bug where if a native text track had mode: 'disabled' and .addCue() was being called on it, the cues property would appear as null rather than a TextTrackCuesList. However, if the track mode changed to 'showing'|'hidden', the cues added whilst the track was disabled would reappear into the cue property.

This is problematic when a native text track with mode: 'disabled' has cues added to it, a new MPD is loaded, and these retained cues aren't actually cleared up because they don't exist on the cues property of the track. If the newly loaded MPD has a text track with the same properties as the previous one (likely if the content provider is the same) but now has mode: 'hidden'|'showing', then the cues property of this track can have the cues from the previous stream with the disabled text track added to it. This causes subtitles from the previous stream to appear as well as subs from the new stream (if cue times are similar, which if it's VOD, could well happen).

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 .addCue() on a track when it has mode: 'disabled'.

@dsilhavy dsilhavy merged commit 37c68da into Dash-Industry-Forum:development Jan 22, 2024
3 checks passed
eirikbjornr pushed a commit to bbc/dash.js that referenced this pull request Feb 14, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants