Media Recording - Avoid to record a stopped MediaStream

VERIFIED FIXED in mozilla26

Status

()

VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: rlin, Assigned: rlin)

Tracking

(Blocks: 1 bug)

unspecified
mozilla26
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 14 obsolete attachments)

1.27 KB, text/plain
Details
3.20 KB, patch
Details | Diff | Splinter Review
1.45 KB, patch
Details | Diff | Splinter Review
A stopped MediaStream can't be resumed and record-able. So we need t patch to prevent mediaRecorder record such MediaStream.
Assignee: nobody → rlin
Created attachment 782387 [details] [diff] [review]
patch v1
Attachment #782387 - Flags: review?(roc)
Created attachment 782388 [details] [diff] [review]
patch v1

Fix wrong comment
Attachment #782387 - Attachment is obsolete: true
Attachment #782387 - Flags: review?(roc)
Attachment #782388 - Flags: review?(roc)
Probably doesn't hurt to include a mochitest here btw. Something as simple as setting up a media stream like the mediarecorder creation test, calling stop on the stream, and starting recording. Verify an exception gets thrown here.
Blocks: 889772
umm, this need the gUM's fake audio ready
Bug 895730 - MediaRecorder - Can't get any encoded data from encoder when recording the mediaStream with mute audio
Because gUM's localMediaStream has the stop method.
Depends on: 895730
Created attachment 784168 [details] [diff] [review]
test case

test case for this issue.
try: https://tbpl.mozilla.org/?tree=Try&rev=bbc875e09bff
Attachment #784168 - Flags: review?(jsmith)
Comment on attachment 784168 [details] [diff] [review]
test case

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

review- for cleanup & setTimeout usage.

