shaka-project / shaka-player

JavaScript player library / DASH & HLS client / MSE-EME player
Apache License 2.0
7.06k stars 1.33k forks source link

Codec string for IMSC1 Text should have ...ttml... (lower case) #2378

Closed porcelijn closed 4 years ago

porcelijn commented 4 years ago

Have you read the FAQ and checked for duplicate open issues? yes

What version of Shaka Player are you using? v2.5.9 (uncompiled)

Can you reproduce the issue with our latest release version? yes

Can you reproduce the issue with the latest code from master? yes

Are you using the demo app or your own custom app? demo app

If custom app, can you reproduce the issue using our demo app? yes

What browser and OS are you using? Ubuntu linux + firefox

For embedded devices (smart TVs, etc.), what model and firmware version are you using? x

What are the manifest and license server URIs? https://demo.unified-streaming.com/video/tears-of-steel/tears-of-steel-ttml.ism/.mpd

What did you do?

What did you expect to happen?

What actually happened?

The subtitle adaptation set looks like:

<AdaptationSet
      id="6"
      group="3"
      contentType="text"
      lang="es"
      mimeType="application/mp4"
      codecs="stpp.ttml.im1t"
      startWithSAP="1">

It works only after changing codecs="stpp.ttml.im1t" to codecs="stpp.TTML.im1t", but this "fix" is probably incorrect. There has been some confusion about whether the correct syntax uses TTML or ttml and unfortunately the official ref isn't very helpful. However, Apple (after fueling the initial confusion) has adjusted their spec to include:

Note that if a Variant Stream specifies one or more Renditions that include IMSC subtitles, the CODECS attribute MUST indicate this with a format identifier such as "stpp.ttml.im1t".

DVB says:

Valid examples include: •"stpp.ttml.etd1"-TTML content suitable for presentation by an EBU-TT-D renderer •"stpp.ttml.etd1|im1t"or "stpp.ttml.im1t|etd1"-TTML content suitable for presentation by an EBU-TT-D or IMSC1

and the imsc1 spec mentions in example:

Example 1 The use of the mimeType="application/mp4" and codecs="stpp.ttml.im1t" attributes in a [MPEGDASH] Manifest signals a track that consists of [ttml-imsc1.0.1] Text Profile documents. In this case the Manifest is part of the document interchange context, and the default processor profile is extracted from the codecs attribute and dereferenced via the [ttml-profile-registry].

IMHO the only change needed would be the registration in shaka-player/lib/text/mp4_ttml_parser.js + adjustment of a couple of test cases.

joeyparrish commented 4 years ago

It's possible that codec strings should all be normalized to lowercase on output from the manifest parsers. Then the TTML parser registration can be lowercase, too.

I'm struggling to find the RFC or spec that will say for certain if codec strings should be case sensitive or not. It seems that RFC 1341 says that MIME type parameters have case-insensitive keys, but case-sensitive values, unless a specific key is spec'd somewhere to have case-insensitive values, too.

I think I'm confident enough, though, to make the change in the master branch and wait to see what breaks. Are you interested to send a PR for this? (Edit: specifically, to convert all codec strings from DASH & HLS manifests to lowercase.)

Another thing we could do for the short-term is register the TTML parser for both uppercase and lowercase variants of the codec strings. This would be safe enough to cherry-pick to the v2.5.x release branch. Are you interested to send a PR for this, too?

porcelijn commented 4 years ago

Thanks for your quick response! I had a look at RFC 6381 and a bunch of other resources but could not find a conclusive answer either way..

Sure I can make a pull request (may take a couple of days). Bear with me.

porcelijn commented 4 years ago

@joeyparrish PR #2381, implements you second suggestion (to register both uppercase and lowercase variants of the codec strings) and adjust the unit test in favour of lower case (to align with rfc8216bis).

This is likely a pragmatic mid/long term solution as well, as the stpp.TTML.im1t variant is presumably to stick around for a while in legacy content.