898696 - Fix some content/media test files by adding preload=metadata to media element

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Martijn Wargers (dead)]

Mobile and b2g have these prefs set:
pref("media.preload.default", 1); // default to preload none
pref("media.preload.auto", 2);    // preload metadata if preload=auto
They were added in bug 631058.

But they cause some failures in some content/media test files on Android and b2g.

It seems that this can be fixed by adding preload="metadata" to the media element.

Comment 1

[Martijn Wargers (dead)]

Attachment: 898696_preload.diff

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=e2d752481151

Comment 2

[Martijn Wargers (dead)]

This would add 2834 tests, btw.

Comment 3

[Martijn Wargers (dead)]

Attachment: 898696_preload.diff

content/media/test/test_bug448534.html and content/media/test/test_playback.html keep on failing on tryserver for b2g mochitest, so I readded those to the exclude list in this latest patch.
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=4b90ef17f648

Comment 4

[Martijn Wargers (dead)]

Attachment: 898696_preload.diff

content/media/test/test_media_selection.html is timing out on try, so put it back in the exclude list.
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=42be5cb7b63c

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

We try to set the default preload case to metadata here:

http://mxr.mozilla.org/mozilla-central/source/content/media/test/manifest.js#699

Why isn't that working for b2g?

We're likely to forget to add preload=metedata in new test that we create, so we're better to rely on a more robust automatic solution like the above...

Comment 6

[Martijn Wargers (dead)]

Oh, I see.
I guess that pref is set when the page (and video) is already loaded.The media element doesn't suddenly preload when that pref is switched, right?
This is also not working for Android.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

In most but not all cases we require manifest.js to be loaded before we start any media mochitests, so this pref change should be made before we start running tests... Is there another way we can enforce this pref change? Isn't there an #ifdef we can use to set prefs in a the profile we use for testing?

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

And it could also be that there's a bug here causing the pref change to stick. Maybe the push/pop actions of the pref changes are interleaving?

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Chris Pearce (:cpearce) from comment #8)
> And it could also be that there's a bug here causing the pref change to
> stick. Maybe the push/pop actions of the pref changes are interleaving?

I mean "*not* stick" of course. ;)

Comment 10

[Matthew Gregan [:kinetik]]

Apparently set*Pref doesn't work on B2g (see bug 889712 comment 3, for instance), so changing manifest.js to use pushPrefEnv is probably the best approach.

Comment 11

[Martijn Wargers (dead)]

set*Pref doesn work, but in b2g, this is done asynchronously, so to be sure the pref is set during a test, pushPrefEnv has to be used, but that has to be run from a callback.
That being said, I haven't really encountered issues, where set*Pref causes problems in b2g. But ideally, set*Pref shouldn't be used anymore, only pushPrefEnv.

