WebVTT parsing issues. #7464

stevemayhew · 2020-06-03T22:12:56Z

This pull fixes issue #7462 Cue timestamps do not wrap with PTS wrap in the stream

stevemayhew · 2020-06-04T06:04:15Z

There are UT that fail, will look into these and fix them first.

stevemayhew · 2020-06-04T19:13:21Z

We are working on an extractor test to verify the logic in WebvttExtractor.processSample()

All the core and hls tests now run, including the one just added for timestamp wrap detection.

stevemayhew · 2020-06-05T00:47:19Z

library/common/src/main/java/com/google/android/exoplayer2/util/TimestampAdjuster.java

@@ -33,7 +33,7 @@
   * The value one greater than the largest representable (33 bit) MPEG-2 TS 90 kHz clock
   * presentation timestamp.
   */
-  private static final long MAX_PTS_PLUS_ONE = 0x200000000L;
+  public static final long MAX_PTS_PLUS_ONE = 0x200000000L;


Can keep this private and move adjustCueForPtsWrap(long) into this class, pretty much a personal preference choice I could go either way.

stevemayhew · 2020-06-05T00:49:43Z

library/core/src/main/java/com/google/android/exoplayer2/text/webvtt/WebvttParserUtil.java

+        input.setPosition(headerStartAt);   // set so next readLine() reads TIMESTAMP header
+      }
+    }
+    return isValidHeader;


It seemed best to keep all the logic dealing with the WEBVTT_HEADER here, Pantos is ambiguous as to termination, the w3c spec explicitly says it can be followed by a space or tab. This code handles space, tab or newline termination.

stevemayhew · 2020-06-05T01:05:10Z

library/core/src/main/java/com/google/android/exoplayer2/text/webvtt/WebvttParserUtil.java

+   */
+  private static long adjustCueForPtsWrap(long webvttCueTimestampMs) {
+    long cueAsPts = (webvttCueTimestampMs * 90) % (TimestampAdjuster.MAX_PTS_PLUS_ONE - 1);
+    return TimestampAdjuster.ptsToUs(cueAsPts);
  }


All WebVTT timestamps pass through this logic, including the LOCAL time, so that should cover all the possible interpretations of Section 3.5 last paragraph. Note, if the timestamp is less then 1 day, 2:30:43.717677 (33 bit PTS wrap max value), then this code becomes a noop. If it's over that, then the actual value is the remainder (line 100).

stevemayhew · 2020-06-05T01:07:10Z

library/core/src/test/java/com/google/android/exoplayer2/text/webvtt/WebvttDecoderTest.java

+    assertThat(subtitle.getEventTime(5)).isEqualTo(5_527_005_000L);
+
+  }
+


This test covers adjustCueForPtsWrap() for wrapped or un-wrapped timestamps (unwrapped is probably covered by other tests. I can remove that if people prefer?

stevemayhew · 2020-06-05T01:08:29Z

library/hls/src/main/java/com/google/android/exoplayer2/source/hls/WebvttExtractor.java

-            TimestampAdjuster.ptsToUs(
-                Long.parseLong(Assertions.checkNotNull(mediaTimestampMatcher.group(1))));
+        tsTimestampPts =
+                Long.parseLong(Assertions.checkNotNull(mediaTimestampMatcher.group(1)));
      }


The MPEGTS values needs to stay a PTS un-molested. You'll see why below on line 183 below.

stevemayhew · 2020-06-05T01:11:48Z

library/hls/src/main/java/com/google/android/exoplayer2/source/hls/WebvttExtractor.java

+
+    // Scale the MPEGTS "media time" (default is 0 if no TIMESTAMP-MAP) to ExoPlayer media time timebase
+    //
+    long mediaTimeOfSegmentStartUs = timestampAdjuster.adjustTsTimestamp(tsTimestampPts);


