588312 - When set, video.currentTime returns old value until seek completes

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

When video.currentTime is set to a new position, video.seeking becomes true immediately but video.currentTime returns the old value until the seeked event fires.

The spec (step 8) says that currentTime should reflect the new position before seeking is fired.

Testcase attached.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: testcase

Attach testcase, like, for real.

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: testcase

Better testcase.  This also shows that we fire timeupdate just before seeked, rather than just after seeking as the spec requires.

Comment 3

[Matthew Gregan [:kinetik]]

When this is fixed, please also revert the workaround made in videocontrols.xml:

                         case "seeked":
+			    // XXX: This call to showBuffered() can be removed
+			    // when bug 588312 is fixed.
+                            this.showBuffered();
+                            this.setupStatusFader();
+                            break;
                         case "playing":

Comment 4

[cajbir (:cajbir)]

Requesting blocking as it blocks blocking bug 584615.

Comment 5

[cajbir (:cajbir)]

Attachment: Fix

Comment 6

[Matthew Gregan [:kinetik]]

-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Did you mean to remove this?

     mElement->SeekStarted();
+    mElement->DispatchSimpleEvent(NS_LITERAL_STRING("timeupdate"));

Can we move the timeupdate into SeekStarted()?  That way other (non-nsBuiltin) backends will have the correct behaviour.

The Wave backend needs a similar fix to what you've done in nsBuiltinDecoderStateMachine::Run().  And I'm not sure if the Wave backend is tested--test_timeupdate3 is Ogg specific.  We should convert it to a backend independent test.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 472531 [details] [diff] [review]
Fix

Drive by review...

>diff --git a/content/media/nsBuiltinDecoder.cpp b/content/media/nsBuiltinDecoder.cpp
>index 2baa287..76b2b0a 100644
>--- a/content/media/nsBuiltinDecoder.cpp
>+++ b/content/media/nsBuiltinDecoder.cpp
>@@ -703,6 +703,7 @@ void nsBuiltinDecoder::SeekingStarted()
>   if (mElement) {
>     UpdateReadyStateForData();
>     mElement->SeekStarted();
>+    mElement->DispatchSimpleEvent(NS_LITERAL_STRING("timeupdate"));
>   }
> }

Does the UpdatePlaybackPosition(seekTime) call you added already cause a timeupdate to be dispatched if the playback position changes?

>diff --git a/content/media/nsBuiltinDecoderStateMachine.cpp b/content/media/nsBuiltinDecoderStateMachine.cpp
>index 4ba3523..a22df31 100644
>--- a/content/media/nsBuiltinDecoderStateMachine.cpp
>+++ b/content/media/nsBuiltinDecoderStateMachine.cpp
>@@ -980,6 +979,16 @@ nsresult nsBuiltinDecoderStateMachine::Run()
>         PRInt64 seekTime = mSeekTime;
>         mDecoder->StopProgressUpdates();
> 
>+        PRBool currentTimeChanged = false;
>+        PRInt64 currentTime = mStartTime + mCurrentFrameTime;

Variable currentTime is unused?

>diff --git a/content/media/test/test_timeupdate3.html b/content/media/test/test_timeupdate3.html
>index 0fb1a3a..b6e7c85 100644
>--- a/content/media/test/test_timeupdate3.html
>+++ b/content/media/test/test_timeupdate3.html
>@@ -27,16 +28,19 @@ function startSeek() {
>     return false;
> 
>   seeking = true;
>+  var v = document.getElementById('v');
>+  ok(v.currentTime == 2.0,
>+     "Check currentTime of " + v.currentTime + " is requested seek time");

s/2.0/seekTime/ ?

Comment 8

[Matthew Gregan [:kinetik]]

(In reply to comment #7)

> Does the UpdatePlaybackPosition(seekTime) call you added already cause a
> timeupdate to be dispatched if the playback position changes?

Not necessarily, because mPositionChangeQueued might be true which would suppress sending the timeupdate caused by seeking.

Comment 9

[cajbir (:cajbir)]

Attachment: Fix

Addresses review comments. I've made test_timeupdate3 a backend independent test and renamed it to test_timeupdate_seek. 

> Does the UpdatePlaybackPosition(seekTime) call you added already cause a
> timeupdate to be dispatched if the playback position changes?

I've not changed this since allowing for it complicates the code quite a bit and the gain of not having one additional timeupdate on a seek start didn't seem worth it. If it's still an issue can we create a bug specific for it to address the refactoring that would be needed to do it?

Comment 10

[Matthew Gregan [:kinetik]]

Comment on attachment 473874 [details] [diff] [review]
Fix

         if (NS_FAILED(rv)) {
           NS_WARNING("Seek failed");
-        }
-        monitor.Enter();
-        if (NS_SUCCEEDED(rv)) {
-          mPlaybackPosition = position;
+          mPlaybackPosition = oldPosition;
         }

We need to fire a timeupdate for this.  It turns out we do because we call FirePositionChanged(PR_TRUE) further down in the SEEKING state.  I think it'd be better to move that call inside the if here, since we've already fired back a timeupdate at the start of the sek in the success case.

r+ with that.

Comment 11

[cajbir (:cajbir)]

Attachment: Fix

Address final review comment and add commit message to patch. carried r=kinetik forward.

Comment 12

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/rev/5c303535a850