Mochitest - Record media with no timeslice for Media Recording API

RESOLVED FIXED in mozilla26

Status

()

Core
Audio/Video: Recording
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jsmith, Assigned: jsmith)

Tracking

(Blocks: 1 bug)

Trunk
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 years ago
Create a mochitest that records media with no timeslice for the media recording API.
(Assignee)

Comment 1

5 years ago
Created attachment 783526 [details] [diff] [review]
Patch v1
(Assignee)

Updated

5 years ago
Blocks: 896303
(Assignee)

Updated

5 years ago
Blocks: 889772
(Assignee)

Comment 2

5 years ago
Created attachment 783530 [details]
Test Output - First Patch
(Assignee)

Comment 3

5 years ago
Comment on attachment 783526 [details] [diff] [review]
Patch v1

Asking for feedback right now because I can't formally land this patch yet as this mochitest reveals a leak. But I'd at least like to know if this patch is on the right track.

I'll file a bug for the leak and also kick off a try run with this patch shortly.
Attachment #783526 - Flags: feedback?(roc)
(Assignee)

Updated

5 years ago
Assignee: nobody → jsmith
Status: NEW → ASSIGNED
The leak issue can be solved by this 
https://bug896353.bugzilla.mozilla.org/attachment.cgi?id=781659
Please open it and I will phase in this solution.
(Assignee)

Updated

5 years ago
Depends on: 899883
(Assignee)

Updated

5 years ago
No longer depends on: 899883
(Assignee)

Updated

5 years ago
Depends on: 896353
(Assignee)

Comment 6

5 years ago
Try run reveals the following:

- I'll need to add a todo for the mime type check after start per bug 898396
- The leak being fixed in bug 896353 needs to land first to get this test to pass without leaks
- There's one timeout on an OS X 10.6 opt build that indicates that ondataavailable never fired. I'll wait for Rob's feedback to see if he knows why we're timing out - is it a bug in the development code or a bug in the test?
Comment on attachment 783526 [details] [diff] [review]
Patch v1

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

::: content/media/test/test_mediarecorder_record_no_timeslice.html
@@ +89,5 @@
> +    mediaRecorder.start();
> +
> +    is(mediaRecorder.state, 'recording', 'Media recorder should be recording');
> +    is(mediaRecorder.mimeType, expectedMimeType,
> +       'Mime type upon starting recording = ' + expectedMimeType);

I don't think we should expect the type to be available synchronously. This is going to be difficult to ensure in general. I think MediaRecorder.mimeType should actually go away.

@@ +105,5 @@
> +
> +    is(mediaRecorder.state, 'inactive',
> +       'Media recorder is inactive afer being stopped');
> +    is(mediaRecorder.mimeType, expectedMimeType,
> +       'Mime type upon stopping recording = ' + expectedMimeType);

As above.
Attachment #783526 - Flags: feedback?(roc) → feedback+
Actually there is a bug in the test. It's possible all the timeupdates we're going to get could fire before canplaythrough fires, in which case this test will never complete.
(Assignee)

Comment 9

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> Actually there is a bug in the test. It's possible all the timeupdates we're
> going to get could fire before canplaythrough fires, in which case this test
> will never complete.

Hmm...maybe that's the reasoning why we timed out on one of the runs.

One idea I could do here is ignore the canplaythrough handling entirely and just rely on two timeupdate events firing. When the first fires, we start recording. When the second fires, we stop recording.
Why not just run the contents of the canplaythrough handler immediately after setting up the element and MediaRecorder?
(Assignee)

Comment 11

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Why not just run the contents of the canplaythrough handler immediately
> after setting up the element and MediaRecorder?

Good point. That's probably a cleaner approach here to use then relying on multiple timeupdate events.
(Assignee)

Updated

5 years ago
Attachment #783526 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #783530 - Attachment description: Test Output → Test Output - First Patch
(Assignee)

Comment 12

5 years ago
Created attachment 786738 [details] [diff] [review]
Rapid Succession Approach Patch
(Assignee)

Comment 13

5 years ago
Updated the patch according to Rob's comments. However, this test currently times out 100%, as onstop and ondataavailable will fail to fire. Filing a bug for this now.
(Assignee)

Updated

5 years ago
Depends on: 902318
(Assignee)

Updated

5 years ago
Attachment #786738 - Attachment description: patch v2 → Rapid Succession Approach Patch
(Assignee)

Comment 14

5 years ago
Created attachment 786745 [details] [diff] [review]
Two Timeupdate Event Approach v2
(Assignee)

Updated

5 years ago
Attachment #786745 - Attachment description: Two Timeslice Event Approach v2 → Two Timeupdate Event Approach v2
(Assignee)

Comment 15

5 years ago
The two timeupdate approach apparently works with the known leak in the dependency.

https://tbpl.mozilla.org/?tree=Try&rev=b80519e274fc
(Assignee)

Comment 16

5 years ago
Comment on attachment 786745 [details] [diff] [review]
Two Timeupdate Event Approach v2

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