I tried changing manifest.js to use pushPrefEnv before, but that means adjusting a lot of files, see bug 868439, comment 18 (but I'll have to to do that for the media.gstreamer.enabled and media.opus.enabled pref anyway).

If you don't see any problem in adding the prefs media.gstreamer.enabled, media.preload.default, media.preload.auto and media.opus.enabled to prefs_general.js, then I could add those there.
Although that means that the modified build that is being tested deviates more from the product that ships.

Comment 12

[Martijn Wargers (dead)]

(In reply to Chris Pearce (:cpearce) from comment #5)
> We're likely to forget to add preload=metedata in new test that we create,
> so we're better to rely on a more robust automatic solution like the above...

With pushPrefEnv, I'll have to change existing test files and new test files will have to use code that is setting these prefs, so anyone that's creating new tests will have to not forget that anyhow.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Please change your patch to use pushPrevEnv in manifest.js. In the tests that don't use manifest.js please set preload=metadata manually on the media elements.


(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #11)

> I tried changing manifest.js to use pushPrefEnv before, but that means
> adjusting a lot of files, see bug 868439, comment 18 

You're doing that anyway, right? ;)

> (but I'll have to to do
> that for the media.gstreamer.enabled and media.opus.enabled pref anyway).

media.opus.enabled defaults to true now anyway, so you can remove that code path from manifest.js.

I don't understand why you'd need to touch anything related to media.gstreamer.enabled. GStreamer is only used on Linux, not B2G, and it doesn't yet need to be enabled on tests that don't use manifest.js.


(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #12)
> With pushPrefEnv, I'll have to change existing test files and new test files
> will have to use code that is setting these prefs, so anyone that's creating
> new tests will have to not forget that anyhow.

In general new media mochitests should all be using the MediaTestManager from manifest.js, as this facilitates running tests across multiple backends/media types, and makes it easy to crank up the parallelism. Some tests don't need to use MediaTestManager, as they test things media-type-independent code, so don't benefit from running across multiple backends/media types.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

[11:23]	mwargers	cpearce, I'm a little bit confused by your answers. Every media test that has <scrip src=manifest.js> in it, is setting these prefs like media.gstreamer.enabled
[11:24]	cpearce	media.gstreamer.enabled only affects Linux. we're not shipping it enabled yet, but we want to test it.
[11:24]	cpearce	setting media.gstreamer.enabled on non linux platforms won't have any effect
[11:24]	mwargers	so that means, in order to get to use pushPrefEnv, I'll have to change every test that is linking to that manifest.js file
[11:24]	mwargers	ok
[11:24]	cpearce	mwargers: I don't understand why you'd have to change every test that links manifest.js
[11:24]	mihneadb	hi, what can I do to fix this: " 0:02.56 REFTEST TEST-UNEXPECTED-FAIL | WARNING: USE_WIDGET_LAYERS disabled "
[11:26]	mwargers	cpearce: because pushPrefEnv works like this: pushPrevEnv('set some prefs in here', resulting_function_that_is_called_after_these_prefs_have_been_set)
[11:27]	cpearce	mwargers: oh, you can have MediaTestManager.runTests only run if the pref setter has run, and the pref setter can do what runTests does if runTests has been called.
[11:28]	cpearce	mwargers: I thought you did something like that based on your comment https://bugzilla.mozilla.org/show_bug.cgi?id=868439#c18 ?
[11:28]	mwargers	cpearce, there are some media test files out there that only link to manifest.js, but don't call MediaTestManager.runTests
[11:29]	mwargers	yeah
[11:29]	mwargers	that's where I describe this problem
[11:29]	cpearce	mwargers: are those the files you list in that comment?
[11:29]	mwargers	yes
[11:29]	cpearce	I see....
[11:30]	tbsaunde	glandium: bug 899368 fwiw
[11:30]	firebot	Check-in: http://hg.mozilla.org/integration/mozilla-inbound/rev/505ad1c506cc - Dan Gohman - Bug 898461 - IonMonkey: Move div and mod overflow and divide-by-zero handling code out of line. r=jandem
[11:32]	glandium	tbsaunde: so, yeah, i did stick constexpr on the wrong GetCCParticipant.
[11:32]	tbsaunde	that would do it
[11:33]	tbsaunde	I did both that I could at first, but the other one doesn't help so I'm not sure there's a point
[11:33]	cpearce	mwargers: ok, we can put preload=metadata on every media element.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 782201 [details] [diff] [review]
898696_preload.diff

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

Also, please do this for the mochitests that are disabled in Makefile.in. We're trying to get them re-enabled.

Comment 16

[Martijn Wargers (dead)]

Attachment: restofpreload.diff

(In reply to Chris Pearce (:cpearce) from comment #15)
> Also, please do this for the mochitests that are disabled in Makefile.in.
> We're trying to get them re-enabled.

Afaik, these are the mochitests that are disabled that also would need this treatment.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 783437 [details] [diff] [review]
restofpreload.diff

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

Thanks!

Comment 18

[Martijn Wargers (dead)]

Attachment: Combined, ready for check-in

Just to be sure, another try run with this one: https://tbpl.mozilla.org/?tree=Try&rev=046f7d5df8fe

Comment 19

[Martijn Wargers (dead)]

Try server again green.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/421861049947

Would have been nice if this was rebased on top of bug 898693, by the way...

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/421861049947

Comment 22

[Martijn Wargers (dead)]

Ryan, sorry for that. I don't know how hg rebase works, I barely know how to work with mq.

Comment 23

[Martijn Wargers (dead)]

I filed bug 902686 for changing manifest.js to use SpecialPowers.pushPrefEnv.