-
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
Smooth Streaming Support : TTML Captions #1878
Smooth Streaming Support : TTML Captions #1878
Conversation
@@ -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(); |
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.
Please add a comment here that MSS has an offset corresponding to the segment start, while DASH has none.
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. |
Hi Went on holidays last week ;-) |
- Display TTML on smooth streaming streams
- Update comment
feaf6c5
to
94ae30d
Compare
@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? |
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 |
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. |
@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? |
@palemieux Smooth does not follow ISO 14496-30, so one needs different timing in TTML for MSS vs DASH. |
Ok. Thanks for clarifying. See proposal at #1693 (comment) |
@TobbeEdgeware, if you activate MSS in dash, the first sample ('Prince of Persia') has subtitles. |
@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 |
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 |
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.