Closed
Bug 1041347
Opened 10 years ago
Closed 10 years ago
Fake GMP plugin has to be accessible and installed for running mochitests using it
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox33 fixed, firefox34 fixed)
RESOLVED
FIXED
mozilla34
People
(Reporter: jesup, Assigned: ted)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
jesup
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•10 years ago
|
Whiteboard: [webrtc-uplift]
Updated•10 years ago
|
Blocks: WebRTC-OpenH264
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(gps)
Reporter | ||
Comment 1•10 years ago
|
||
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.)
Assignee | ||
Comment 2•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → ted
Assignee | ||
Comment 3•10 years ago
|
||
This needs a pass by the try server to test the packaged case and the Android changes.
Attachment #8460935 -
Flags: review?(jmaher)
Assignee | ||
Comment 4•10 years ago
|
||
Flags: needinfo?(gps)
Could we get this same change in testing/gtest? I have a GMP-related gtest on the way that can use the same plugin.
Blocks: 1039886
Assignee | ||
Comment 6•10 years ago
|
||
(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•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
(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.)
Assignee | ||
Comment 9•10 years ago
|
||
Also I have a revision of this, I'm punting Android support to a followup. I'll have it up shortly.
Assignee | ||
Comment 10•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8460935 -
Attachment is obsolete: true
Comment 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
I answered that in comment 8. Does that make sense?
Flags: needinfo?(jmaher)
Assignee | ||
Comment 13•10 years ago
|
||
Try build is better, but still broken on B2G desktop:
https://tbpl.mozilla.org/?tree=Try&rev=e8558856f5c2
Assignee | ||
Comment 15•10 years ago
|
||
Okay, less broken on b2g desktop now:
https://tbpl.mozilla.org/?tree=Try&rev=86131a602957
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Updated•10 years ago
|
Whiteboard: [webrtc-uplift] → [openh264-uplift]
Reporter | ||
Comment 16•10 years ago
|
||
Keywords: checkin-needed
Target Milestone: --- → mozilla34
Assignee | ||
Comment 17•10 years ago
|
||
Sorry about that, apparently I made one more change locally to get those B2G desktop tests passing but forgot to attach it here...
Assignee | ||
Updated•10 years ago
|
Attachment #8461619 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
Pushed the updated patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d32c7255e4b9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 20•10 years ago
|
||
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?
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8462495 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 21•10 years ago
|
||
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.
Assignee | ||
Comment 22•10 years ago
|
||
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.
Reporter | ||
Comment 23•10 years ago
|
||
Whiteboard: [openh264-uplift]
Reporter | ||
Comment 24•10 years ago
|
||
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)
Assignee | ||
Comment 25•10 years ago
|
||
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)
Comment 26•10 years ago
|
||
(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•10 years ago
|
||
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)
Reporter | ||
Comment 28•10 years ago
|
||
(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)
Assignee | ||
Comment 29•10 years ago
|
||
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)
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•