Last Comment Bug 641718 - Store video/audio timestamps in microseconds
: Store video/audio timestamps in microseconds
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
Depends on:
Blocks: 635649 647984
  Show dependency treegraph
 
Reported: 2011-03-14 18:23 PDT by Chris Pearce (:cpearce)
Modified: 2011-04-16 15:28 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch: store timestamps in usecs instead of ms (101.47 KB, patch)
2011-03-20 15:52 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch: Convert nsWaveReader to usecs (4.51 KB, patch)
2011-03-20 16:03 PDT, Chris Pearce (:cpearce)
kinetik: review+
Details | Diff | Splinter Review
Patch: store timestamps in usecs instead of ms (v2) (102.77 KB, patch)
2011-03-21 20:31 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch: store timestamps in usecs instead of ms (v3) (103.92 KB, patch)
2011-03-28 15:09 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch: store timestamps in usecs instead of ms (v4) (107.89 KB, patch)
2011-03-30 14:20 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch: store timestamps in usecs instead of ms (v5) (113.63 KB, patch)
2011-03-30 21:34 PDT, Chris Pearce (:cpearce)
kinetik: review+
Details | Diff | Splinter Review
Patch: store timestamps in usecs instead of ms (v6) (113.16 KB, patch)
2011-03-31 18:53 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch: video (5.28 KB, patch)
2011-03-31 19:03 PDT, Chris Pearce (:cpearce)
kinetik: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2011-03-14 18:23:22 PDT
Firefox is the only browser that rounds the media element's duration to the nearest millisecond. Chrome, Safari, IE9b and Opera are returning (slightly different!) un-rounded floating point numbers for duration and currentTime. It would be good if we were a bit more precise, and stored our timestamps internally in microseconds.

Bonus points for storing SoundData times in samples, and converting to time at the last possible moment, to prevent rounding errors a la bug 634747.

This is a spin off from bug 635649.
Comment 1 Chris Pearce (:cpearce) 2011-03-20 15:52:58 PDT
Created attachment 520561 [details] [diff] [review]
Patch: store timestamps in usecs instead of ms

* Store internal nsBuiltin*Decoder timestamps in usecs rather than ms.
* Change nsBuiltinDecoder::SetDuration() to take a double, so that it matches nsBuiltinDecoder::GetDuration().
* I didn't change to unsigned timestamps, that can happen in another patch/bug.
* This patch is based on top of (in order) bug 580531, 638617, 628665, 641102, 639391.
* Greenish on Try: http://tbpl.mozilla.org/?tree=MozillaTry&rev=87ef61bd98fe
Comment 2 Chris Pearce (:cpearce) 2011-03-20 16:03:02 PDT
Created attachment 520564 [details] [diff] [review]
Patch: Convert nsWaveReader to usecs

* Convert nsWaveReader to use usecs instead of ms. I split this out so we can land the nsWaveReader refactoring and the patches in this bug separately.
* This patch is based on top of (in order) bug 580531, bug 638617, bug 628665, bug 641102, bug 639391, and bug 635649.
Comment 3 Chris Pearce (:cpearce) 2011-03-21 16:36:10 PDT
Comment on attachment 520561 [details] [diff] [review]
Patch: store timestamps in usecs instead of ms

I discovered that I was getting the following assertion failure:

NS_ASSERTION(mCurrentFrameTime <= clock_time, "Clock should go forwards");
[http://mxr.mozilla.org/mozilla-central/source/content/media/nsBuiltinDecoderStateMachine.cpp#1336]

In test_seek.html, seek5.js on split.webm. This was because we store mPlayDuration as a TimeDuration, whereas we store frame times in usecs. When we initialize mPlayDuration after seeking to be the timestamp of the first frame after seek, we round off from usecs to a TimeDuration and that causes the current time to appear to go backwards by 500us when resuming playback after a seek.

I've reworked my patch to store mPlayDuration in usecs. I was converting mPlayDuration to usecs before use in most cases, and converting usecs to TimeDuration to store into it in some cases anyway, so changing mPlayDuration to usecs makes it less lossy.

I'll re-request review on my new patch after it's gone green on TryServer...
Comment 4 Chris Pearce (:cpearce) 2011-03-21 20:31:32 PDT
Created attachment 520842 [details] [diff] [review]
Patch: store timestamps in usecs instead of ms (v2)

* With mPlayDuration in usecs instead of TimeDuration. Greenish on TryServer.
Comment 5 Chris Pearce (:cpearce) 2011-03-28 15:09:32 PDT
Created attachment 522488 [details] [diff] [review]
Patch: store timestamps in usecs instead of ms (v3)

Same as the last version, but unbitrotten and rebased on current trunk, and I also had to change nsSkeletonState to store (the recently added) nsSkeletonState::mPresentationTime in usecs instead of ms for tests to pass. Still greenish on Try. Any chance of doing the review soonish?
Comment 6 Matthew Gregan [:kinetik] 2011-03-28 21:13:59 PDT
Comment on attachment 520564 [details] [diff] [review]
Patch: Convert nsWaveReader to usecs

+  PRInt64 position = RoundDownToSample(TimeToBytes(seekTime) / 1e6);

Use double(USECS_PER_S).
Comment 7 Matthew Gregan [:kinetik] 2011-03-28 22:48:06 PDT
Comment on attachment 522488 [details] [diff] [review]
Patch: store timestamps in usecs instead of ms (v3)

Side note: it'd be nice if we hoisted the various defines of stuff like USEC_PER_S into a single place rather than redefining them in multiple places.  Especially since we're using two different names (MICROSECONDS_PER_SECOND and USEC_PER_S).

+// Number of microseconds per second. 1e6.
+#define USECS_PER_S 1000000
+
+// Number of microseconds per millisecond.
+#define USECS_PER_MS 1000

Maybe declare these const PRInt64?  Doesn't matter too much though as we're inconsistent on this point in content/media already.

+    return length * 1e6 / mDuration;

Use USEC_PER_S here.

+     return static_cast<double>(mDuration) / 1e6;

Use USECS_PER_S, and use double(mDuration) instead of static_cast.
(Or did we agree to use static_cast in content/media?)

Also anywhere else there's a 1e6.

-#define SEEK_FUZZ_MS 500
+#define SEEK_FUZZ_USECS 500

Presumably this should be 500000.

-  ok(d == 3999, "Checking duration: " + d);
+  ok(d == 4000, "Checking duration: " + d);

Why'd this change?

Also, in tha manifest.js, seek.ogv's metadata claims the duration is 3.966, as does ogginfo.
Comment 8 Chris Pearce (:cpearce) 2011-03-29 19:39:09 PDT
(In reply to comment #7)
> Comment on attachment 522488 [details] [diff] [review]
> Patch: store timestamps in usecs instead of ms (v3)
> 
> Side note: it'd be nice if we hoisted the various defines of stuff like
> USEC_PER_S into a single place rather than redefining them in multiple places. 

Good point. I'll have them all use USECS_PER_S which I defined in VideoUtils.h.

> +     return static_cast<double>(mDuration) / 1e6;
> 
> Use USECS_PER_S, and use double(mDuration) instead of static_cast.
> (Or did we agree to use static_cast in content/media?)

static_cast<type> as agreed F2F.


> -#define SEEK_FUZZ_MS 500
> +#define SEEK_FUZZ_USECS 500
> 
> Presumably this should be 500000.

D'oh! and SEEK_DECODE_MARGIN too. Thanks!

> -  ok(d == 3999, "Checking duration: " + d);
> +  ok(d == 4000, "Checking duration: " + d);
> 
> Why'd this change?

Because the duration is 3.999600s. When we store that in usecs, we retain the 0.000600, but when we store it in ms we effectively floor off the 0.000600. test_seekLies multiplies the duration by 1000 and rounds, so it rounds 3.9996*1000 to 4, whereas with ms timestamps it would round 3.999*1000 to 3999.

> Also, in tha manifest.js, seek.ogv's metadata claims the duration is 3.966, as
> does ogginfo.

OggIndex tells me the duration is 3999ms, manifest.js is wrong, as is ogginfo. When we test that we've found the duration correctly, we only check that the duration is within 0.1s of the expected duration, so we never noticed this disparity before.

Many of the files in manifest.js are reporting durations slightly different (but within 100ms) of the duration stored in manifest.js. We should go through and figure out the correct duration for each file and increase the accuracy of the duration checks some time.
Comment 9 Matthew Gregan [:kinetik] 2011-03-29 21:12:29 PDT
Comment on attachment 522488 [details] [diff] [review]
Patch: store timestamps in usecs instead of ms (v3)

-  double t = aTime * 1000.0;
+  double t = aTime * 1e6;

Still using 1e6 here.

-               GetUndecodedData() / 1000.0,
+               GetUndecodedData() / 1e6,

And here.

+        aBuffered->Add(startTime / 1e6, (endTime - aStartTime) / 1e6);

And here.

Mind filing a bug on fixing the metadata in manifest.js and tightening up the checks in the tests?

(In reply to comment #7)
> Side note: it'd be nice if we hoisted the various defines of stuff like
> USEC_PER_S into a single place rather than redefining them in multiple places. 
> Especially since we're using two different names (MICROSECONDS_PER_SECOND and
> USEC_PER_S).

And for this.  I think it's important for all of these domain conversions to use a consistent name and define/const so they're easy to grep for and verify.
Comment 10 Matthew Gregan [:kinetik] 2011-03-29 21:13:30 PDT
Comment on attachment 522488 [details] [diff] [review]
Patch: store timestamps in usecs instead of ms (v3)

Oops, this is the same patch.  I thought you attached a new one when you commented, sorry.
Comment 11 Chris Pearce (:cpearce) 2011-03-29 21:46:18 PDT
(In reply to comment #9)
> Mind filing a bug on fixing the metadata in manifest.js and tightening up the
> checks in the tests?

Bug 646331.

> > Especially since we're using two different names (MICROSECONDS_PER_SECOND and
> > USEC_PER_S).
> 
> And for this.  I think it's important for all of these domain conversions to
> use a consistent name and define/const so they're easy to grep for and verify.

Bug 646333.
Comment 12 Matthew Gregan [:kinetik] 2011-03-29 21:54:03 PDT
The second thing I mentioned is really two issues.  We're using USEC_PER_S in some places and MICROSECONDS_PER_SECOND in others.  It should be a single define/const.  I filed bug 646334.
Comment 13 Chris Pearce (:cpearce) 2011-03-30 14:20:19 PDT
Created attachment 523117 [details] [diff] [review]
Patch: store timestamps in usecs instead of ms (v4)

With review comments addressed.
Comment 14 Chris Pearce (:cpearce) 2011-03-30 14:59:37 PDT
Comment on attachment 523117 [details] [diff] [review]
Patch: store timestamps in usecs instead of ms (v4)

The WAVE reader landed last night, I'll rebase my patch on top of that, and re-request review, since the patch to make nsWaveReader use usecs doesn't apply cleanly any more.
Comment 15 Chris Pearce (:cpearce) 2011-03-30 21:34:32 PDT
Created attachment 523220 [details] [diff] [review]
Patch: store timestamps in usecs instead of ms (v5)

* Rebased on trunk, includes changes to make nsWaveReader use usecs instead of ms.
* Added a few casts to suppress warnings with MSVC. 
* Includes one extra notable change:

 PRBool nsWaveReader::DecodeAudioData()
 {
   MonitorAutoEnter mon(mMonitor);
   NS_ASSERTION(mDecoder->OnStateMachineThread() || mDecoder->OnDecodeThread(),
                "Should be on state machine thread or decode thread.");
 
-  PRInt64 pos = GetPosition();
+  PRInt64 pos = GetPosition() - mWavePCMOffset;

This ensures that the start time of the audio is calculated correctly (it would be off by a few hundred microseconds without this). Also without this, |pos| can be greater than |len|, causing |remaining| to be negative, and operator new to fail further down.

* The video controls round duration and current time to ms, so when you seek to the end using the video controls' scrubber, you may actually end up less than 1ms before the end.
Comment 16 cajbir (:cajbir) 2011-03-31 07:23:01 PDT
Bug 646819 seems to be the DecodeAudioData issue raised by cpearce in comment 15. We should factor that change into bug 646819 and land sooner rather than wait for this patch.
Comment 17 Chris Pearce (:cpearce) 2011-03-31 15:33:22 PDT
http://hg.mozilla.org/mozilla-central/rev/44d43f095a4f
Comment 18 Chris Pearce (:cpearce) 2011-03-31 17:23:58 PDT
Nuts, caused timeouts in toolkit/content/tests/widgets/test_videocontrols_*_direction.html mochitest. *sigh* backed out:
http://hg.mozilla.org/mozilla-central/rev/c1553501c496
Comment 19 Chris Pearce (:cpearce) 2011-03-31 18:53:23 PDT
Created attachment 523494 [details] [diff] [review]
Patch: store timestamps in usecs instead of ms (v6)

Former patch rebased on trunk.
Comment 20 Chris Pearce (:cpearce) 2011-03-31 19:03:54 PDT
Created attachment 523496 [details] [diff] [review]
Patch: video

* Fix for permaorange on toolkit/content/tests/widget/test_videocontrols_*_direction.html.
* Problem was that nsWaveReader::GetBuffered() is returning time ranges which have times which are not rounded down to the nearest usec, whereas the duration is rounded to usec. The failing test was finishing when video.buffered.end(0)==video.duration, but that couldn't happen due to the precision mismatch. So this patch rounds buffered ranges' times down to the nearest usec. I also added the wav file from toolkit/content/widgets/test/ which exposed the problem to our content/media mochitests, as test_buffered would have found this problem if we'd been testing with this file. test_buffered also shows that this problem doesn't exist in the other backends.

Note You need to log in before you can comment on or make changes to this bug.