First adjust the only PTS value we have, either the default of 0 (almost no one that has synchronized VTT to a live MPEG stream is able to do this) or the value from MPEGTS attribute. This value is normalized to sample time (ExoPlayer's normalized time across all elementary streams).

We can debate a better name, this is basically the time for the start of the segment the VTT file covers. Pantos calls this media time, but in ExoPlayer this is sample time right?

We shouldn't pass this through the stateful timestampAdjuster.adjustTsTimestamp() method it updates internal state in TimestampAdjuster assuming this is the 'next' sample timestamp, and it might not be a sample timestamp at all (see comment below about how this is just a mapping, it might not relate to the cue timestamps at all).

The only values that should be passed through adjustTsTimestamp() are times that correspond to actual samples to be displayed. This is distinct from the static methods on TimestampAdjuster which don't update any internal state. I think that's why the existing code is careful to use the static methods until initialising sampleTimeUs below.

stevemayhew · 2020-06-05T01:13:57Z

library/hls/src/main/java/com/google/android/exoplayer2/source/hls/WebvttExtractor.java

+    // base, time 0.  The delta from the adjusted PTS is the time to offset samples times with
+    //
+    long subsampleOffsetUs = mediaTimeOfSegmentStartUs - vttTimestampUs;
+


The renderer adds this to the Cue timestamp to covert the Cue timestamp to media time, so we subtract the segment start time stamp, aka vttTimestampUs (maybe a different name would make this more clear)

vttTimestampUs is not the segment start time stamp. It's only valid for mapping directly to the MPEGTS value from the X-TIMESTAMP-MAP header (tsTimestampsPts). Paragraph 3.5 says:
"The cue timestamp in the LOCAL attribute MAY fall outside the range of time covered by the segment." So we can't assume it indicates the start of the segment.

stevemayhew · 2020-06-05T01:15:41Z

library/hls/src/main/java/com/google/android/exoplayer2/source/hls/WebvttExtractor.java

-        TimestampAdjuster.usToPts(firstCueTimeUs + tsTimestampUs - vttTimestampUs));
-    long subsampleOffsetUs = sampleTimeUs - firstCueTimeUs;
+    long sampleTimeUs = firstCueTimeUs + subsampleOffsetUs;
+


A further trip through the adjustTsTimestamp() will not work here. The Cue's sample time is always simply it's timestamp + subsampleOffsetUs (already normalized to media time)

stevemayhew · 2020-06-05T01:27:00Z

library/hls/src/test/java/com/google/android/exoplayer2/source/hls/WebvttExtractorTest.java

+    FakeExtractorOutput output = TestUtil.extractAllSamplesFromFile(extractor, ApplicationProvider.getApplicationContext(), "webvtt/wrapped_pts");
+
+    output.assertOutput(ApplicationProvider.getApplicationContext(), "webvtt/wrapped_pts_golden_extractor_output");
+  }


We can add more test data, not sure if it was ok to depend on the testdata module here.

stevemayhew · 2020-06-05T01:39:31Z

testdata/src/test/assets/webvtt/wrapped_pts_golden_extractor_output

+    sampleMimeType = text/vtt
+    subsampleOffsetUs = 622
+  sample 0:
+    time = 5522778844


This is correct for the first sample, the first time stamp is: 266:39:21.999, which unwrapped is 5522778222, or 1:32:02.778222.

Add the offset:

>>> print 5522778222 + 622 5522778844

stevemayhew · 2020-06-08T16:37:18Z

@icbaker are you proposing this commit ee0c622 with this pull request or instead of it?

I'm fine to keep the \t vs \n bug fix ourselves in our own branch and remove it from the pull request (FWIW, this is a major origin vendor that presents the header in this way, they argue that Safari plays the content) we will keep trying to convince them of the error in their ways, thanks for your support with this with data from the specs.

I'll rebase this pull then to include your fix and my test cases (which I"ll change to fix with your change). I'm playing this branch against a live feed of the channel, will let you all know if there are further issues.

