Closed Bug 938022 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Web Audio, defect)

25 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: lapsio3, Assigned: roc)

References

Details

Attachments

(7 files, 2 obsolete files)

12.04 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
8.15 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
2.12 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
938 bytes, patch
cpearce
: review+
Details | Diff | Splinter Review
5.26 KB, patch
padenot
: review+
Details | Diff | Splinter Review
14.03 KB, patch
padenot
: review+
Details | Diff | Splinter Review
799 bytes, text/html
Details
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
This is pretty much the same issue as bug 934100, but has a simpler test case.
Blocks: 934100
Status: UNCONFIRMED → NEW
Ever confirmed: true
This is different from what we talked about. Instead of redefining mAudioEndTime, this roughly keeps the current definition ("data pushed to output stream").
Assignee: nobody → roc
Attachment #8335070 - Flags: review?(cpearce)
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.
Attachment #8335072 - Flags: review?(cpearce)
Attachment #8335070 - Flags: review?(cpearce) → review+
Attachment #8335072 - Flags: review?(cpearce) → review+
To get tests to work I had to fix more things. Additional patches coming.
Attachment #8337926 - Flags: review?(paul) → review+
Attachment #8337923 - Flags: review?(cpearce) → review+
Attachment #8337925 - Flags: review?(cpearce) → review+
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?
Attachment #8337927 - Flags: review?(paul) → review+
Blocks: 944924
Duplicate of this bug: 934100
Depends on: 950509
Flags: in-testsuite?
Attached file test_bug938022.html (obsolete) —
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.
Depends on: 973782
Removed the test due to bug 973782
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef3dad336585
Flags: in-testsuite+ → in-testsuite?
Attached file test_bug938022.html (obsolete) —
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.
Attachment #8376912 - Attachment is obsolete: true
Attached file test_bug938022.html
This is just a small amendment to verify currentTime == 0 rather than currentTime != duration, as it is a more accurate check.
Attachment #8377972 - Attachment is obsolete: true
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.
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.