938022 - context.createMediaElementSource() breaks audio element currentTime/ontimeupdate event

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Damian]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/30.0.1599.101 Safari/537.36

Steps to reproduce:

create AudioContext
use <audio> element as source for context with MediaElementAudioSourceNode
check <audio> playback progress (currentTime)

demo:
http://wubz.in/testing/waa.html


Actual results:

After attaching to context timeupdate doesn't fire anymore, currentTime is also not changing. However manual setting value of currentTime does change playback progress as well as currenTime property itself, even though meassuring current progress is still impossible.


Expected results:

Audio element should still fire timeupdate/change currentTime value properly

Comment 1

[Karl Tomlinson (:karlt)]

This is pretty much the same issue as bug 934100, but has a simpler test case.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Update mAudioEndTime when we're pushing data to the MediaStream

This is different from what we talked about. Instead of redefining mAudioEndTime, this roughly keeps the current definition ("data pushed to output stream").

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Block captured MediaStream when !MediaDecoderStateMachine::IsPlaying

This fixes a pretty major bug in the capture code. When the MediaDecoderStateMachine stops playback due to buffering, we were still feeding data into the MediaStream (and playing it) as we decoded whatever we had. That's bad.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

To get tests to work I had to fix more things. Additional patches coming.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=06b2554a070a

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: Add some MOZ_OVERRIDEs

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Add MediaTimeToMicroseconds

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 5: Add aCurrentTime parameter to MediaStreamListener::NotifyOutput and fire notifications when the MediaStreamGraph is in a good state

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 6: Have MediaDecoder/MediaDecoderStateMachine that's producing a MediaStream use that stream's current time as the media clock

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=06b2554a070a

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 8337927 [details] [diff] [review]
Part 6: Have MediaDecoder/MediaDecoderStateMachine that's producing a MediaStream use that stream's current time as the media clock

Review of attachment 8337927 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaDecoder.cpp
@@ +206,5 @@
>      mHaveBlockedForStateMachineNotPlaying(false)
>  {
>    mStream->AddMainThreadListener(this);
> +  mListener = new DecodedStreamGraphListener(mStream);
> +  aStream->AddListener(mListener);

s/aStream/mStream/ for consistency.

::: content/media/MediaDecoderStateMachine.cpp
@@ +2447,5 @@
>    AssertCurrentThreadInMonitor();
>  
>    // Determine the clock time. If we've got audio, and we've not reached
>    // the end of the audio, use the audio clock. However if we've finished
>    // audio, or don't have audio, use the system clock.

Update this comment to reflect the fact that we can now also use a clock coming from a MediaStreamGraph.

::: content/media/MediaDecoderStateMachine.h
@@ +327,5 @@
>      }
>      mDecoder = nullptr;
>    }
>  
> +  void SetSyncPointForMediaStream();

Can you add a short comment on this one?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7994af691f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d75ae037706
https://hg.mozilla.org/integration/mozilla-inbound/rev/56a302c46bec
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f0a7db5211e
https://hg.mozilla.org/integration/mozilla-inbound/rev/184ed18485c7
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3af7dbf540f

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c7994af691f5
https://hg.mozilla.org/mozilla-central/rev/4d75ae037706
https://hg.mozilla.org/mozilla-central/rev/56a302c46bec
https://hg.mozilla.org/mozilla-central/rev/3f0a7db5211e
https://hg.mozilla.org/mozilla-central/rev/184ed18485c7
https://hg.mozilla.org/mozilla-central/rev/b3af7dbf540f

Comment 15

[Tony Young [:rfw]]

Attachment: test_bug938022.html

Here's a test case for this bug. When the "playing" event is fired, the currentTime is set to the duration of the audio, if the audio element is connected to the AudioContext's graph via a MediaElementSourceNode. This checks if the currentTime is 0 at the beginning of playback.

Comment 16

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e574d7a951a

Comment 17

[Karl Tomlinson (:karlt)]

Removed the test due to bug 973782
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef3dad336585

Comment 18

[Tony Young [:rfw]]

Attachment: test_bug938022.html

Apparently, the "playing" event can fire after the media starts playing, i.e. when the playing event fires the currentTime may have already advanced past zero. This replacement test case instead adds a listener on the "loadeddata" event which should be fired when the currentTime is 0 (in versions where the bug has been fixed), and also asserts that currentTime is not the duration of the <audio> element.

Comment 19

[Tony Young [:rfw]]

Attachment: test_bug938022.html

This is just a small amendment to verify currentTime == 0 rather than currentTime != duration, as it is a more accurate check.

Comment 20

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8423ee98bc77

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8423ee98bc77

Comment 22

[Karl Tomlinson (:karlt)]

Reverted the test.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5867d6a895f8

roc tells me (though not speaker in the precise context of this test) that the state of media elements can change before events are processed.