Tests the logic for wrapping timestamps where a PTS wrap has occured but cue timestamps in the WebVTT file do not. This is allowed as per the lkast paragraph in the Pantos spec [section 3.5](https://tools.ietf.org/html/draft-pantos-hls-rfc8216bis-04#section-3.5)

There was no coverage for this code, so this is a start. We can find or make more samples to cover all the cases from here.

icbaker · 2020-06-08T16:51:38Z

@icbaker are you proposing this commit ee0c622 with this pull request or instead of it?

I'm proposing "instead of", or at least instead of the changes in WebvttExtractor. I have added a test to WebvttExtractorTest in a follow-up commit (just pushed) - but it doesn't cover all the way to rendering.

I'm fine to keep the \t vs \n bug fix ourselves in our own branch and remove it from the pull request (FWIW, this is a major origin vendor that presents the header in this way, they argue that Safari plays the content) we will keep trying to convince them of the error in their ways, thanks for your support with this with data from the specs.

Have you shown categorically that Safari is parsing the X-TIMESTAMP-MAP? Because the example you provided to us only has that 622µs difference between TS and WebVTT time, I'm not sure how you could tell the difference between parsing it and ignoring it (and assuming 0:0) - it would only shift the subtitles by 622µs which I suspect is undetectable.

(maybe the offset isn't always 622µs on this channel, and the sample you provided just happens to have that)

For now I think we should keep our parsing as-is (require the header on a new line), though I'm happy if you'd like to file a separate issue to track this and others can reply there if they start seeing more content that looks like this out in the wild.

FWIW you can probably carry a simpler patch in your own branch by just adding webvttData.setPosition(0); after this line:

ExoPlayer/library/hls/src/main/java/com/google/android/exoplayer2/source/hls/WebvttExtractor.java

Line 141 in b1e5630

WebvttParserUtil.validateWebvttHeaderLine(webvttData);

And then change L151 to .contains("X-TIMESTAMP-MAP"):

ExoPlayer/library/hls/src/main/java/com/google/android/exoplayer2/source/hls/WebvttExtractor.java

Line 151 in b1e5630

if (line.startsWith("X-TIMESTAMP-MAP")) {

This just rewinds to look at the WEBVTT line in the first iteration of the loop.

I'll rebase this pull then to include your fix and my test cases (which I"ll change to fix with your change). I'm playing this branch against a live feed of the channel, will let you all know if there are further issues.

Thanks! Let me know if you think we need more test coverage in WebvttExtractorTest - I suspect what I've added is good enough for now - but if you've got a test that is more end-to-end that would be interesting.

Use the values calculated from the commit ee0c622 In that change the subsample offset is computed using the full un-wrapped value of the first cue timestamp.

stevemayhew · 2020-06-08T19:13:48Z

@ojw28 and @icbaker Please let me know if you want any of the unit tests. I am working on a test for the balance of the samples (only the first sample is dealt with by WebvttExtractorTest), but if you are working on this internally I'll stop.

We are working to increase the unit test coverage (our internal organization has coverage goals).

I'm perfectly fine if my commits are cleaned up before merging in, When it creates conflicts in the pull request though I'd appreciate a heads up as to what I should do with the pull request, I don't want to spend time on work that is duplicated.

stevemayhew · 2020-06-08T20:07:11Z

@icbaker are you proposing this commit ee0c622 with this pull request or instead of it?

I'm proposing "instead of", or at least instead of the changes in WebvttExtractor. I have added a test to WebvttExtractorTest in a follow-up commit (just pushed) - but it doesn't cover all the way to rendering.

Ok.. I'll shoot a note here before I work on any other changes. I'm testing re-based with your changes on the channel here. I reverted my WebvttExtractorTest

I'm fine to keep the \t vs \n bug fix ourselves in our own branch and remove it from the pull request (FWIW, this is a major origin vendor that presents the header in this way, they argue that Safari plays the content) we will keep trying to convince them of the error in their ways, thanks for your support with this with data from the specs.

Have you shown categorically that Safari is parsing the X-TIMESTAMP-MAP?

That is a great point. Yes, I can modify locally to shift it many seconds and maybe prove Safari does not parse it, that will help convince the origin vendor they need to fix it. Meanwhile we will keep that change out.

For now I think we should keep our parsing as-is (require the header on a new line), though I'm happy if you'd like to file a separate issue

Yes, agreed.. Hopefully we can prove Safari ignores it too.

stevemayhew · 2020-06-08T21:25:11Z

@icbaker Good call.. I'll file the bug back to the origin vendor. Safari ignores the malformed WEBVTT header. Thanks so much for the background from the specs, that will help.

If you've got a handle on doing test cases and all I can close this pull request and let you continue. Please let me know if there is anything else we can help with.

icbaker · 2020-06-09T08:54:16Z

@icbaker Good call.. I'll file the bug back to the origin vendor. Safari ignores the malformed WEBVTT header. Thanks so much for the background from the specs, that will help.

Interesting! Thanks for checking Safari with a bigger offset.

If you've got a handle on doing test cases and all I can close this pull request and let you continue. Please let me know if there is anything else we can help with.

I'm planning to leave the testing as-is (i.e. b7486f4, checking the output of WebvttExtractor). It's not perfect, as you say ideally we'd check on the rendering side, but writing tests of that scope is hard (harder than it should be probably) and I don't have the bandwidth to dedicate to working on it right now I'm afraid.

So I think you can probably close this.

stevemayhew · 2020-06-09T13:10:49Z

We’re fine doing a test case on the render side, my organization is motivated to increase test coverage and it’s always good to have test cases around code that is complicated to understand and possibly will get refactored. Will rebase rest of the changes out then, or just open a new pull From Steve's iPhone

…

On Jun 9, 2020, at 1:54 AM, Ian Baker ***@***.***> wrote: @icbaker Good call.. I'll file the bug back to the origin vendor. Safari ignores the malformed WEBVTT header. Thanks so much for the background from the specs, that will help. Interesting! Thanks for checking Safari with a bigger offset. If you've got a handle on doing test cases and all I can close this pull request and let you continue. Please let me know if there is anything else we can help with. I'm planning to leave the testing as-is (i.e. b7486f4, checking the output of WebvttExtractor). It's not perfect, as you say ideally we'd check on the rendering side, but writing tests of that scope is hard (harder than it should be probably) and I don't have the bandwidth to dedicate to working on it right now I'm afraid. So I think you can probably close this. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

stevemayhew · 2020-06-11T05:03:23Z

I’ll open a fresh pull request with the test cases when they are ready, everything substantial in this branch is “merged”. Thanks @icbaker

googlebot added the cla: yes label Jun 3, 2020

stevemayhew marked this pull request as draft June 4, 2020 06:03

andrewlewis requested a review from icbaker June 4, 2020 07:24

andrewlewis assigned icbaker Jun 4, 2020

stevemayhew marked this pull request as ready for review June 4, 2020 19:13

stevemayhew commented Jun 5, 2020

View reviewed changes

stevemayhew added 2 commits June 8, 2020 09:43

Add simple test for the WebvttExtractor TIMESTAMP_MAP parsing

0f571ac

There was no coverage for this code, so this is a start. We can find or make more samples to cover all the cases from here.

Fix test case to have tab seperator

8000b98

Use the values calculated from the commit ee0c622 In that change the subsample offset is computed using the full un-wrapped value of the first cue timestamp.

stevemayhew force-pushed the p-fix-webvtt-issues branch from 1352b71 to 8000b98 Compare June 8, 2020 19:02

stevemayhew closed this Jun 11, 2020

google locked and limited conversation to collaborators Aug 11, 2020

stevemayhew deleted the p-fix-webvtt-issues branch June 11, 2021 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WebVTT parsing issues. #7464

WebVTT parsing issues. #7464

stevemayhew commented Jun 3, 2020 •

edited

Loading

stevemayhew commented Jun 4, 2020

stevemayhew commented Jun 4, 2020

stevemayhew Jun 5, 2020

stevemayhew Jun 5, 2020

stevemayhew Jun 5, 2020

stevemayhew Jun 5, 2020

stevemayhew Jun 5, 2020

stevemayhew Jun 5, 2020

icbaker Jun 5, 2020

stevemayhew Jun 5, 2020

icbaker Jun 5, 2020

stevemayhew Jun 5, 2020

stevemayhew Jun 5, 2020

stevemayhew Jun 5, 2020

stevemayhew commented Jun 8, 2020

icbaker commented Jun 8, 2020

stevemayhew commented Jun 8, 2020

stevemayhew commented Jun 8, 2020

stevemayhew commented Jun 8, 2020

icbaker commented Jun 9, 2020

stevemayhew commented Jun 9, 2020 via email

stevemayhew commented Jun 11, 2020

		assertThat(subtitle.getEventTime(5)).isEqualTo(5_527_005_000L);

		}

WebVTT parsing issues. #7464

WebVTT parsing issues. #7464

Conversation

stevemayhew commented Jun 3, 2020 • edited Loading

stevemayhew commented Jun 4, 2020

stevemayhew commented Jun 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevemayhew commented Jun 8, 2020

icbaker commented Jun 8, 2020

stevemayhew commented Jun 8, 2020

stevemayhew commented Jun 8, 2020

stevemayhew commented Jun 8, 2020

icbaker commented Jun 9, 2020

stevemayhew commented Jun 9, 2020 via email

stevemayhew commented Jun 11, 2020

stevemayhew commented Jun 3, 2020 •

edited

Loading