1041347 - Fake GMP plugin has to be accessible and installed for running mochitests using it

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

In bug 1037125 and bug 1040436, we created a fake GMP plugin and test infrastructure to use it.  The tricky part was referencing it for MOZ_GMP_PATH so tests could use it, since we need an absolute filesystem path.  runtests.py knows where the executable dir is, so for manual testing it points MOZ_GMP_PATH to $(BIN)/gmp-fake.  However, that doesn't help for automated tests - it needs to get included in the support files for testing, and then runtests.py needs to know how to find it and set an absolute path to it.

1040436 landed disabled for this reason (manually runnable and green) until this is resolved.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

We wont to get this enabled in automation soon, to catch things like bug 1041525 (does crashreporter get built in any of the builds for TBPL tests on inbound?  On m-c?  The nightlies have it.)

Comment 2

[(not currently active) Ted Mielczarek]

Yes, we build and enable the crashreporter in all builds (we have tests for it, even). I'll take a look at this today.

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: Package and provide path to fake GMP plugin for Mochitests

This needs a pass by the try server to test the packaged case and the Android changes.

Comment 4

[(not currently active) Ted Mielczarek]

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=6679046e4795

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Could we get this same change in testing/gtest? I have a GMP-related gtest on the way that can use the same plugin.

Comment 6

[(not currently active) Ted Mielczarek]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #5)
> Could we get this same change in testing/gtest? I have a GMP-related gtest
> on the way that can use the same plugin.

That is a whole other kettle of fish. File a separate bug?

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8460935 [details] [diff] [review]
Package and provide path to fake GMP plugin for Mochitests

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

all is looking well here, just a couple of questions.

::: testing/mochitest/runtests.py
@@ +1161,5 @@
> +    gmp_path = self.getGMPPluginPath(options)
> +    if gmp_path is None:
> +      log.error('Could not find path to gmp-fake plugin!')
> +      return None
> +    browserEnv["MOZ_GMP_PATH"] = gmp_path

so if we don't have gmp path, do we assume everything else doesn't work?  I don't see why you are returning None here.

::: testing/mochitest/runtestsremote.py
@@ +565,5 @@
> +            self._dm.pushDir(local_gmp_path, remote_gmp_path)
> +        except devicemanager.DMError:
> +            log.error("Automation Error: Unable to copy gmp-fake to device.")
> +            return None
> +        return remote_gmp_path

it is too bad we can't just do:
localPath = Mochitest.getGMPPluginPath(self, options)
if not localPath:
    return None
remotePath = os.path.join(self._dm.deviceRoot, 'gmp-fake')
try:
    self._dm.pushDir(localPath, remotePath)
except:
    return None
return remotePath

Comment 8

[(not currently active) Ted Mielczarek]

(In reply to Joel Maher (:jmaher) from comment #7)
> Comment on attachment 8460935 [details] [diff] [review]
> Package and provide path to fake GMP plugin for Mochitests
> 
> Review of attachment 8460935 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> all is looking well here, just a couple of questions.
> 
> ::: testing/mochitest/runtests.py
> @@ +1161,5 @@
> > +    gmp_path = self.getGMPPluginPath(options)
> > +    if gmp_path is None:
> > +      log.error('Could not find path to gmp-fake plugin!')
> > +      return None
> > +    browserEnv["MOZ_GMP_PATH"] = gmp_path
> 
> so if we don't have gmp path, do we assume everything else doesn't work?  I
> don't see why you are returning None here.

This is built in-tree, so it should always be available. This is a sanity check to make sure we error with a clear message if something breaks. (Instead of just having tests that depend on it fail.)

Comment 9

[(not currently active) Ted Mielczarek]

Also I have a revision of this, I'm punting Android support to a followup. I'll have it up shortly.

Comment 10

[(not currently active) Ted Mielczarek]

Attachment: Package and provide path to fake GMP plugin for Mochitests

Slight rework, I'm pushing Android support out to a followup so I made not finding the plugin only fatal on desktop Mochitest. (It turns out Android tests don't pass the plugins dir to --extra-profile-files, so I have to work out another way to make that work if we want it.)

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8461619 [details] [diff] [review]
Package and provide path to fake GMP plugin for Mochitests

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

