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

Smooth Streaming Support : TTML Captions #1878

Conversation

jeremco
Copy link
Contributor

@jeremco jeremco commented Apr 13, 2017

Hello

The goal of this PR is to fix a pb on Smooth Streaming subtitles.
The TTML begin and end tags are absolute. But in this case they are relative to fragments start time.

@@ -326,7 +330,10 @@ function TextSourceBuffer() {
subOffset += sample.subSizes[j];
}
try {
result = parser.parse(ccContent, sampleStart / timescale, (sampleStart + sample.duration) / timescale, images);
// check if we must apply an offset to ttml time
let manifest = manifestModel.getValue();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment here that MSS has an offset corresponding to the segment start, while DASH has none.

@TobbeEdgeware
Copy link

This is definitely a bug, since the MSS time intervals will be off without the fix. The code looks fine, but I think it is essential to point out that the offset parameter is there only to support MSS, so that there is no-one thinking that it can be used for DASH as well.

@jeremco
Copy link
Contributor Author

jeremco commented Apr 24, 2017

Hi

Went on holidays last week ;-)
I have modified the comment

- Display TTML on smooth streaming streams
- Update comment
@TobbeEdgeware
Copy link

@dsparacio The code climate check is not helpful here since it complains that we declare the same variables in multiple files. How can we suppress the failing check?

@TobbeEdgeware TobbeEdgeware mentioned this pull request May 9, 2017
@palemieux
Copy link

The TTML begin and end tags are absolute. But in this case they are relative to fragments start time.

Is this consistent with the following prose in ISO 14496-30?

The top-level internal timing values in the timed text samples based on TTML express times on the track
presentation timeline – that is, the track media time as optionally modified by the edit list. For example,
the begin and end attributes of the element, if used are relative to the start of the track, not
relative to the start of the sample.

@nigelmegitt
Copy link
Contributor

If I understand correctly @palemieux the constraints of ISO 14496-30 do not necessarily apply to Smooth streams. Regardless of whether that is correct according to the spec for Smooth, all the TTML in Smooth that I have seen has fragment-relative times.

@TobbeEdgeware
Copy link

@jeremco Can you add a subtitled Smooth test asset to the reference player so that we have a way of checking that this patch works, and so that we can also make sure it works when integration imsc1JS?

@TobbeEdgeware
Copy link

@palemieux Smooth does not follow ISO 14496-30, so one needs different timing in TTML for MSS vs DASH.

@palemieux
Copy link

palemieux commented May 9, 2017

Ok. Thanks for clarifying.

See proposal at #1693 (comment)

@jeremco
Copy link
Contributor Author

jeremco commented May 10, 2017

@TobbeEdgeware, if you activate MSS in dash, the first sample ('Prince of Persia') has subtitles.
To activate MSS, juste add <script src="../../dist/dash.mss.debug.js"></script> in index.html of dash-if <script src="../../dist/dash.mss.debug.js"></script>

@TobbeEdgeware
Copy link

@jeremco I checked the MSS source you mentioned, and the French subtitles work, while the English don't.

Anyway, I think this is good, so I'll merge this. We then need to carry over this to the imscJS1 integration in #1693. Instead of sending an offset to TTMLParser, I think that we should send the flag ttmlTimeIsRelative, since that code becomes simpler and the ttmlTimeIsRelative flag fits nicely into the calculations of startTime and stopTime

@TobbeEdgeware TobbeEdgeware merged commit 0ce70f4 into Dash-Industry-Forum:development May 10, 2017
@jeremco
Copy link
Contributor Author

jeremco commented May 10, 2017

I agree wih you @TobbeEdgeware.

Thanks for the merge. I have checked for english subtitles. Actually, I think they work, but the subtitles are not from Prince of Persia, and they still are written in french ;-) This is a test stream

@jeremco jeremco deleted the MSS_TTML_Captions branch June 6, 2017 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants