Randell Jesup [:jesup] (needinfo me) Assignee
	Description • 10 years ago

We have support functions in webrtc (VcmSIPCCBinding.cpp) for addVideoCodecsGmp(mask), and I'm going to add a removeVideoCodecsGmp(mask) for enabling specific GMP video codecs in webrtc.  Right now, nothing calls these, and we override that with the h264_enabled pref.

We need to hook webrtc up to the plugin code so we can find out what is available (before the first call), and so we can handle user revocation of the codec without restarting.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 2 • 10 years ago

Comment on attachment 8456966 [details] [diff] [review]
Query GMPService to determine if H.264 is available

While calling the functions to cache knowledge of codecs in SIPCC would be nice, it's a bunch of extra work to synchronize, so defer or punt on that.  Added an interface to query if a codec is supported without having to proxy to GMP thread (use the mutex for mPlugins access).

There's still one issue of the scanning is async, and we want to know if the scan is done before looking for them.  Worked around that for now with a sync dispatch to GMP thread that does nothing to force synchronization, at the cost of a one-time block of mainthread on first use.  We'll need a followuup bug to resolve that (and preferably uplift that followup to 33).

Also fixes that it was looking for a "vp8" codec, not h264 8-/

	Ethan Hugg [:ehugg]
	Comment 3 • 10 years ago

Comment on attachment 8456966 [details] [diff] [review]
Query GMPService to determine if H.264 is available

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

This looks fine to me.  I gave it a quick test on OSX and also was able to remove 'vp8' from the gmpopenh264.info file.  I will push that change to openh264 along with the upcoming API changes.

	Benjamin Smedberg
	Comment 4 • 10 years ago

Comment on attachment 8456966 [details] [diff] [review]
Query GMPService to determine if H.264 is available

I'd like to know what we plan on using mScanIsDone for (it doesn't appear to be used in this patch) and on which threads (is it protected by the mutex?). 

Note that MOZ_GMP_PATH is only scanned for development: OpenH264 is going to be added programmatically by the addon manager. I'm not sure we've figured out whether that's going to happen at startup. If so, is the memory/CPU footprint of launching the GMP service a problem? If not, we'll need a signal (probably a global observer notification) for the GMP service to tell the addon manager provider to register any builtin GMP plugins.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 5 • 10 years ago

Comment on attachment 8456966 [details] [diff] [review]
Query GMPService to determine if H.264 is available

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

