962883 - Fix & re-enable test_mediarecorder_creation_fail.html on emulators

Status: RESOLVED FIXED

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

Description

[Jason Smith [:jsmith]]

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.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

I will investigate these disabled bugs one by one and try to fix them.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: print debug logs.

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?

Comment 3

[Jason Smith [:jsmith]]

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?

Comment 4

[Martijn Wargers (dead)]

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.

Comment 5

[Martijn Wargers (dead)]

(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.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Attachment: part1_pushPrefEnv.patch

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.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

https://tbpl.mozilla.org/?tree=Try&rev=3173e283f35b
all green on mochitests.

Comment 8

[Jason Smith [: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);

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 10

[Jason Smith [:jsmith]]

(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.

Comment 11

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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.

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Or do you want me to use a webaudio stream so that there is no audio file at all?

Comment 13

[Jason Smith [:jsmith]]

(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?

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

(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.

Comment 15

[Jason Smith [:jsmith]]

(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.

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

Attachment: part1_pushPrefEnv-v2.patch

Use a webaudio stream instead of an audio file to avoid some mime type could be disabled by preference.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

https://tbpl.mozilla.org/?tree=Try&rev=a65c81727bda
try server logs.

Comment 18

[Jason Smith [: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

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

Attachment: part1_pushPrefEnv-v3.patch

Fix naming.

Comment 20

[JW Wang [:jwwang] [:jw_wang]]

Attachment: part2_enableTest-v1.patch

try: https://tbpl.mozilla.org/?tree=Try&rev=8fee1a877151

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 22

[JW Wang [:jwwang] [:jw_wang]]

Hi Ryan,
Please check in part1_pushPrefEnv-v3.patch and part2_enableTest-v1.patch. Thanks.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/187e63c535f0
https://hg.mozilla.org/integration/b2g-inbound/rev/d5e4d89832ed

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/187e63c535f0
https://hg.mozilla.org/mozilla-central/rev/d5e4d89832ed