Skip to content

fix(TTML): Correctly handle multiple samples in a segment#8088

Merged
avelad merged 1 commit intoshaka-project:mainfrom
julijane:fix-ttml-multiple-sample-issue-8087
Feb 17, 2025
Merged

fix(TTML): Correctly handle multiple samples in a segment#8088
avelad merged 1 commit intoshaka-project:mainfrom
julijane:fix-ttml-multiple-sample-issue-8087

Conversation

@julijane
Copy link
Contributor

@julijane julijane commented Feb 15, 2025

Fixes #8087

Implements handling of multiple samples in a MP4/ISOBMFF/DASH TTML segment/fragment. Such segments are allowed by ISO14496-12 and ISO23000-19. gpac creates such segments. The prior code just treated the full MDAT as one TTML XML document and tried to parse it in whole without accounting for sample(s). A testcase is included which was created by taking the testdata from ttml-segment.mp4 and splitting the subtitles into two independent TTML-XML documents, which then were put as individual samples.

The testdata for the prior existing multiple MDAT testcase was invalid. It was created by taking the same ttml-segment.mp4 as a source and just duplicating the MDAT box, but without then also fixing the TRUN box. The duplicated data was thus not referenced. The test case still worked, because the prior code did not look at the TRUN box and the sample specification at all and just handled any full MDAT box = 1 sample. The testdata was replaced with a new file, which is basically the same as for the multiple samples case, but with the two samples split into two MDAT boxes.

@julijane julijane force-pushed the fix-ttml-multiple-sample-issue-8087 branch from 5f618b5 to 62877e1 Compare February 15, 2025 03:08
@theodab theodab added type: bug Something isn't working correctly component: TTML The issue involves TTML subtitles specifically priority: P1 Big impact or workaround impractical; resolve before feature release labels Feb 15, 2025
@shaka-bot
Copy link
Collaborator

Incremental code coverage: 98.48%

@julijane julijane force-pushed the fix-ttml-multiple-sample-issue-8087 branch from 62877e1 to c1ffbd5 Compare February 15, 2025 09:23
@julijane julijane requested a review from theodab February 15, 2025 09:25
@avelad avelad requested review from avelad and tykus160 February 17, 2025 08:48
@avelad avelad added this to the v4.14 milestone Feb 17, 2025
@julijane julijane force-pushed the fix-ttml-multiple-sample-issue-8087 branch from c1ffbd5 to 862aec2 Compare February 17, 2025 09:34
@julijane julijane requested a review from tykus160 February 17, 2025 09:35
Fixes shaka-project#8087

Implements handling of multiple samples in a segment. Also fixes the testdata for the multiple MDAT test, as the prior data was invalid (not conforming to ISO14496-12).
@julijane julijane force-pushed the fix-ttml-multiple-sample-issue-8087 branch from 862aec2 to d9900c7 Compare February 17, 2025 10:34
@julijane
Copy link
Contributor Author

julijane commented Feb 17, 2025

Oops. When looking on my change again, I noticed I missed to change one line when I moved to a Map for the subSampleSizes as suggested by theodab. Just fixed that. Looks like that does not reset approvals here though? But also seems pipelines don't run again now?

@avelad
Copy link
Member

avelad commented Feb 17, 2025

Oops. When looking on my change again, I noticed I missed to change one line when I moved to a Map for the subSampleSizes as suggested by theodab. Just fixed that. Looks like that does not reset approvals here though? But also seems pipelines don't run again now?

This is your first PR, I have to manually approve the tests. Sorry for the inconvenience!

@julijane
Copy link
Contributor Author

julijane commented Feb 17, 2025

This is your first PR, I have to manually approve the tests. Sorry for the inconvenience!

No worries, just wanted to point out that I had to make another change to the PR.

One of the tests (Firefox on Windows) was failing also at the last run, but in WebVTT code, so not related to my change.

@avelad
Copy link
Member

avelad commented Feb 17, 2025

This is your first PR, I have to manually approve the tests. Sorry for the inconvenience!

No worries, just wanted to point out that I had to make another change to the PR.

One of the tests (Firefox on Windows) was failing also at the last run, but in WebVTT code, so not related to my change.

Don't worry, it's just a bug that we have to update some screenshots, but that machine in the lab is not working right now, but I'll fix it in a few days.

@avelad avelad dismissed theodab’s stale review February 17, 2025 11:06

Approved by Wojciech and Alvaro

@avelad avelad merged commit 2562384 into shaka-project:main Feb 17, 2025
16 of 17 checks passed
@julijane julijane deleted the fix-ttml-multiple-sample-issue-8087 branch February 17, 2025 11:48
avelad pushed a commit that referenced this pull request Feb 17, 2025
Fixes #8087

Implements handling of multiple samples in a MP4/ISOBMFF/DASH TTML
segment/fragment. Such segments are allowed by ISO14496-12 and
ISO23000-19. gpac creates such segments. The prior code just treated the
full MDAT as one TTML XML document and tried to parse it in whole
without accounting for sample(s). A testcase is included which was
created by taking the testdata from ttml-segment.mp4 and splitting the
subtitles into two independent TTML-XML documents, which then were put
as individual samples.

The testdata for the prior existing multiple MDAT testcase was invalid.
It was created by taking the same ttml-segment.mp4 as a source and just
duplicating the MDAT box, but without then also fixing the TRUN box. The
duplicated data was thus not referenced. The test case still worked,
because the prior code did not look at the TRUN box and the sample
specification at all and just handled any full MDAT box = 1 sample. The
testdata was replaced with a new file, which is basically the same as
for the multiple samples case, but with the two samples split into two
MDAT boxes.
joeyparrish pushed a commit that referenced this pull request Feb 22, 2025
Fixes #8087

Implements handling of multiple samples in a MP4/ISOBMFF/DASH TTML
segment/fragment. Such segments are allowed by ISO14496-12 and
ISO23000-19. gpac creates such segments. The prior code just treated the
full MDAT as one TTML XML document and tried to parse it in whole
without accounting for sample(s). A testcase is included which was
created by taking the testdata from ttml-segment.mp4 and splitting the
subtitles into two independent TTML-XML documents, which then were put
as individual samples.

The testdata for the prior existing multiple MDAT testcase was invalid.
It was created by taking the same ttml-segment.mp4 as a source and just
duplicating the MDAT box, but without then also fixing the TRUN box. The
duplicated data was thus not referenced. The test case still worked,
because the prior code did not look at the TRUN box and the sample
specification at all and just handled any full MDAT box = 1 sample. The
testdata was replaced with a new file, which is basically the same as
for the multiple samples case, but with the two samples split into two
MDAT boxes.

Backported to v4.9.x
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Apr 18, 2025
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Apr 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

component: TTML The issue involves TTML subtitles specifically priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MP4 TTML segments with multiple samples are not correctly handled

5 participants