mScanIsDone was to support future work, and probably is premature (thought I'd need it, but managed to work around the need).  It would be protected by the Mutex I think (and I placed it in the normal "protected-by-mutex-at-top" block.

Thanks for the update on where the load will occur later.  In any case, I just need to be able to query the list and know if plugin is there.

Biggest cost to early enumeration is a thread I think.  Perhaps kill the thread after early enumeration, and start it later as needed (and maybe kill it if it's idle longer than X and there are no open users).  For desktop, though, these optimizations are moot, and there's no immediate plan to land these on b2g/android (though we might - HW encoders often come with serious limitations).

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 6 • 10 years ago

Comment on attachment 8456966 [details] [diff] [review]
Query GMPService to determine if H.264 is available

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

r- because this is vulnerable to thread races.

::: content/media/gmp/GMPService.cpp
@@ +345,5 @@
> +  NS_ENSURE_ARG(aTags && aTags->Length() > 0);
> +  NS_ENSURE_ARG(aResult);
> +
> +  nsCString temp(aAPI);
> +  GMPParent *parent = SelectPluginForAPI(aOrigin, temp, *aTags);

If the main thread gets the GMPService the first time and immediately calls HasPluginForAPI(), then you'll be racing with the GMP thread to populate the list of plugins, so the result could be wrong. You need some way (wait on a monitor until mScanIsDone or shutdown perhaps?) to block the main thread until the mPlugin list is populated.

::: content/media/gmp/GMPService.h
@@ +74,4 @@
>    nsCOMPtr<nsIThread> mGMPThread;
>    bool mShuttingDown;
>    bool mShuttingDownOnGMPThread;
> +  bool mScanIsDone;  // Initial scan of GMP dirs is complete

This is write only, why is it needed? Please remove it unless you need it.

Also, the origin segregation code (which has been regressed because I never had time to write a test) depended on being able to scan multiple times. So assuming we're only going to do one scan may not be the best idea going forward. That'll probably only become clear once I fix the origin segregation code again...

::: content/media/gmp/mozIGeckoMediaPluginService.idl
@@ +33,5 @@
> +   * Get a plugin that supports the specified tags.
> +   * Callable on any thread
> +   */
> +  [noscript]
> +  boolean hasPluginForAPI([optional] in AString origin,

Because you're changing the binary interface of mozIGeckoMediaPluginService, you need to also run `./mach update-uuids mozIGeckoMediaPluginService` to ensure that the uuid of this interface and any that inherit it are udpated. Otherwise things that expect the old interface/uuid won't know its vtable has changed.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 7 • 10 years ago

(In reply to Chris Pearce (:cpearce) from comment #6)
> Comment on attachment 8456966 [details] [diff] [review]
> Query GMPService to determine if H.264 is available
> 
> Review of attachment 8456966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- because this is vulnerable to thread races.
> 
> ::: content/media/gmp/GMPService.cpp
> @@ +345,5 @@
> > +  NS_ENSURE_ARG(aTags && aTags->Length() > 0);
> > +  NS_ENSURE_ARG(aResult);
> > +
> > +  nsCString temp(aAPI);
> > +  GMPParent *parent = SelectPluginForAPI(aOrigin, temp, *aTags);
> 
> If the main thread gets the GMPService the first time and immediately calls
> HasPluginForAPI(), then you'll be racing with the GMP thread to populate the
> list of plugins, so the result could be wrong. You need some way (wait on a
> monitor until mScanIsDone or shutdown perhaps?) to block the main thread
> until the mPlugin list is populated.

That's the point of the (ugly) dummy DISPATCH_SYNC to GMP thread - to force mainthread to wait for it to be populated.

> 
> ::: content/media/gmp/GMPService.h
> @@ +74,4 @@
> >    nsCOMPtr<nsIThread> mGMPThread;
> >    bool mShuttingDown;
> >    bool mShuttingDownOnGMPThread;
> > +  bool mScanIsDone;  // Initial scan of GMP dirs is complete
> 
> This is write only, why is it needed? Please remove it unless you need it.

Right; no longer needed (at least for now).

> 
> Also, the origin segregation code (which has been regressed because I never
> had time to write a test) depended on being able to scan multiple times. So
> assuming we're only going to do one scan may not be the best idea going
> forward. That'll probably only become clear once I fix the origin
> segregation code again...

I agree this will need to be revisited (and I'd love to get rid of the DISPATCH_SYNC); per bsmedberg this will all change anyways, so I'm keeping things simple for now.

> 
> ::: content/media/gmp/mozIGeckoMediaPluginService.idl
> @@ +33,5 @@
> > +   * Get a plugin that supports the specified tags.
> > +   * Callable on any thread
> > +   */
> > +  [noscript]
> > +  boolean hasPluginForAPI([optional] in AString origin,
> 
> Because you're changing the binary interface of mozIGeckoMediaPluginService,
> you need to also run `./mach update-uuids mozIGeckoMediaPluginService` to
> ensure that the uuid of this interface and any that inherit it are udpated.
> Otherwise things that expect the old interface/uuid won't know its vtable
> has changed.

Yup. (Though I seriously doubt anyone has hard-coded the UUID for GMP yet).  Thanks

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 8 • 10 years ago

(In reply to Randell Jesup [:jesup] from comment #7)
> (In reply to Chris Pearce (:cpearce) from comment #6)
> > Comment on attachment 8456966 [details] [diff] [review]
> > Query GMPService to determine if H.264 is available
> > 
> > Review of attachment 8456966 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r- because this is vulnerable to thread races.
> > 
> > ::: content/media/gmp/GMPService.cpp
> > @@ +345,5 @@
> > > +  NS_ENSURE_ARG(aTags && aTags->Length() > 0);
> > > +  NS_ENSURE_ARG(aResult);
> > > +
> > > +  nsCString temp(aAPI);
> > > +  GMPParent *parent = SelectPluginForAPI(aOrigin, temp, *aTags);
> > 
> > If the main thread gets the GMPService the first time and immediately calls
> > HasPluginForAPI(), then you'll be racing with the GMP thread to populate the
> > list of plugins, so the result could be wrong. You need some way (wait on a
> > monitor until mScanIsDone or shutdown perhaps?) to block the main thread
> > until the mPlugin list is populated.
> 
> That's the point of the (ugly) dummy DISPATCH_SYNC to GMP thread - to force
> mainthread to wait for it to be populated.

Ah, so then you're leaking the details of the implementation into the client code, which is bad design.

But even with your sync dispatch to GMPService::GetThread() you're still racing, as GMPService::GetThread() does an async dispatch to the GMP thread to LoadFromEnvironment(), which is the bit that actually populates mPlugins. Right?

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 9 • 10 years ago

(In reply to Chris Pearce (:cpearce) from comment #8)
> (In reply to Randell Jesup [:jesup] from comment #7)
> > (In reply to Chris Pearce (:cpearce) from comment #6)
> > > Comment on attachment 8456966 [details] [diff] [review]
> > > Query GMPService to determine if H.264 is available
> > > 
> > > Review of attachment 8456966 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > r- because this is vulnerable to thread races.
> > > 
> > > ::: content/media/gmp/GMPService.cpp
> > > @@ +345,5 @@
> > > > +  NS_ENSURE_ARG(aTags && aTags->Length() > 0);
> > > > +  NS_ENSURE_ARG(aResult);
> > > > +
> > > > +  nsCString temp(aAPI);
> > > > +  GMPParent *parent = SelectPluginForAPI(aOrigin, temp, *aTags);
> > > 
> > > If the main thread gets the GMPService the first time and immediately calls
> > > HasPluginForAPI(), then you'll be racing with the GMP thread to populate the
> > > list of plugins, so the result could be wrong. You need some way (wait on a
> > > monitor until mScanIsDone or shutdown perhaps?) to block the main thread
> > > until the mPlugin list is populated.
> > 
> > That's the point of the (ugly) dummy DISPATCH_SYNC to GMP thread - to force
> > mainthread to wait for it to be populated.
> 
> Ah, so then you're leaking the details of the implementation into the client
> code, which is bad design.

I wasn't claiming a paragon of design :-).  Especially given bsmedberg's comments that the initial load will likely change later this week or next anyways.

> 
> But even with your sync dispatch to GMPService::GetThread() you're still
> racing, as GMPService::GetThread() does an async dispatch to the GMP thread
> to LoadFromEnvironment(), which is the bit that actually populates mPlugins.
> Right?

No.  Init() pokes GetThread(), which starts LoadFromEnvironment(), which is in the GMP thread's event queue before Init() returns.  Then scanForGmpPlugins() does a DISPATCH_SYNC to GMPThread, which is in the queue behind LoadFromEnvironment(). (For that matter, even if we didn't poke it in Init(), this would still be safe due to that.)

Only after that do we do HasPluginForAPI().

I could have made a LoadPlugins() method to do WrapRunnable() to which (in practice) was a no-op but served to synchronize, but again, given the upcoming changes seemed like unneeded busy-work.

So, today, it safely makes sure the plugins are loaded before it checks the first time.  It does briefly block mainthread on first use (maybe extremely briefly).

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 10 • 10 years ago

Comment on attachment 8456966 [details] [diff] [review]
Query GMPService to determine if H.264 is available

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

Ok, you convinced me the way you're calling it is threadsafe.

We should really pre-load the metadata for the GMPs sometime during startup or earlier, as we'll need to query these in HTMLMediaElement.canPlayType(), which is synchronous too, and it would be nicer if we could not require such detailed knowledge of the threading model, just to call hasPluginForAPI.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 11 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d976c67e926

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 12 • 10 years ago

nsThread leak, backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5be9f365771

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 13 • 10 years ago

fix verified; r+ cpearce on IRC
https://hg.mozilla.org/integration/mozilla-inbound/rev/bddf7a4c0ac0

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 14 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b623f53cdac1
And one can't use DISPATCH_SYNC from a non-nsThread without risking Runnable/nsThread leaks
(We really should add code in Dispatch()/nsThreadManager to flag that case)

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 15 • 10 years ago

That was r=cpearce

	Georg Fritzsche [:gfritzsche]
	Comment 16 • 10 years ago

Comment on attachment 8456966 [details] [diff] [review]
Query GMPService to determine if H.264 is available

I only scanned through this quickly as this is already landed and seems to have good review coverage.

Is there no test-coverage for these parts or are they happening elsewhere on a higher level?

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 17 • 10 years ago

> Is there no test-coverage for these parts or are they happening elsewhere on
> a higher level?

These are hit by every test that does webrtc calls (dom/media/tests/*), since we check for H.264 availability on every call.

Testing for the positive case is being done by hand, and will be done in the tree when the fake h.264 plugin lands for testing the rest of the GMP mechanics in-tree (we can't easily download external plugins in tbpl).

	Carsten Book [:Tomcat]
	Comment 18 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/bddf7a4c0ac0
https://hg.mozilla.org/mozilla-central/rev/b623f53cdac1