Given that bug 902318 exists, we can't use the rapid succession approach. So I'm flagging review on the approach that uses two timeupdate events.
Attachment #786745 - Flags: review?(roc)
(Assignee)

Comment 17

5 years ago
Try run kicked off with the two timeupdate approach post landing of the memory leak fix - https://tbpl.mozilla.org/?tree=Try&rev=9fb7b34fc18b.
(Assignee)

Updated

5 years ago
Depends on: 899883
(Assignee)

Updated

5 years ago
No longer depends on: 899883
(Assignee)

Comment 18

5 years ago
(In reply to Jason Smith [:jsmith] from comment #17)
> Try run kicked off with the two timeupdate approach post landing of the
> memory leak fix - https://tbpl.mozilla.org/?tree=Try&rev=9fb7b34fc18b.

Turns out I forgot to pull the latest changes when I did this try run. On a followup try run Randy kicked off, we're green - https://tbpl.mozilla.org/?tree=Try&rev=f5fe2c48718d.
(Assignee)

Comment 19

5 years ago
Flagging checkin-needed per comment 18's green try run.
Keywords: checkin-needed
oh, please wait until my another bug 903758 landded for this two line.
+      is(mediaRecorder.state, 'inactive',
+         'Media recorder is inactive afer being stopped');
(Assignee)

Comment 21

5 years ago
(In reply to Randy Lin [:rlin] from comment #20)
> oh, please wait until my another bug 903758 landded for this two line.
> +      is(mediaRecorder.state, 'inactive',
> +         'Media recorder is inactive afer being stopped');

Okay. I'll hold checkin-needed until that lands.
Depends on: 903758
Keywords: checkin-needed
(Assignee)

Comment 22

5 years ago
Dependency is on inbound, so we're good to check this in now.
Keywords: checkin-needed
(Assignee)

Comment 25

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> Backed out for B2G timeouts.
> https://hg.mozilla.org/integration/b2g-inbound/rev/e35b9df27729
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=26447273&tree=B2g-Inbound

Hmm...that implies ondataavailable failed to fire when it should.

Randy - Can you investigate what's failing here?
Flags: needinfo?(rlin)
(Assignee)

Comment 27

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #26)
> And Windows.
> https://tbpl.mozilla.org/php/getParsedLog.php?id=26450032&tree=B2g-Inbound

Okay, so that further confirms ondataavailable is failing to fire intermittently. And it's platform independent likely then.
In this case, it seems start recording right at end the music playback(media stream is under endding?). The test opus duration is 2.92s, Can we start recording after element.oncanplaythrough() event?

windows. 
14:08:37     INFO -  151805 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | [started detodos.opus-0] Length of array should match number of running tests
14:08:40     INFO -  151806 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | Media recorder should be recording
14:08:40     INFO -  151807 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | Media recorder stream = element stream at the start of recording


b2g emu
19:54     INFO -  08-12 19:29:07.412   693   693 I GeckoDump: 780 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | [started detodos.opus-0] Length of array should match number of running tests
13:19:54     INFO -  08-12 19:29:10.622   693   693 I GeckoDump: 781 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | Media recorder should be recording
13:19:54     INFO -  08-12 19:29:10.642   693   693 I GeckoDump: 782 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | Media recorder stream = element stream at the start of recording

test pass case. MacOSX
12:25:18     INFO -  147537 INFO TEST-INFO | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | Started Mon Aug 12 2013 12:25:18 GMT-0700 (PDT) (1376335518.692s)
12:25:18     INFO -  147538 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | [started detodos.opus-0] Length of array should match number of running tests
12:25:19     INFO -  147539 INFO TEST-PASS | /tests/content/media/test/test_mediarecorder_record_no_timeslice.html | Media recorder should be recording
Flags: needinfo?(rlin)
(Assignee)

Comment 29

5 years ago
That makes sense. Starting after canplaythrough was what I originally did. However, the one problem that does exist is knowing when to call stop on media recording. Doing the rapid succession approach on canplaythrough won't work due to bug 902318.

I could however wait until onended fires potentially on the media element to stop recording right after onended fires.

An important question here to ask is - if the media element has finished playing, doesn't mean that the stream has ended itself? Wouldn't that imply that onstop should fire if the stream has ended? The try run shows that neither ondataavailable nor onstop fired, which seems strange.

Rob - What do you think?
Flags: needinfo?(roc)
It's possible that by the time canplaythrough fires, media element playback has already ended. The same goes for any event, because events can be delayed an arbitrarily long time.

I think we simply have to handle the case where playback has already ended as an additional possible success case.
Flags: needinfo?(roc)
Test with " Two Timeupdate Event Approach v2 " patch and remove the recorder.stop() function call. I found when the audio element play end, the EndTrack in TrackUnionStream.h has been called, also the  l->NotifyQueuedTrackChanges invoked, but I found the MediaEncoder.cpp miss receiving this notify, a little strange.
(Assignee)

Comment 32

5 years ago
(In reply to Randy Lin [:rlin] from comment #31)
> Test with " Two Timeupdate Event Approach v2 " patch and remove the
> recorder.stop() function call. I found when the audio element play end, the
> EndTrack in TrackUnionStream.h has been called, also the 
> l->NotifyQueuedTrackChanges invoked, but I found the MediaEncoder.cpp miss
> receiving this notify, a little strange.

Should we file a bug for this issue?
I think this bug is for this issue. 

Bug 899935 - Stopping the MediaRecorder does not recieve a TRACK_EVENT_ENDED event from callback 
Hi roc. 
Do I need to hook anything in mediaRecorder to trigger the TrackUnionStream::EndTrack?
It seems there are two instances of TrackUnionStream while recording, but the one used for recording isn't trigger the EndTrack method.
Flags: needinfo?(roc)
I don't know why EndTrack wouldn't be called. It should be.
Flags: needinfo?(roc)
Modify some test case code and try server looks ok. 
https://tbpl.mozilla.org/?tree=Try&rev=97abea53d51d
src = https://hg.mozilla.org/try/raw-rev/97abea53d51d

BTW, should fire another one to look why mochitest can't found EndTrack event from mediaStream and cause timeout.
(Assignee)

Comment 36

5 years ago
(In reply to Randy Lin [:rlin] from comment #35)
> Modify some test case code and try server looks ok. 
> https://tbpl.mozilla.org/?tree=Try&rev=97abea53d51d
> src = https://hg.mozilla.org/try/raw-rev/97abea53d51d
> 
> BTW, should fire another one to look why mochitest can't found EndTrack
> event from mediaStream and cause timeout.

Yup, that was about what I was thinking was needed to fix this initially, but there's one issue - when bug 899935 gets fixed, this test will have onstop to fire twice. What I'd change here is switch mozCaptureStreamUntilEnded to mozCaptureStream so that ending the playback of the element won't cause onstop to fire.
(Assignee)

Comment 37

5 years ago
Just kicked off a try run for using Randy's patch with mozCaptureStream instead here - https://tbpl.mozilla.org/?tree=Try&rev=59d692f64008. If it's green, I'll post the patch up for review.
(Assignee)

Updated

5 years ago
Attachment #786738 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #786745 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #783530 - Attachment is obsolete: true
(Assignee)

Comment 38

5 years ago
Created attachment 792829 [details] [diff] [review]
Patch v3
(Assignee)

Updated

5 years ago
Attachment #792829 - Flags: review?(roc)
(Assignee)

Comment 39

5 years ago
We're green on try, so we're ready for review here.
Comment on attachment 792829 [details] [diff] [review]
Patch v3

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

::: content/media/test/test_mediarecorder_record_no_timeslice.html
@@ +88,5 @@
> +    mediaRecorder.start();
> +    is(mediaRecorder.state, 'recording',
> +       'Media recorder should be recording');
> +    is(mediaRecorder.stream, element.stream,
> +       'Media recorder stream = element stream at the start of recording');

The stream could already have ended by the time canplaythrough fires, so I think this test could fail.
(Assignee)

Comment 41

5 years ago
Comment on attachment 792829 [details] [diff] [review]
Patch v3

Oh right. Forgot about the above comment you mentioned above about that.
Attachment #792829 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #792829 - Attachment is obsolete: true
(Assignee)

Comment 42

5 years ago
Created attachment 793334 [details] [diff] [review]
Patch v4
(Assignee)

Comment 43

5 years ago
Comment on attachment 793334 [details] [diff] [review]
Patch v4

Is this what you are meaning to imply by stating that we should treat finding out ended = true in canplaythrough as a success condition?

What this does instead is that the test only runs in the case when canplaythrough fires with ended = false at the start of the event handler. If ended = true when canplaythrough fires, we skip the test.
Attachment #793334 - Flags: review?(roc)
Comment on attachment 793334 [details] [diff] [review]
Patch v4

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

::: content/media/test/test_mediarecorder_record_no_timeslice.html
@@ +84,5 @@
> +  };
> +
> +  element.oncanplaythrough = function () {
> +    // If content has ended, skip the test
> +    if(element.ended) {

space before (

@@ +86,5 @@
> +  element.oncanplaythrough = function () {
> +    // If content has ended, skip the test
> +    if(element.ended) {
> +      ok(true, 'ended fired before canplaythrough, skipping test');
> +      manager.finished(token);

Yeah, this is what I meant.
Attachment #793334 - Flags: review?(roc) → review+
(Assignee)

Updated

5 years ago
Attachment #793334 - Attachment is obsolete: true
(Assignee)

Comment 45

5 years ago
Created attachment 793355 [details] [diff] [review]
Patch v5

Fixed nit on spacing. Carrying forward r+.
Attachment #793355 - Flags: review+
(Assignee)

Comment 46

5 years ago
Kicked off a try run again to double check here - https://tbpl.mozilla.org/?tree=Try&rev=84935daa810d. If it's green, then I'll mark checkin-needed for landing.
(Assignee)

Comment 47

5 years ago
Try run is green and we have a r+. Ready for landing.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cf3db1cabcb2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(Assignee)

Updated

5 years ago
Whiteboard: [qa-]
Component: Video/Audio → Video/Audio: Recording
You need to log in before you can comment on or make changes to this bug.