Skip to content

Commit

Permalink
fix(VTT): Fix timing of VTT in HLS without map header
Browse files Browse the repository at this point in the history
When there is no X-TIMESTAMP-MAP header, the HLS spec states that the
client should map 0 to 0, effectively.  That means we were wrong to
offset the cues by the segment time.

Since DASH IOP states that segmented text should be packaged in MP4,
this should not affect compliant DASH content.

Closes #2714

Change-Id: Iddb00f2fd1afeb4f0f2c99f92f65e5db0e3a84f1
  • Loading branch information
joeyparrish committed Aug 12, 2020
1 parent 34a20d0 commit 4bf22e2
Show file tree
Hide file tree
Showing 3 changed files with 9 additions and 18 deletions.
4 changes: 3 additions & 1 deletion lib/text/vtt_text_parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ shaka.text.VttTextParser = class {
shaka.util.Error.Code.INVALID_TEXT_HEADER);
}

let offset = time.segmentStart;
// NOTE: "periodStart" is the timestamp offset applied via TextEngine.
// It is no longer closely tied to periods, but the name stuck around.
let offset = time.periodStart;

This comment has been minimized.

Copy link
@Iragne

Iragne Feb 12, 2021

Contributor

Hi @joeyparrish I'm checking that changed and i'm a bit worried aboutit, it will impact also DASH, also in the DASH IOP, we have well the possibility to have subtitle as text. Let me know if i'm not understanding it correctly, I think it's link to my cc issue and in fact affect dash

This comment has been minimized.

Copy link
@joeyparrish

joeyparrish Feb 12, 2021

Author Member

Hi @Iragne,

I don't understand what you mean by that. Please file an issue if you haven't already, and we can discuss there. Thank you!

This comment has been minimized.

Copy link
@Iragne

Iragne Feb 13, 2021

Contributor

@joeyparrish i was planning to do it with an example. the point is that by revert back this change it's working.
I don't want to create other PR without not understanding your fix. Thanks will do it


if (blocks[0].includes('X-TIMESTAMP-MAP')) {
// https://bit.ly/2K92l7y
Expand Down
2 changes: 1 addition & 1 deletion test/text/cue_integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ describe('Cue', () => {
'WEBVTT\n\n' +
'00:00:20.000 --> 00:00:40.000\n' +
'Test',
{periodStart: 0, segmentStart: 7, segmentEnd: 0});
{periodStart: 7, segmentStart: 10, segmentEnd: 60});
expect(cues.length).toBe(1);
expect(cues[0].startTime).toBe(27);
expect(cues[0].endTime).toBe(47);
Expand Down
21 changes: 5 additions & 16 deletions test/text/vtt_text_parser_unit.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,17 +83,6 @@ describe('VttTextParser', () => {
{periodStart: 0, segmentStart: 0, segmentEnd: 0});
});

it('ignores offset', () => {
verifyHelper(
[
{startTime: 20, endTime: 40, payload: 'Test'},
],
'WEBVTT\n\n' +
'00:00:20.000 --> 00:00:40.000\n' +
'Test',
{periodStart: 0, segmentStart: 0, segmentEnd: 0});
});

it('supports cues with no settings', () => {
verifyHelper(
[
Expand Down Expand Up @@ -407,22 +396,22 @@ describe('VttTextParser', () => {
{periodStart: 0, segmentStart: 0, segmentEnd: 0});
});

it('uses segment time', () => {
it('uses time offset from periodStart, not segmentStart', () => {
verifyHelper(
[
{
startTime: 40, // Note these are 20s off of the cue
endTime: 60, // because using relative timestamps
startTime: 70,
endTime: 80,
payload: 'Test',
textAlign: 'center',
size: 56,
writingMode: Cue.writingMode.VERTICAL_LEFT_TO_RIGHT,
},
],
'WEBVTT\n\n' +
'0:00:20.000 --> 0:00:40.000 align:center size:56% vertical:lr\n' +
'0:00:10.000 --> 0:00:20.000 align:center size:56% vertical:lr\n' +
'Test',
{periodStart: 0, segmentStart: 20, segmentEnd: 0});
{periodStart: 60, segmentStart: 80, segmentEnd: 100});
});


Expand Down

0 comments on commit 4bf22e2

Please sign in to comment.