Closed Bug 1041347 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox33 fixed, firefox34 fixed)

RESOLVED FIXED
mozilla34
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: jesup, Assigned: ted)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Whiteboard: [webrtc-uplift]
Blocks: OpenH264
Flags: needinfo?(gps)
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.)
Yes, we build and enable the crashreporter in all builds (we have tests for it, even). I'll take a look at this today.
Assignee: nobody → ted
This needs a pass by the try server to test the packaged case and the Android changes.
Attachment #8460935 - Flags: review?(jmaher)
Could we get this same change in testing/gtest? I have a GMP-related gtest on the way that can use the same plugin.
(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?
Blocks: 1043403
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
Attachment #8460935 - Flags: review?(jmaher) → review+
(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.)
Also I have a revision of this, I'm punting Android support to a followup. I'll have it up shortly.
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.)
Attachment #8461619 - Flags: review?(jmaher)
Attachment #8460935 - Attachment is obsolete: true
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
Attachment #8461619 - Flags: review?(jmaher) → review+
I answered that in comment 8. Does that make sense?
Flags: needinfo?(jmaher)
Try build is better, but still broken on B2G desktop:
https://tbpl.mozilla.org/?tree=Try&rev=e8558856f5c2
oh, I missed that, thanks for clarifying.
Flags: needinfo?(jmaher)
Whiteboard: [webrtc-uplift] → [openh264-uplift]
Sorry about that, apparently I made one more change locally to get those B2G desktop tests passing but forgot to attach it here...
Attachment #8461619 - Attachment is obsolete: true
Depends on: 1044213
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
Attachment #8462495 - Flags: review+
Attachment #8462495 - Flags: approval-mozilla-aurora?
Flags: in-testsuite+
Attachment #8462495 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
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.
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?
Flags: needinfo?(ted)
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.
Flags: needinfo?(ted)
(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?
Depends on: 1057328
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.
Flags: needinfo?(rjesup)
(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.
Flags: needinfo?(rjesup) → needinfo?(ted)
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.
Flags: needinfo?(ted)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.