1072483 - video buffered TimeRanges give incorrect values for WebM

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[brian]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20140922030204

Steps to reproduce:

1) Create a video element and set the src to a webm file
2) attach event listeners for loadedmetadata, seeked and playing
3) in loadedmetadata event, set currentTime to half of duration and play
4) log readyState and buffered once seeking is done and video has resumed playing

Related to: https://bugzilla.mozilla.org/show_bug.cgi?id=600724

Tested in Firefox Nightly 35.0a1 2014-09-22


Actual results:

To see actual results, go to this link and open the console:
http://jsbin.com/giciqo/2

The example runs this process for two separate webm video files, and both show incorrect results for the "buffered" sections. One file (olympia-hd.webm) actually shows negative values (-3000 for start and -2998.824 for end). The other (giga.webm) shows positive values but they do not include the full range of buffered video.


Expected results:

Start and end times for buffered time ranges should not be negative. (olympia-hd.webm)

We know that the positive values for giga.webm are wrong, because the video is successfully playing so the currentTime *must* be buffered.

Also, for both files, readyState is 4 once seeking is completed. This is the correct value (although maybe it should be 3), but buffered should agree with it.

For comparison, Chrome has the (more or less) correct values for buffered (readyState is a whole other ballgame).

Comment 1

[Matthew Gregan [:kinetik]]

I can reproduce the problem with olympia-hd.webm but not with giga.webm so far.

The problem with olympia-hd.webm is a combination of a unit conversion error in WebMReader::GetBuffered and bug 739094.

Comment 2

[Matthew Gregan [:kinetik]]

Unit conversion error appears to have been introduced in bug 599217:

-    PRInt64 startTimeOffsetNS = aStartTime * NS_PER_USEC;
     for (PRUint32 index = 0; index < ranges.Length(); index++) {
-      mBufferedState->CalculateBufferedForRange(aBuffered,
-                                                ranges[index].mStart,
-                                                ranges[index].mEnd,
-                                                timecodeScale,
-                                                startTimeOffsetNS);
+      PRUint64 start, end;
+      bool rv = mBufferedState->CalculateBufferedForRange(ranges[index].mStart,
+                                                          ranges[index].mEnd,
+                                                          &start, &end);
+      if (rv) {
+        double startTime = start * timecodeScale / NS_PER_S - aStartTime;
+        double endTime = end * timecodeScale / NS_PER_S - aStartTime;

Note that aStartTime is in usec, but (start * timecodeScale / NS_PER_S) is in seconds.

Comment 3

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #2)
> Note that aStartTime is in usec, but (start * timecodeScale / NS_PER_S) is
> in seconds.

Actually, aStartTime may have been ms back then.

Comment 4

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #1)
> I can reproduce the problem with olympia-hd.webm but not with giga.webm so
> far.

Caching issue, I can repro all of the issues now.

Comment 5

[Matthew Gregan [:kinetik]]

The remaining issue is:  when olympia-hd.webm seeks to 49.2005, we need stream offset 15489494 (computed from the cue point for 48.351), but the server responds with a content-range of 15466496-29764475/29764476 (giving us an additional 22998 bytes before the requested offset).  WebMReader::NotifyDataArrived (which builds the buffered byte-time mapping) is called starting at offset 15466496, the parser attempts to read a valid EBML ID from this offset and fails.

This is a regression from bug 1044498, which had a cruder WebM parser that always searched for a cluster ID to use as a sync point before beginning parsing.

Comment 6

[Matthew Gregan [:kinetik]]

Attachment: p1: fix unit conversion error in WebMReader::GetBuffered

Convert aStartTime into internal time units before subtracting it from the buffered start and end times.

Also add a non-fatal assertion and sanity check to avoid underflowing start time calculation.

Comment 7

[Matthew Gregan [:kinetik]]

Attachment: p2: restore cluster sync search to WebM buffered parser

Restore cluster sync search to WebM buffered parser (from bug 1044498) and use it when starting a parser from an arbitrary offset.

Comment 8

[Matthew Gregan [:kinetik]]

https://tbpl.mozilla.org/?tree=Try&rev=8f43f1265407

Comment 9

[Matthew Gregan [:kinetik]]

Attachment: p2.1: restore cluster sync search to WebM buffered parser

Missing init.

Comment 10

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a1675bee882
https://hg.mozilla.org/integration/mozilla-inbound/rev/99e24a19bfd6

Comment 11

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e573326dc516

Comment 12

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #10)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/99e24a19bfd6

Accidentally attached the old patch that was missing the init, causing M1 bustage:

https://tbpl.mozilla.org/php/getParsedLog.php?id=48923093&tree=Mozilla-Inbound#error0

Bustage fix pushed per comment 11.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a1675bee882
https://hg.mozilla.org/mozilla-central/rev/99e24a19bfd6
https://hg.mozilla.org/mozilla-central/rev/e573326dc516

Comment 14

[brian]

Thanks for the very quick response on this one. Looking forward to seeing it in Nightly and beyond.