::: content/media/test/test_mediarecorder_recordstopms.html
@@ +10,5 @@
> +<script class="testbody" type="text/javascript">
> +
> +function startTest() {
> +
> +  navigator.mozGetUserMedia({audio:true,fake:true},

gUM right now is only supported on Desktop Firefox & Firefox for Android, so we'll miss out on Firefox OS coverage until 1.2 gUM support lands on B2G.

I think we'll benefit better if we use the gMediaRecorderTests flow like what's seen in test_mediarecorder_creation.html

@@ +15,5 @@
> +                       function(s) {
> +                         s.stop();
> +                         mr = new MediaRecorder(s);
> +                         // wait the mediaStream stop for a while
> +                         setTimeout(function() {

I don't think you need a setTimeout function here - remove this.

@@ +28,5 @@
> +                       },
> +                       function(e) {dump(e)});
> +}
> +
> +startTest();

It would actually be better to use the gMediaRecorderTests workflow here instead of using a specialized test here not using the framework - that would get more coverage when we add new mime types to gMediaRecorderTests.

manager.runTests(gMediaRecorderTests, startTest);
Attachment #784168 - Flags: review?(jsmith) → review-
Ok, sound reasonable. Could you help to add this test into gMediaRecorderTests also? :)
(In reply to Randy Lin [:rlin] from comment #8)
> Ok, sound reasonable. Could you help to add this test into
> gMediaRecorderTests also? :)

I could, although this should be a quick fix to implement this if you would like to finish this off. You just need to use manager.runTests(gMediaRecorderTests, startTest) to start the test, do the typical element setup you see in mediarecorder_creation, stop the stream, and try to start recording with that stopped stream. Verify that an exception gets thrown.
The mediaStream comes from mozCaptureStreamUntilEnded doesn't support the stop function, 
So I can't use this to test it.
The localmediastream's stop method is async, so I delay 1s for waiting the mediastream state enter endded.
(In reply to Randy Lin [:rlin] from comment #10)
> The mediaStream comes from mozCaptureStreamUntilEnded doesn't support the
> stop function, 
> So I can't use this to test it.

Why not try using mozCaptureStream then?

(In reply to Randy Lin [:rlin] from comment #11)
> The localmediastream's stop method is async, so I delay 1s for waiting the
> mediastream state enter endded.

You could get around doing a timeout here by attaching the stream under test to a media element and waiting for onended to fire on the element after stop is called.

General note - It's usually best to avoid using setTimeout in automation, mainly because it can cause tests to run unreliably in continuous integration.
The mozCaptureStream return the MediaStream interface, not the localmediastream object.
BTW, I will try to find a better way to test this. Thanks for your information. :)
(In reply to Randy Lin [:rlin] from comment #13)
> The mozCaptureStream return the MediaStream interface, not the
> localmediastream object.
> BTW, I will try to find a better way to test this. Thanks for your
> information. :)

Ah, that's right. So in that case, we have to use gUM then. So ignore my comments then about using gMediaRecorder tests.

Here's what you need to do instead then:

* Get rid of the setTimeout call
* Use the media element's onended event handler to know stop() was called on the stream
* Then call start in the onended event handler with stopped stream
* Verify you get an exception

Note - since gUM is not enabled on b2g yet, you'll see to add this test to the b2g.json file as disabled test. We can reenable it once B2G gUM support lands.
Created attachment 784435 [details] [diff] [review]
test case

try to stop the audio tag and get a stopped mediastream.
Attachment #784168 - Attachment is obsolete: true
Attachment #784435 - Flags: review?(jsmith)
Comment on attachment 784435 [details] [diff] [review]
test case

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

If this invokes the exception in this bug, then this looks good with one hit. Only giving feedback+ cause I'm less knowledgeable about if this mochitest triggers the behavior we expect, so I'm sending the review over to roc to confirm.

::: content/media/test/test_mediarecorder_record_stopms.html
@@ +19,5 @@
> +  element.src = test.name;
> +  element.test = test;
> +  element.stream = element.mozCaptureStreamUntilEnded();
> +  var mediaRecorder = new MediaRecorder(element.stream);
> +    try {

Nit - indentation is off here with the try block.
Attachment #784435 - Flags: review?(roc)
Attachment #784435 - Flags: review?(jsmith)
Attachment #784435 - Flags: feedback+
(In reply to Jason Smith [:jsmith] from comment #17)
> Comment on attachment 784435 [details] [diff] [review]
> test case
> 
> Review of attachment 784435 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If this invokes the exception in this bug, then this looks good with one
> hit. Only giving feedback+ cause I'm less knowledgeable about if this
> mochitest triggers the behavior we expect, so I'm sending the review over to
> roc to confirm.

That should be nit, not hit.
Comment on attachment 784435 [details] [diff] [review]
test case

Catch wrong exception, let me check check
throw by element.currentTime = 0;
Attachment #784435 - Flags: review?(roc)
Created attachment 784835 [details] [diff] [review]
test case

Test case for capture this bug.
Use two audio element and try to avoid the latency of media stream isFinished status chagned.
Attachment #784435 - Attachment is obsolete: true
Attachment #784835 - Flags: review?(roc)
Comment on attachment 784835 [details] [diff] [review]
test case

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

The concept of the test looks okay, but I'm confused why two media elements are needed here.

::: content/media/test/test_mediarecorder_record_stopms.html
@@ +16,5 @@
> +  element2.src = test.name;
> +  element2.onended = function() {
> +    element.play();
> +  }
> +  element2.play();

Why do you need two media elements here?

@@ +28,5 @@
> +  element.test = test;
> +
> +  element.onended = function() {
> +    element.mr = new MediaRecorder(element.stream);
> +    element.mr.ondataavailable = function(e) {};

You shouldn't need this for what you are aiming to test in this mochitest.

@@ +32,5 @@
> +    element.mr.ondataavailable = function(e) {};
> +    try {
> +      element.mr.start(1000);
> +      ok(false, 'Should catch exception and block recording');
> +      manager.finished(token);

Nit - you can cut down the code duplication of two manager.finished(token) calls by introducing a finally block.

try {

} catch(e) {

} finally {

}
I found the media stream isn't stopped during the audio element onended event. 
And It seems no event for notifying mediastream has been stopped. 
So I use this way to avoid this async problem.
(In reply to Randy Lin [:rlin] from comment #23)
> I found the media stream isn't stopped during the audio element onended
> event. 
> And It seems no event for notifying mediastream has been stopped. 
> So I use this way to avoid this async problem.

Odd. I'm starting to think that we might want to just to go back to fake gUM stream approach.
Created attachment 785592 [details] [diff] [review]
(Untested) Stopped gUM Stream Mochitest Approach

Here's an untested demonstration of what I'm talking about.
I think both approach is fine for this bug. :)
Comment on attachment 784835 [details] [diff] [review]
test case

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

::: content/media/test/test_mediarecorder_record_stopms.html
@@ +16,5 @@
> +  element2.src = test.name;
> +  element2.onended = function() {
> +    element.play();
> +  }
> +  element2.play();

I agree, we should be able to just play an element through until it's ended, then create a MediaRecorder for its stream and start() it.
Created attachment 785626 [details] [diff] [review]
test case

use only one audio element to catch this bug.
Attachment #784835 - Attachment is obsolete: true
Attachment #784835 - Flags: review?(roc)
Attachment #785626 - Flags: review?(roc)
Comment on attachment 785626 [details] [diff] [review]
test case

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

::: content/media/test/test_mediarecorder_record_stopms.html
@@ +21,5 @@
> +
> +  element.onended = function() {
> +    element.play();
> +    element.onended = function() {
> +      element.mr = new MediaRecorder(element.stream);

Why are you playing the element through twice? Once should be enough.
Is any way to capture the mediaStream isFinished event? I found if I just try to record the mediaStream after onended event, check the mediaStream status and found it isn't IsFinished() already, should wait a while...
Created attachment 786159 [details] [diff] [review]
test case v3

Change to use fake gUM to get the stopped media stream.
try result
https://tbpl.mozilla.org/?tree=Try&rev=80b6445514ae
Attachment #785626 - Attachment is obsolete: true
Attachment #785626 - Flags: review?(roc)
Attachment #786159 - Flags: review?(roc)
Attachment #786159 - Flags: feedback?(jsmith)
Comment on attachment 786159 [details] [diff] [review]
test case v3

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

Concept of the test looks okay, although you need to get rid of the setTimeout call.

::: content/media/test/test_mediarecorder_record_stopms.html
@@ +16,5 @@
> + */
> +function startTest() {
> +  navigator.mozGetUserMedia({audio: true, fake: true}, function(stream) {
> +    stream.stop();
> +    setTimeout(function () {

This shouldn't be needed. We should avoid using setTimeout in tests as it leads to inconsistent test results.
Attachment #786159 - Flags: feedback?(jsmith) → feedback+
Return the original problem, The media stream stop method is async, 
Hi Roc, do we have a better way to identify the media stream has entered a stopped state?
(In reply to Randy Lin [:rlin] from comment #33)
> Return the original problem, The media stream stop method is async, 
> Hi Roc, do we have a better way to identify the media stream has entered a
> stopped state?

Hmm...you could create a media element in the test, attach the gUM stream to that element, play it, set an onended handler, and call stop on the stream. When onended fires, you should be able to confirm that the stream has stopped.

Updated

5 years ago
Attachment #785592 - Attachment is obsolete: true
Created attachment 786728 [details]
Concept using onended

Updated

5 years ago
Attachment #786728 - Attachment is obsolete: true
Created attachment 786730 [details]
Concept using onended
See the attached concept. That's one way you can guarantee the stream is definitely stopped.
Btw - don't forget to pref this off in the testing/mochitest/b2g.json file. We currently don't have gUM enabled on Firefox OS, so this mochitest will only be able to be ran on Desktop & Android until gUM gets enabled on Firefox OS.
(In reply to Randy Lin [:rlin] from comment #33)
> Return the original problem, The media stream stop method is async, 
> Hi Roc, do we have a better way to identify the media stream has entered a
> stopped state?

You could create a new media element, set its srcObject to the stream, and wait for "ended" to fire on the media element.
Thanks roc and Jason, I will modify it :)
Oh I see Jason beat me to it in comment #34. Sorry :-)
Created attachment 786748 [details] [diff] [review]
patch v4

Thanks Jason's test code. It works. 
try record:
https://tbpl.mozilla.org/?tree=Try&rev=ea16005446c6
Attachment #782388 - Attachment is obsolete: true
Attachment #786159 - Attachment is obsolete: true
Attachment #786159 - Flags: review?(roc)
Attachment #786748 - Flags: review?(roc)
Comment on attachment 786748 [details] [diff] [review]
patch v4

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

::: content/media/test/test_mediarecorder_record_stopms.html
@@ +16,5 @@
> +  navigator.mozGetUserMedia({audio: true, fake: true}, function(stream) {
> +    var testAudio = document.getElementById('testAudio');
> +
> +    testAudio.oncanplaythrough = function() {
> +      stream.stop();

I think you should call stream.stop() right after calling testAudio.play().
Created attachment 786759 [details] [diff] [review]
patch v5

Follow by roc's suggestion, 
try 
https://tbpl.mozilla.org/?tree=Try&rev=031be29a1092
Attachment #786748 - Attachment is obsolete: true
Attachment #786748 - Flags: review?(roc)
Attachment #786759 - Flags: review?(roc)
Created attachment 786806 [details] [diff] [review]
disable test on b2g

By comment 14, 
gUM is not enabled on b2g, disable test first.
Attachment #786806 - Flags: review?(jsmith)

Updated

5 years ago
Attachment #786806 - Attachment is patch: true
Comment on attachment 786806 [details] [diff] [review]
disable test on b2g

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

::: testing/mochitest/b2g.json
@@ +20,5 @@
>      "content/media/test/test_bug686942.html": "timed out",
>      "content/media/test/test_can_play_type.html":"timed out",
>      "content/media/test/test_can_play_type_mpeg.html":"7 failures out of 27",
>      "content/media/test/test_can_play_type_no_dash.html":"",
> +    "content/media/test/test_mediarecord_stopped_stream.html":"",

This isn't the name of the mochitest you've created. The name of the mochitest you've created is:

test_mediarecorder_record_stopms.html
Attachment #786806 - Flags: review?(jsmith) → review-
Created attachment 787283 [details] [diff] [review]
disable b2g test
Attachment #786806 - Attachment is obsolete: true
Attachment #787283 - Flags: review?(jsmith)

Updated

5 years ago
Attachment #787283 - Flags: review?(jsmith) → review+
Created attachment 787289 [details] [diff] [review]
check-in patch 1

check-in patch 1, carry reviewer.
Created attachment 787290 [details] [diff] [review]
check-in patch 2

check in patch 2, carry reviewer.
Created attachment 787291 [details] [diff] [review]
check-in patch 2

check-in patch, carry reviewer
Attachment #787290 - Attachment is obsolete: true
Whiteboard: checkin-needed

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: checkin-needed
Attachment #786759 - Attachment is obsolete: true
Attachment #787283 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/86ed8b6613f2
https://hg.mozilla.org/mozilla-central/rev/f30d4621eac8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Verified via landing of mochitest with no backouts.
Status: RESOLVED → VERIFIED
> https://hg.mozilla.org/mozilla-central/rev/f30d4621eac8

Is a bug filed for this failure? Can you list the bug number here?
Flags: needinfo?(rlin)
From comment 14, the gUM isn't enabled on b2g, so avoid this this test first.
Flags: needinfo?(rlin)
(In reply to Randy Lin [:rlin] from comment #55)
> From comment 14, the gUM isn't enabled on b2g, so avoid this this test first.

I was asking for a bug, not an explanation.
Flags: needinfo?(rlin)
I created new one for tracking this one.
Bug 903765 - Media Recording - enable mochitest which used the mozGetUserMedia on b2g.
Flags: needinfo?(rlin)
Component: Video/Audio → Video/Audio: Recording
You need to log in before you can comment on or make changes to this bug.