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.

* 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

* 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 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...

* With mPlayDuration in usecs instead of TimeDuration. Greenish on TryServer.

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 on attachment 520564 [details] [diff] [review] Patch: Convert nsWaveReader to usecs + PRInt64 position = RoundDownToSample(TimeToBytes(seekTime) / 1e6); Use double(USECS_PER_S).

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.

(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 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 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.

(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.

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.

With review comments addressed.

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.

* 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.

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.

Nuts, caused timeouts in toolkit/content/tests/widgets/test_videocontrols_*_direction.html mochitest. *sigh* backed out: http://hg.mozilla.org/mozilla-central/rev/c1553501c496

Former patch rebased on trunk.

* 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.

http://hg.mozilla.org/mozilla-central/rev/f548e43998be http://hg.mozilla.org/mozilla-central/rev/14a336205f22