::: testing/mochitest/runtests.py
@@ +1164,5 @@
> +      if gmp_path is not None:
> +          browserEnv["MOZ_GMP_PATH"] = gmp_path
> +    except EnvironmentError:
> +      log.error('Could not find path to gmp-fake plugin!')
> +      return None

in my last review I wanted to know why we were returning None here and terminating flow in the program if gmp plugin was not found

Comment 12

[(not currently active) Ted Mielczarek]

I answered that in comment 8. Does that make sense?

Comment 13

[(not currently active) Ted Mielczarek]

Try build is better, but still broken on B2G desktop:
https://tbpl.mozilla.org/?tree=Try&rev=e8558856f5c2

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

oh, I missed that, thanks for clarifying.

Comment 15

[(not currently active) Ted Mielczarek]

Okay, less broken on b2g desktop now:
https://tbpl.mozilla.org/?tree=Try&rev=86131a602957

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0356454faea3

Comment 17

[(not currently active) Ted Mielczarek]

Attachment: Package and provide path to fake GMP plugin for Mochitests

Sorry about that, apparently I made one more change locally to get those B2G desktop tests passing but forgot to attach it here...

Comment 18

[(not currently active) Ted Mielczarek]

Pushed the updated patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d32c7255e4b9

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8462495 [details] [diff] [review]
Package and provide path to fake GMP plugin for Mochitests

Approval Request Comment
[Feature/regressing bug #]:N/A

[User impact if declined]: No testing of GMP plugin loading and exeuction on TBPL

[Describe test coverage new/current, TBPL]: This is the testing :-)

[Risks and why]: None - test only.

[String/UUID change made/needed]: none

Comment 21

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

Does this allow us to use different GMPs for different mochitests? We need to be able to test the ClearKey GMP for EME, which will be completely different from the one used for WebRTC tests.

Comment 22

[(not currently active) Ted Mielczarek]

As implemented this only sets the path to gmp-fake. The GMPService code supports multiple paths in the environment var:
http://hg.mozilla.org/mozilla-central/annotate/55c4d770f88b/content/media/gmp/GMPService.cpp#l348

so once we have ClearKey in the tree it should be simple to modify the Mochitest code to add its path to the env var.

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/debfa7a18dff

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

ted: can we load different versions (which provide the same codecs) for some tests, so we could load a "crashes after N frames" version for one test, and a "doesn't crash" for a different test?

Comment 25

[(not currently active) Ted Mielczarek]

I don't know enough about the GMP loading machinery to say anything useful about that. Right now the harness is just pointing that env var before starting the browser.

Comment 26

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

(In reply to Randell Jesup [:jesup] from comment #24)
> ted: can we load different versions (which provide the same codecs) for some
> tests, so we could load a "crashes after N frames" version for one test, and
> a "doesn't crash" for a different test?

If you can get a tag into the GMPService::GetGMPVideoDecoder() that distinguished between the crashing and non-crashing plugin you could. We can do this easily enough for EME, as we can just pass a different EME KeySystem string to the EME JS APIs. But for WebRTC I assume you'd need to create a hole in your API for this? Maybe a something on the API surface that specifies the type of codec you're using?

Comment 27

[Benjamin Smedberg]

I don't understand what was broken that required this change. Why are we using MOZ_GMP_PATH at all? I thought that the way this was set up, each test that needs the fake plugin will register it from the support-files and then unregister it when the test is done.

I'm disappointed that we had to put this into $(PKG_STAGE)/bin/plugins instead of using normal support-files machinery.

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #27)
> I don't understand what was broken that required this change. Why are we
> using MOZ_GMP_PATH at all? I thought that the way this was set up, each test
> that needs the fake plugin will register it from the support-files and then
> unregister it when the test is done.

When this bug was filed, the fake GMP plugin wasn't in the test package at all - and there was no download/install mechanism IIRC.  If there's now a better way to dynamically expose a GMP plugin to a test, great! - let's file a bug to switch.

> I'm disappointed that we had to put this into $(PKG_STAGE)/bin/plugins
> instead of using normal support-files machinery.

I'll defer to Ted on that.  I didn't think support-files could install things into the profile, though, but honestly I don't know.

Comment 29

[(not currently active) Ted Mielczarek]

support-files doesn't have any mechanism for installing files from the objdir, was the sticking point. Given that, and the fact that someone had already implemented MOZ_GMP_PATH, this was the simplest path to success.