Closed Bug 962883 Opened 7 years ago Closed 7 years ago

Fix & re-enable test_mediarecorder_creation_fail.html on emulators

Categories

(Core :: Audio/Video: Recording, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jsmith, Assigned: jwwang)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

In bug 916135, we managed to get a majority of content/media mochitests enabled. However, we were unsuccessful in getting test_mediarecorder_creation_fail.html enabled because there's a bug in the test as cited in https://bugzilla.mozilla.org/show_bug.cgi?id=916135#c57. We need to fix the test & re-enable it on emulators.
Blocks: 889772
I will investigate these disabled bugs one by one and try to fix them.
Assignee: nobody → jwwang
Add diagnostics logs.

logcat output:
TEST-INFO | unknown test url | disable ogg
TEST-INFO | unknown test url | ogg.enabled=false
Preferences::GetBool, media.ogg.enabled=1
Preferences::GetBool, media.opus.enabled=1
MediaRecorder, CreateEncoder=1

For the log, you can see the opus encoder is still created successfully even after the preference is already disabled from Javascript side. That is why the test fails because the encoder is created as unexpected.

It might help to add timeouts to delay the creation of the encoder so that gecko can see a consistent view of the pref value as Javascript side. However, I would like to investigate the cause of the inconsistent view of pref values between Javascript and gecko sides so that other test cases won't stomp on this inconsistency problem in the future, thoughts?
Martijn - How have you worked the fact that a preference needed to be set in your patches with taking into account that SpecialPowers.setBoolPref was async?
Flags: needinfo?(martijn.martijn)
SpecialPowers.setBoolPref needs to be converted to SpecialPowers.pushPrefEnv. I don't know if that would help in this case though.
In some cases the page also has to be reloaded.
Flags: needinfo?(martijn.martijn)
(In reply to Martijn Wargers [:mwargers] (QA) from comment #4)
> In some cases the page also has to be reloaded.

..which would be a bug in the code, btw.
Attached patch part1_pushPrefEnv.patch (obsolete) — Splinter Review
Replace SpecialPowers.setBoolPref with SpecialPowers.pushPrefEnv to fix the timing issue incurred by the async behavior of SpecialPowers.setBoolPref. SpecialPowers.pushPrefEnv receives a callback function so that caller can ensure the pref value change is complete before proceeding the test. Also changing the test file from .opus to .wav for ogg will be disabled during the test and .opus files won't play at all.
Attachment #8365046 - Flags: review?(jsmith)
Comment on attachment 8365046 [details] [diff] [review]
part1_pushPrefEnv.patch

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

r- for the change to using the wav file instead of using the opus file

::: content/media/test/test_mediarecorder_creation_fail.html
@@ +13,5 @@
>  function startTest() {
>    var element = document.createElement('audio');
>  
> +  // Don't use an ogg file since we will disable ogg in the preference.
> +  element.src = 'wavedata_s16.wav';

The whole point of the test is to playback an ogg file with ogg disabled, so this is changing the behavior of the intention of the test.

@@ +68,5 @@
>  SimpleTest.waitForExplicitFinish();
> +SpecialPowers.pushPrefEnv({"set": [["media.ogg.enabled", false]]},
> +  function() {
> +    startTest();
> +  }

Nit - you can reduce this to:

SpecialPowers.pushPrefEnv({"set": [["media.ogg.enabled", false]]}, startTest);
Attachment #8365046 - Flags: review?(jsmith) → review-
Comment on attachment 8365046 [details] [diff] [review]
part1_pushPrefEnv.patch

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

::: content/media/test/test_mediarecorder_creation_fail.html
@@ +13,5 @@
>  function startTest() {
>    var element = document.createElement('audio');
>  
> +  // Don't use an ogg file since we will disable ogg in the preference.
> +  element.src = 'wavedata_s16.wav';

https://bugzilla.mozilla.org/attachment.cgi?id=816846&action=edit
The input is a webaudio stream in Jesse's test case.

I think the intention of the test is to test if the media recorder can handle ogg is disabled.

@@ +68,5 @@
>  SimpleTest.waitForExplicitFinish();
> +SpecialPowers.pushPrefEnv({"set": [["media.ogg.enabled", false]]},
> +  function() {
> +    startTest();
> +  }

Will do.
(In reply to JW Wang[:jwwang] from comment #9)
> Comment on attachment 8365046 [details] [diff] [review]
> part1_pushPrefEnv.patch
> 
> Review of attachment 8365046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/test/test_mediarecorder_creation_fail.html
> @@ +13,5 @@
> >  function startTest() {
> >    var element = document.createElement('audio');
> >  
> > +  // Don't use an ogg file since we will disable ogg in the preference.
> > +  element.src = 'wavedata_s16.wav';
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=816846&action=edit
> The input is a webaudio stream in Jesse's test case.
> 
> I think the intention of the test is to test if the media recorder can
> handle ogg is disabled.

Well right, but that means we should be using an ogg file here, not a wav file.
ah, If we disable the ogg before play audio tag , the audio element would be unplayable.
In this case we want to check the encoder creating logic problem, not the invalid media stream case.
Or do you want me to use a webaudio stream so that there is no audio file at all?
(In reply to Randy Lin [:rlin] from comment #11)
> ah, If we disable the ogg before play audio tag , the audio element would be
> unplayable.
> In this case we want to check the encoder creating logic problem, not the
> invalid media stream case.

So why not modify the test case here to set the preference after the audio element starts playing?
(In reply to Jason Smith [:jsmith] from comment #13)
> (In reply to Randy Lin [:rlin] from comment #11)
> > ah, If we disable the ogg before play audio tag , the audio element would be
> > unplayable.
> > In this case we want to check the encoder creating logic problem, not the
> > invalid media stream case.
> 
> So why not modify the test case here to set the preference after the audio
> element starts playing?

We can do that. But since the purpose of this test is to test the error handling of media recorder when ogg is diabled and the input audio source is not relevant, we would like to keep the test code as simple as possible.
(In reply to JW Wang[:jwwang] from comment #14)
> (In reply to Jason Smith [:jsmith] from comment #13)
> > (In reply to Randy Lin [:rlin] from comment #11)
> > > ah, If we disable the ogg before play audio tag , the audio element would be
> > > unplayable.
> > > In this case we want to check the encoder creating logic problem, not the
> > > invalid media stream case.
> > 
> > So why not modify the test case here to set the preference after the audio
> > element starts playing?
> 
> We can do that. But since the purpose of this test is to test the error
> handling of media recorder when ogg is diabled and the input audio source is
> not relevant, we would like to keep the test code as simple as possible.

Yup, agreed. If the web audio approach keeps things more simple, then feel free to go down that path instead.
Attached patch part1_pushPrefEnv-v2.patch (obsolete) — Splinter Review
Use a webaudio stream instead of an audio file to avoid some mime type could be disabled by preference.
Attachment #8365046 - Attachment is obsolete: true
Attachment #8366374 - Flags: review?(jsmith)
Comment on attachment 8366374 [details] [diff] [review]
part1_pushPrefEnv-v2.patch

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

::: content/media/test/test_mediarecorder_creation_fail.html
@@ +15,5 @@
>    // 1. onerror
>    // 2. ondataavailable
>    // 3. onstop
>    var callbackStep = 0;
> +  var s = new AudioContext().createMediaStreamDestination().stream;

Nit - non-descriptive variable naming, let's use the word 'stream' instead of s
Attachment #8366374 - Flags: review?(jsmith) → review+
Fix naming.
Attachment #8366374 - Attachment is obsolete: true
Attachment #8366448 - Flags: review+
Comment on attachment 8370512 [details] [diff] [review]
part2_enableTest-v1.patch

Can't say this made B2G mochitest-2 anymore failure-prone than it already is, so sure.

That said, we are hitting bug 951100, bug 906752, and bug 964674 an awful lot in those retriggers. I hope you can look at those tests soon.
Attachment #8370512 - Flags: review?(ryanvm) → review+
Hi Ryan,
Please check in part1_pushPrefEnv-v3.patch and part2_enableTest-v1.patch. Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/187e63c535f0
https://hg.mozilla.org/mozilla-central/rev/d5e4d89832ed
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Component: Video/Audio → Video/Audio: Recording
You need to log in before you can comment on or make changes to this bug.