957928 - Gecko Media Plugins (GMP) support

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Josh Aas]

We need a plugin architecture that will allow us to run media plugins on all platforms (e.g. Cisco openh264). During planning discussions we started calling these plugins Gecko Media Plugins (GMPs).

GMPs run out of process, and we are planning to sandbox them. This bug is about getting an implementation that will run out-of-process GMPs in place, short of sandboxing. Sandboxing will happen in another bug.

I will work on putting my local documentation re: APIs and usage online soon. I'll also be posting more information about how GMPs work with my first patch.

Comment 1

[Josh Aas]

Attachment: Impl v1.0

Notes on this initial patch:

1) Works on OS X, Linux, and Windows.

2) The video decoding API is a stub - parent sends DecodeVideo(int32_t videoID) to child, child sends DecodeVideoComplete(int32_t videoID) to parent. That's it. Chris Pearce is going to be working on a real video decoder API.

3) This is hooked up to the MediaDecoder object for testing. You can test by going to this URL (http://www.w3.org/2010/05/video/mediaevents.html), then hit play on the video. When you hit play a decoder child process will spin up and do fake decoding. When you hit pause the GMP process will shut down. My choice of pause was arbitrary, I just needed a button to invoke shutdown.

4) A consumer of the video decoding API would get the gecko media plugin service, request a decoder API object, set an observer on the object (for decoding callbacks), and then use the API via the decoder API object. When the consumer is finished they call "Destroy" on the decoder API object, and when no API objects are extant for a plugin it shuts down its child process.

5) There is a directory in the source which contains header files for the plugin APIs (gmp-api). These are used by the host and any plugins.

Comment 2

[Josh Aas]

Attachment: Sample GMP v1.0

This is a sample Gecko Media Plugin. It pretends to be an h264 decoder, but does not actually do any work. It only prints out messages when its methods are called and makes any expected callbacks. The build script is for OS X - it in't be hard to modify it to create a shared library for any other OS.

Comment 3

[Josh Aas]

Attachment: Impl v1.1

Update name of GMP get API function and move to 2-clause BSD for GMP headers.

Comment 4

[Josh Aas]

Attachment: Sample GMP v1.1

Comment 5

[Josh Aas]

Documentation is here:

https://wiki.mozilla.org/GeckoMediaPlugins

Comment 6

[Josh Aas]

Attachment: Impl v1.2

Updated patch against current mozilla-central.

Development is happening here:

https://github.com/bdaehlie/gecko-dev

Currently on the gmp3 branch.

Comment 7

[Josh Aas]

Attachment: Impl v1.3

Ready to start review. This currently has no consumers in Gecko code, so if it were landed it would be entirely un-used, with nothing accessible from JS.

The biggest known issue with this code is that the GMP API itself is in need of review/revision. What we have now is good enough to work with, but the actual API we ship will be revised in mostly minor ways (such as dropping VP8 in favor of something else (e.g. openh264). Chris Pearce and Eric Rescorla will help to figure this out.

Tests are part of the WebRTC consumer code right now (much like how NPAPI code is tested via its DOM/content consumer). That will land separately.

Comment 8

[Ethan Hugg [:ehugg]]

It would be useful to have some logging for failures in loading or initializing a GMP.  Most of the GMPService methods for example return on error without logging any reason information.  This makes it difficult to diagnose problems with the plugin without using the debugger.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8399671 [details] [diff] [review]
Impl v1.3

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

Not done, just wanted to get a bit of feedback out now since I have some threading concerns with GMPService that might be complicated to solve (it looks like it does main thread I/O currently, and has some thread-unsafe modifications to the plugins array).

::: content/media/gmp/GMPParent.cpp
@@ +44,5 @@
> +  return ReadGMPMetaData();
> +}
> +
> +nsresult
> +GMPParent::LoadProcess()

Hrm, there's no protection here if the process previously loaded and then crashed... It looks like you'll try to recreate it here. Have you tested that? I'm not sure that IPDL can reuse actors like this.

::: content/media/gmp/GMPParent.h
@@ +28,5 @@
> +  nsCString mAPIName;
> +  nsTArray<nsCString> mAPITags;
> +};
> +
> +class GMPParent : public nsISupports,

This doesn't need to be nsISupports-derived, does it? I think you probably just want refcounting with NS_INLINE_DECL_REFCOUNTING

::: content/media/gmp/GMPService.cpp
@@ +14,5 @@
> +namespace gmp {
> +
> +NS_IMPL_ISUPPORTS2(GeckoMediaPluginService, mozIGeckoMediaPluginService, nsIObserver)
> +
> +GeckoMediaPluginService::GeckoMediaPluginService()

Since we're doing a bunch of thread gymnastics with this patch I suggest asserting the thread you expect to run on everywhere you can. It makes reading the code much easier and it also catches tons of bugs. A simple |MOZ_ASSERT(NS_IsMainThread())| is what I'm looking for.

@@ +19,5 @@
> +: mThreadCreateMutex("gmp-thread-create")
> +{
> +  nsCOMPtr<nsIObserverService> obsService = mozilla::services::GetObserverService();
> +  if (obsService) {
> +    obsService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);

This isn't safe, you're calling an XPCOM method before you have a non-zero refcount. I suggest a static Create function that does things safely.

@@ +35,5 @@
> +{
> +  if (!strcmp(NS_XPCOM_SHUTDOWN_OBSERVER_ID, aTopic)) {
> +    if (mGMPThread) {
> +      mGMPThread->Dispatch(NS_NewRunnableMethod(this, &GeckoMediaPluginService::UnloadPlugins),
> +                           NS_DISPATCH_SYNC);

Should you maybe set a flag here so that you don't try to create or hand out a thread after this notification? And how about handing out encoders/decoders?

Also, calling UnloadPlugins will iterate and modify the |mPlugins| array on another thread. You don't currently have any kind of protection against concurrent access to this array that I can see.

@@ +46,5 @@
> +GeckoMediaPluginService::GetThread(nsIThread** aThread)
> +{
> +  MOZ_ASSERT(aThread, "Must pass valid out ptr!");
> +
> +  MutexAutoLock lock(mThreadCreateMutex);

It's kinda dangerous to lock while calling into code you don't control... But since the service doesn't seem to be needed off the main thread you can probably just lose all this.

@@ +62,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +GeckoMediaPluginService::GetGMPVideoDecoderVP8(GMPVideoHost** outVideoHost, GMPVideoDecoder** gmpVD)

Params always get an 'a' prefix nowadays.

@@ +64,5 @@
> +
> +NS_IMETHODIMP
> +GeckoMediaPluginService::GetGMPVideoDecoderVP8(GMPVideoHost** outVideoHost, GMPVideoDecoder** gmpVD)
> +{
> +  *gmpVD = nullptr;

In general no need to set outparams unless you return a success code.

@@ +66,5 @@
> +GeckoMediaPluginService::GetGMPVideoDecoderVP8(GMPVideoHost** outVideoHost, GMPVideoDecoder** gmpVD)
> +{
> +  *gmpVD = nullptr;
> +
> +  nsCOMPtr<GMPParent> gmp = SelectPluginForAPI(NS_LITERAL_CSTRING("decode-video"),

This should be nsRefPtr.

@@ +89,5 @@
> +GeckoMediaPluginService::GetGMPVideoEncoderVP8(GMPVideoHost** outVideoHost, GMPVideoEncoder** gmpVE)
> +{
> +  *gmpVE = nullptr;
> +
> +  nsCOMPtr<GMPParent> gmp = SelectPluginForAPI(NS_LITERAL_CSTRING("encode-video"),

nsRefPtr

@@ +240,5 @@
> +nsresult
> +GeckoMediaPluginService::SearchDirectory(nsIFile* aSearchDir)
> +{
> +  if (!aSearchDir) {
> +    return NS_ERROR_INVALID_ARG;

Can this just be asserted?

@@ +244,5 @@
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  nsCOMPtr<nsISimpleEnumerator> iter;
> +  nsresult rv = aSearchDir->GetDirectoryEntries(getter_AddRefs(iter));

This will happen on the main thread, right? We really shouldn't be adding any more main thread I/O, even for directory traversal.

@@ +285,5 @@
> +  if (NS_FAILED(rv)) {
> +    return;
> +  }
> +
> +  nsString prefix = NS_LITERAL_STRING("gmp-");

NS_NAMED_LITERAL_STRING

@@ +287,5 @@
> +  }
> +
> +  nsString prefix = NS_LITERAL_STRING("gmp-");
> +  if (leafName.Length() <= prefix.Length() ||
> +      !Substring(leafName, 0, prefix.Length()).Equals(prefix)) {

To be clear, you do care about case-sensitivity here?

@@ +297,5 @@
> +  if (NS_FAILED(rv)) {
> +    return;
> +  }
> +
> +  mPlugins.AppendElement(gmp);

So it looks to me like things that are added to this list are never released until shutdown.

@@ +314,5 @@
> +    SearchDirectory(searchDirs[i]);
> +  }
> +
> +  nsTArray<nsCOMPtr<nsIFile>> possiblePlugins;
> +  rv = GetPossiblePlugins(possiblePlugins);

It looks like this only does stuff on Windows, right? But on my machine those reg entries point to npapi *files*, not directories. The ProcessPossiblePlugin call below will do nothing with them since they are not directories... Something seems wrong here.

::: content/media/gmp/GMPService.h
@@ +11,5 @@
> +#include "nsIObserver.h"
> +#include "nsTArray.h"
> +#include "nsIFile.h"
> +#include "mozilla/Mutex.h"
> +#include "nsIThread.h"

A bunch of these should be forward-declared.

@@ +16,5 @@
> +
> +namespace mozilla {
> +namespace gmp {
> +
> +class GeckoMediaPluginService : public mozIGeckoMediaPluginService,

Mark this MOZ_FINAL, and then you can have a non-virtual destructor.

@@ +22,5 @@
> +{
> +public:
> +  GeckoMediaPluginService();
> +
> +  NS_DECL_THREADSAFE_ISUPPORTS

Does this actually need to be threadsafe?

::: content/media/gmp/GMPTypes.ipdlh
@@ +11,5 @@
> +  Shmem mBuffer;
> +  uint32_t mEncodedWidth;
> +  uint32_t mEncodedHeight;
> +  uint32_t mTimeStamp;
> +  int64_t mCaptureTime_ms;

Might want to reorder this so it packs better

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ok, Josh set me straight on the intended thread mechanisms here, looks like main thread IO isn't a problem really.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

FYI I'm not really a good reviewer for the actual gmp-api parts. You'll need to find someone else for that I think...

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8399671 [details] [diff] [review]
Impl v1.3

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

::: content/media/gmp/GMPChild.cpp
@@ +33,5 @@
> +               MessageLoop* aIOLoop,
> +               IPC::Channel* aChannel)
> +{
> +  return LoadPluginLibrary(aPluginPath) &&
> +         Open(aChannel, aParentProcessHandle, aIOLoop);

A static Create() function might be nicer here too. You need to unload the library if Open fails I believe.

@@ +39,5 @@
> +
> +bool
> +GMPChild::LoadPluginLibrary(const std::string& aPluginPath)
> +{
> +  nsAutoCString pluginPath(aPluginPath.c_str());

nsDependentCString

@@ +42,5 @@
> +{
> +  nsAutoCString pluginPath(aPluginPath.c_str());
> +
> +  nsCOMPtr<nsIFile> libFile;
> +  NS_NewNativeLocalFile(pluginPath, true, getter_AddRefs(libFile));

This needs to check for errors or null.

Also, there are problematic issues with 'native' paths on windows at least. Please have bsmedberg look over this part, I think he knows what should and should not happen here.

@@ +73,5 @@
> +  if (!initFunc) {
> +    return false;
> +  }
> +
> +  auto platformAPI = new GMPPlatformAPI();

Who deletes this?

@@ +94,5 @@
> +
> +void
> +GMPChild::ActorDestroy(ActorDestroyReason why)
> +{
> +  if (mLib) {

Shouldn't this get freed somehow?

::: content/media/gmp/GMPMessageUtils.h
@@ +19,5 @@
> +  {
> +    WriteParam(aMsg, aParam.mPictureLossIndicationOn);
> +    WriteParam(aMsg, aParam.mFeedbackModeOn);
> +    WriteParam(aMsg, static_cast<int>(aParam.mComplexity));
> +    WriteParam(aMsg, static_cast<int>(aParam.mResilience));

You should use the ContiguousEnumSerializer helper for this. It checks the range automatically so that the parent won't crash if the child sends bad data. You need to do this for all your enums.

@@ +111,5 @@
> +
> +  static void Write(Message* aMsg, const paramType& aParam)
> +  {
> +    WriteParam(aMsg, static_cast<int>(aParam.mCodecType));
> +    WriteParam(aMsg, nsAutoCString(aParam.mPLName));

This copies, use nsDependentCString

@@ +142,5 @@
> +    aResult->mCodecType = static_cast<GMPVideoCodecType>(codecType);
> +
> +    nsAutoCString plName;
> +    if (!ReadParam(aMsg, aIter, &plName) ||
> +        plName.Length() > kGMPPayloadNameSize) {

You actually need kGMPPayloadNameSize - 1, because you always have to ensure null termination here.

@@ +146,5 @@
> +        plName.Length() > kGMPPayloadNameSize) {
> +      return false;
> +    }
> +    memset(aResult->mPLName, 0, kGMPPayloadNameSize);
> +    memcpy(aResult->mPLName, plName.get(), plName.Length());

I would just memset-0 everything uninitialized after copying the string...

And make sure you always null-terminate!

@@ +168,5 @@
> +      return false;
> +    }
> +
> +    if (!ReadParam(aMsg, aIter, &(aResult->mQPMax)) ||
> +        !ReadParam(aMsg, aIter, &(aResult->mNumberOfSimulcastStreams))) {

This had better be checked against kGMPMaxSimulcastStreams otherwise you'll overflow.

@@ +172,5 @@
> +        !ReadParam(aMsg, aIter, &(aResult->mNumberOfSimulcastStreams))) {
> +      return false;
> +    }
> +
> +    for (uint32_t i = 0; i < aResult->mNumberOfSimulcastStreams; i++) {

Do you want to do anything (memset-0?) to the unused array entries?

::: content/media/gmp/GMPParent.cpp
@@ +38,5 @@
> +  nsresult rv = pluginDir->GetLeafName(basename);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  mName = Substring(basename, 4, basename.Length() - 1);

Should probably assert length > 4 first.

@@ +63,5 @@
> +  }
> +
> +  bool opened = Open(mProcess->GetChannel(), mProcess->GetChildProcessHandle());
> +  if (!opened) {
> +    return NS_ERROR_FAILURE;

You need to do the delete thing here too right?

@@ +90,5 @@
> +
> +  mState = GMPStateNotLoaded;
> +
> +  // Invalidate and remove any remaining API objects.
> +  for (int32_t i = mVideoDecoders.Length() - 1; i >= 0; i--) {

These loops will break if there are ever more than INT32_MAX encoders or decoders... I'd just write the loop to handle more, but I guess you could assert instead.

@@ +109,5 @@
> +
> +void
> +GMPParent::VideoDecoderDestroyed(GMPVideoDecoderParent* aDecoder)
> +{
> +  mVideoDecoders.RemoveElement(aDecoder);

I usually try to assert that the element I'm removing was actually in the array. You can do that as follows:

  MOZ_ALWAYS_TRUE(mVideoDecoders.RemoveElement(aDecoder));

@@ +120,5 @@
> +
> +void
> +GMPParent::VideoEncoderDestroyed(GMPVideoEncoderParent* aEncoder)
> +{
> +  mVideoEncoders.RemoveElement(aEncoder);

Ditto

@@ +196,5 @@
> +
> +void
> +GMPParent::ActorDestroy(ActorDestroyReason why)
> +{
> +  UnloadProcess();

I don't think this is going to work as you expect... Here the actor is destroyed, and you'll then call UnloadProcess, which will call EncodingComplete/DecodingComplete on anything in your arrays. But those actors should already be dead (sub-actors are destroyed before manager-actors), so this will probably trigger assertions at least on debug builds. The Encoder/Decoder actors don't override ActorDestroy so there's no code in place to remove them from your array here if the child process crashes (as opposed to the way you handle Recv__delete__ for those actors).

This looks like it will need some testing.

@@ +232,5 @@
> +  return true;
> +}
> +
> +nsresult
> +GMPParent::ParseNextRecord(nsILineInputStream* aLineInputStream,

This could be a static standalone function, right?

@@ +248,5 @@
> +      !Substring(record, 0, aPrefix.Length()).Equals(aPrefix)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  aResult = Substring(record, aPrefix.Length(), record.Length() - aPrefix.Length());

This will copy... I don't think it's going to be hot enough to matter but figured it was worth noting.

@@ +260,5 @@
> +{
> +  MOZ_ASSERT(mDirectory, "Plugin directory cannot be NULL!");
> +  MOZ_ASSERT(!mName.IsEmpty(), "Plugin mName cannot be empty!");
> +
> +  nsString info_leaf = mName + NS_LITERAL_STRING(".info");

nsAutoString, or avoid the temporary altogether and pass |mName + NS_LIT...| to the |AppendRelativePath| function.

@@ +262,5 @@
> +  MOZ_ASSERT(!mName.IsEmpty(), "Plugin mName cannot be empty!");
> +
> +  nsString info_leaf = mName + NS_LITERAL_STRING(".info");
> +  nsCOMPtr<nsIFile> infoFile;
> +  mDirectory->Clone(getter_AddRefs(infoFile));

You either need to check for failure or for null right after

@@ +276,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  nsAutoCString value;

nsCString, this probably just wastes stack space.

@@ +324,5 @@
> +    return rv;
> +  }
> +  nsCCharSeparatedTokenizer apiTokens(value, ',');
> +  while (apiTokens.hasMoreTokens()) {
> +    nsAutoCString api(apiTokens.nextToken());

This copies, just use |const nsDependentCSubstring| here.

@@ +325,5 @@
> +  }
> +  nsCCharSeparatedTokenizer apiTokens(value, ',');
> +  while (apiTokens.hasMoreTokens()) {
> +    nsAutoCString api(apiTokens.nextToken());
> +    api.StripWhitespace();

You sure you want Strip here and not Trim? Don't know how forgiving you want your format to be.

@@ +330,5 @@
> +    if (api.IsEmpty()) {
> +      continue;
> +    }
> +
> +    auto tagsStart = api.FindChar('[');

I generally only think 'auto' is useful when the type is obvious from the statement (e.g. |auto foo = new Foo();|). Otherwise I have to go look up the function in question to figure out the type here.

@@ +346,5 @@
> +      // We know the API name here.
> +      cap->mAPIName.Assign(Substring(api, 0, tagsStart));
> +
> +      auto tagsEnd = api.FindChar(']');
> +      if (tagsEnd != -1 &&

-1 seems like it should return an error, right?

@@ +349,5 @@
> +      auto tagsEnd = api.FindChar(']');
> +      if (tagsEnd != -1 &&
> +          tagsEnd > tagsStart &&
> +          (tagsEnd - tagsStart) > 1) {
> +        nsAutoCString ts(Substring(api, tagsStart + 1, tagsEnd - tagsStart - 1));

This copies, just use |const nsDependentCSubstring| here.

@@ +352,5 @@
> +          (tagsEnd - tagsStart) > 1) {
> +        nsAutoCString ts(Substring(api, tagsStart + 1, tagsEnd - tagsStart - 1));
> +        nsCCharSeparatedTokenizer tagTokens(ts, ':');
> +        while (tagTokens.hasMoreTokens()) {
> +          nsAutoCString tag(tagTokens.nextToken());

And here.

@@ +353,5 @@
> +        nsAutoCString ts(Substring(api, tagsStart + 1, tagsEnd - tagsStart - 1));
> +        nsCCharSeparatedTokenizer tagTokens(ts, ':');
> +        while (tagTokens.hasMoreTokens()) {
> +          nsAutoCString tag(tagTokens.nextToken());
> +          cap->mAPITags.AppendElement(tag);

You didn't do anything with whitespace here, is that right?

@@ +361,5 @@
> +
> +    mCapabilities.AppendElement(cap);
> +  }
> +
> +  if (mCapabilities.Length() < 1) {

Nit: IsEmpty() ?

::: content/media/gmp/GMPParent.h
@@ +28,5 @@
> +  nsCString mAPIName;
> +  nsTArray<nsCString> mAPITags;
> +};
> +
> +class GMPParent : public nsISupports,

MOZ_FINAL, and non-virtual destructor. For all non-inherited classes pretty much.

@@ +36,5 @@
> +  GMPParent();
> +
> +  NS_DECL_ISUPPORTS
> +
> +  enum GMPState {

Could be private?

@@ +41,5 @@
> +    GMPStateNotLoaded,
> +    GMPStateLoaded
> +  };
> +
> +  nsresult Init(nsIFile *pluginDir);

General style nit, please ensure your * and & are on the left everywhere.

::: content/media/gmp/GMPPlatform.cpp
@@ +11,5 @@
> +
> +static MessageLoop* sMainLoop = nullptr;
> +
> +// We just need a refcounted wrapper for GMPTask objects.
> +class Runnable : public RefCounted<Runnable>

RefCounted has lots of problems, please just use the normal NS_INLINE_DECL_REFCOUNTING macro.

@@ +22,5 @@
> +  {
> +    MOZ_ASSERT(mTask);
> +  }
> +
> +  virtual ~Runnable()

This should be private.

@@ +35,5 @@
> +private:
> +  nsAutoPtr<GMPTask> mTask;
> +};
> +
> +class SyncRunnable : public RefCounted<Runnable>

Hrm, this looks like a typo? But RefCounted should go away anyway.

@@ +43,5 @@
> +  : mTask(aTask),
> +    mMessageLoop(aMessageLoop),
> +    mMonitor("GMPSyncRunnable")
> +  {
> +    MOZ_ASSERT(mTask);

Also assert message loop

@@ +46,5 @@
> +  {
> +    MOZ_ASSERT(mTask);
> +  }
> +
> +  virtual ~SyncRunnable()

private

@@ +53,5 @@
> +
> +  void Post()
> +  {
> +    mozilla::MonitorAutoLock lock(mMonitor);
> +    mMessageLoop->PostTask(FROM_HERE, NewRunnableMethod(this, &SyncRunnable::Run));

This could deadlock very easily without any restrictions on the direction of this blocking... Is there any way you can enforce that it always blocks in one direction?

@@ +76,5 @@
> +  if (!aThread) {
> +    return GMPGenericErr;
> +  }
> +
> +  auto thread = new GMPThreadImpl();

This is infallible.

@@ +104,5 @@
> +  return GMPNoErr;
> +}
> +
> +GMPErr
> +SyncRunOnMainThread(GMPTask* aTask)

This needs to ensure that you're not already on the main thread, otherwise you'll deadlock.

@@ +128,5 @@
> +    return GMPGenericErr;
> +  }
> +
> +  auto mutex = new GMPMutexImpl();
> +  if (!mutex) {

Infallible

@@ +153,5 @@
> +{
> +  // We'll assume that the first time someone constructs a thread object
> +  // they're doing it from the main thread. I like to live dangerously.
> +  if (!sMainLoop) {
> +    sMainLoop = MessageLoop::current();

I don't think it's a good idea to leave this up to chance... I'd set sMainLoop in InitPlatformAPI.

@@ +165,5 @@
> +void
> +GMPThreadImpl::Post(GMPTask* aTask)
> +{
> +  if (!mThread.IsRunning()) {
> +    bool started = mThread.Start();

This is racy, there's no protection against having the first Post called simultaneously on two different threads.

::: content/media/gmp/GMPPlatform.h
@@ +14,5 @@
> +namespace gmp {
> +
> +static void InitPlatformAPI(GMPPlatformAPI& aPlatformAPI);
> +
> +class GMPThreadImpl : public GMPThread

I would keep all this in the cpp file, not expose it at all. Everythig should use the API you have in gmp-platform.h right?

::: content/media/gmp/GMPProcessChild.cpp
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "GMPProcessChild.h"
> +
> +#include "prlink.h"

Needed?

::: content/media/gmp/GMPProcessChild.h
@@ +23,5 @@
> +  virtual bool Init() MOZ_OVERRIDE;
> +  virtual void CleanUp() MOZ_OVERRIDE;
> +
> +protected:
> +  static GMPProcessChild* current() {

I don't see any callers of this.

::: content/media/gmp/GMPProcessParent.cpp
@@ +49,5 @@
> +{
> +  MessageLoop* currentLoop = MessageLoop::current();
> +  MessageLoop* ioLoop = XRE_GetIOMessageLoop();
> +
> +  if (currentLoop == ioLoop) {

This doesn't look possible, right?

::: content/media/gmp/GMPService.cpp
@@ +35,5 @@
> +{
> +  if (!strcmp(NS_XPCOM_SHUTDOWN_OBSERVER_ID, aTopic)) {
> +    if (mGMPThread) {
> +      mGMPThread->Dispatch(NS_NewRunnableMethod(this, &GeckoMediaPluginService::UnloadPlugins),
> +                           NS_DISPATCH_SYNC);

If you don't have a thread maybe assert that mPlugins is empty.

::: content/media/gmp/GMPVideoDecoderChild.cpp
@@ +13,5 @@
> +namespace gmp {
> +
> +GMPVideoDecoderChild::GMPVideoDecoderChild()
> +: mVideoDecoder(nullptr),
> +  mVideoHost(this)

needs MOZ_THIS_IN_INITIALIZER_LIST

@@ +22,5 @@
> +{
> +}
> +
> +bool
> +GMPVideoDecoderChild::Init(GMPVideoDecoder* aDecoder)

Doesn't look like this can fail, make it void instead?

::: content/media/gmp/GMPVideoDecoderParent.cpp
@@ +14,5 @@
> +GMPVideoDecoderParent::GMPVideoDecoderParent(GMPParent *aPlugin)
> +: mCanSendMessages(true),
> +  mPlugin(aPlugin),
> +  mObserver(nullptr),
> +  mVideoHost(this)

MOZ_THIS_IN_INITIALIZER_LIST

@@ +56,5 @@
> +
> +  if (!aCallback) {
> +    return GMPVideoGenericErr;
> +  }
> +  mObserver = aCallback;

Should probably assert that this is currently null before assigning.

@@ +62,5 @@
> +  if (!SendInitDecode(aCodecSettings, aCoreCount)) {
> +    return GMPVideoGenericErr;
> +  }
> +
> +  // Async IPC, we don't have access to a return value.

In general it looks like there is currently no mechanism for the child to communicate errors to the parent. Is that desirable?

@@ +89,5 @@
> +                  aRenderTimeMs)) {
> +    return GMPVideoGenericErr;
> +  }
> +
> +  aInputFrame->Destroy();

This is a little tricky... It means that callers must sometimes destroy the input frame themselves if there is an error? Sounds like a recipe for leaks :) I think it's much easier to just make the API contract that Destroy will always be called, regardless of error.

@@ +96,5 @@
> +  return GMPVideoNoErr;
> +}
> +
> +GMPVideoErr
> +GMPVideoDecoderParent::Reset()

So what happens to any frames that the child has already decoded that are waiting to be processed in the event queue? There's no mechanism to ignore/discard them?

@@ +157,5 @@
> +    return false;
> +  }
> +
> +  // Ignore any return code. It is OK for this to fail without killing the process.
> +  mObserver->Decoded(f);

I guess the callback is supposed to delete |f|?

::: content/media/gmp/GMPVideoDecoderParent.h
@@ +17,5 @@
> +namespace gmp {
> +
> +class GMPParent;
> +
> +class GMPVideoDecoderParent : public RefCounted<GMPVideoDecoderParent>,

Please remove RefCounted, use macros.

@@ +26,5 @@
> +public:
> +  MOZ_DECLARE_REFCOUNTED_TYPENAME(GMPVideoDecoderParent)
> +
> +  GMPVideoDecoderParent(GMPParent *aPlugin);
> +  virtual ~GMPVideoDecoderParent();

This needs to be private.

@@ +58,5 @@
> +  virtual bool Recv__delete__() MOZ_OVERRIDE;
> +
> +  bool mCanSendMessages;
> +  GMPParent* mPlugin;
> +  GMPDecoderCallback* mObserver;

Nit: "observer" is confusing... Why not just call it |mCallback|?

::: content/media/gmp/GMPVideoEncodedFrameImpl.cpp
@@ +19,5 @@
> +  mHost(aHost)
> +{
> +}
> +
> +GMPVideoEncodedFrameImpl::GMPVideoEncodedFrameImpl(GMPVideoEncodedFrameData aFrameData,

This could be much simpler if you make this class hold a GMPVideoEncodedFrameData member.

@@ +59,5 @@
> +  mHost = nullptr;
> +}
> +
> +bool
> +GMPVideoEncodedFrameImpl::InitFrameData(GMPVideoEncodedFrameData& aFrameData)

This too.

@@ +73,5 @@
> +
> +  // This method is called right before Shmem is sent to another process.
> +  // We need to effectively zero out our member copy so that we don't
> +  // try to delete Shmem we don't own later.
> +  mBuffer = ipc::Shmem();

Hm, so the name of the method is misleading. You're actually transferring ownership here.

@@ +91,5 @@
> +GMPVideoEncodedFrameImpl::CreateEmptyFrame(uint32_t aSize)
> +{
> +  DestroyBuffer();
> +
> +  if (aSize > 0) {

Nit: != 0

@@ +108,5 @@
> +GMPVideoEncodedFrameImpl::CopyFrame(const GMPVideoEncodedFrame& aFrame)
> +{
> +  auto& f = static_cast<const GMPVideoEncodedFrameImpl&>(aFrame);
> +
> +  if (f.mSize > 0) {

!=

::: content/media/gmp/GMPVideoEncodedFrameImpl.h
@@ +43,5 @@
> +{
> +  friend struct IPC::ParamTraits<mozilla::gmp::GMPVideoEncodedFrameImpl>;
> +public:
> +  GMPVideoEncodedFrameImpl(GMPVideoHostImpl* aHost);
> +  GMPVideoEncodedFrameImpl(GMPVideoEncodedFrameData aFrameData, GMPVideoHostImpl* aHost);

Shouldn't this be a |GMPVideoEncodedFrameData&|? Otherwise you're making a temporary

@@ +87,5 @@
> +  uint32_t mTimeStamp;
> +  int64_t  mCaptureTime_ms;
> +  GMPVideoFrameType mFrameType;
> +  uint32_t mSize;
> +  bool     mCompleteFrame;

Hm, rather than duplicating all the members of GMPVideoEncodedFrameData, why not just have a GMPVideoEncodedFrameData member?

::: content/media/gmp/GMPVideoEncoderChild.cpp
@@ +11,5 @@
> +namespace gmp {
> +
> +GMPVideoEncoderChild::GMPVideoEncoderChild()
> +: mVideoEncoder(nullptr),
> +  mVideoHost(this)

MOZ_THIS_IN_INITIALIZER_LIST

@@ +20,5 @@
> +{
> +}
> +
> +bool
> +GMPVideoEncoderChild::Init(GMPVideoEncoder* aEncoder)

return void?

@@ +44,5 @@
> +  ef->InitFrameData(frameData);
> +
> +  SendEncoded(frameData, aCodecSpecificInfo);
> +
> +  aEncodedFrame->Destroy();

I think it would make more sense to make this method take a |const GMPVideoEncodedFrame&|, and then you could call InitFrameData() on it to get your data. Then the caller would auto-delete it so you wouldn't have to remember to do it manually.

@@ +86,5 @@
> +    return false;
> +  }
> +
> +  auto f = new GMPVideoi420FrameImpl(aInputFrame, &mVideoHost);
> +  if (!f) {

infallible

::: content/media/gmp/GMPVideoEncoderParent.cpp
@@ +14,5 @@
> +GMPVideoEncoderParent::GMPVideoEncoderParent(GMPParent *aPlugin)
> +: mCanSendMessages(true),
> +  mPlugin(aPlugin),
> +  mObserver(nullptr),
> +  mVideoHost(this)

MOZ_THIS_IN_INITIALIZER_LIST

@@ +82,5 @@
> +
> +  GMPVideoi420FrameData frameData;
> +  inputFrameImpl->InitFrameData(frameData);
> +
> +  InfallibleTArray<int> frameTypes;

Might as well call SetCapacity to avoid multiple mallocs.

::: content/media/gmp/GMPVideoEncoderParent.h
@@ +16,5 @@
> +namespace gmp {
> +
> +class GMPParent;
> +
> +class GMPVideoEncoderParent : public RefCounted<GMPVideoEncoderParent>,

RefCounted...

@@ +25,5 @@
> +public:
> +  MOZ_DECLARE_REFCOUNTED_TYPENAME(GMPVideoEncoderParent)
> +
> +  GMPVideoEncoderParent(GMPParent *aPlugin);
> +  virtual ~GMPVideoEncoderParent();

private

::: content/media/gmp/GMPVideoHost.cpp
@@ +31,5 @@
> +  *aFrame = nullptr;
> +
> +  if (aFormat == kGMPI420VideoFrame) {
> +    auto f = new GMPVideoi420FrameImpl(this);
> +    if (f) {

Infallible new, below too.

@@ +35,5 @@
> +    if (f) {
> +      *aFrame = f;
> +      return GMPVideoNoErr;
> +    }
> +  } else if (aFormat == kGMPEncodedVideoFrame) {

For enums I generally prefer switch statements.

@@ +60,5 @@
> +  }
> +  *aPlane = nullptr;
> +
> +  auto p = new GMPPlaneImpl(this);
> +  if (!p) {

infallible

@@ +81,5 @@
> +GMPVideoHostImpl::InvalidateShmem()
> +{
> +  for (uint32_t i = 0; i < mPlanes.Length(); i++) {
> +    mPlanes[i]->InvalidateShmem();
> +    mPlanes.RemoveElementAt(i);

These loops don't work, you're always incrementing |i| so you'll skip elements.

@@ +93,5 @@
> +
> +void
> +GMPVideoHostImpl::PlaneDestroyed(GMPPlaneImpl* aPlane)
> +{
> +  mPlanes.RemoveElement(aPlane);

MOZ_ALWAYS_TRUE?

@@ +99,5 @@
> +
> +void
> +GMPVideoHostImpl::EncodedFrameDestroyed(GMPVideoEncodedFrameImpl* aFrame)
> +{
> +  mEncodedFrames.RemoveElement(aFrame);

Here too.

::: content/media/gmp/GMPVideoPlaneImpl.cpp
@@ +7,5 @@
> +
> +namespace mozilla {
> +namespace gmp {
> +
> +GMPPlaneImpl::GMPPlaneImpl(GMPVideoHostImpl* aHost)

Assert non-null?

@@ +14,5 @@
> +  mHost(aHost)
> +{
> +}
> +
> +GMPPlaneImpl::GMPPlaneImpl(GMPPlaneData aPlaneData, GMPVideoHostImpl* aHost)

Here too

::: content/media/gmp/GMPVideoPlaneImpl.h
@@ +13,5 @@
> +namespace gmp {
> +
> +class GMPVideoHostImpl;
> +
> +class GMPPlaneImpl : public GMPPlane

Hm, why does all of this code use a signed int for size? The frame code uses unsigned everywhere (like I think it should...)

::: content/media/gmp/GMPVideoi420FrameImpl.cpp
@@ +35,5 @@
> +{
> +}
> +
> +bool
> +GMPVideoi420FrameImpl::InitFrameData(GMPVideoi420FrameData& aFrameData)

Same comments here about having individual members vs. a single GMPVideoi420FrameData member

@@ +82,5 @@
> +      return &mUPlane;
> +    case kGMPVPlane:
> +      return &mVPlane;
> +    default:
> +      assert(false);

MOZ_CRASH, here and below.

@@ +139,5 @@
> +
> +GMPVideoErr
> +GMPVideoi420FrameImpl::CreateFrame(int32_t aSize_y, const uint8_t* aBuffer_y,
> +                                   int32_t aSize_u, const uint8_t* aBuffer_u,
> +                                   int32_t aSize_v, const uint8_t* aBuffer_v,

Seems like these pointers should be asserted at least

::: content/media/gmp/gmp-api/gmp-platform.h
@@ +44,5 @@
> +};
> +
> +class GMPThread {
> +public:
> +  virtual ~GMPThread() {}

You expect consumers to just call delete manually? What happens if they didn't join yet?

@@ +45,5 @@
> +
> +class GMPThread {
> +public:
> +  virtual ~GMPThread() {}
> +  virtual void Post(GMPTask* task) = 0;

It seems weird that there's no way to know if this succeeds or not. What if the thread has already been joined? Or if the thread can't be started?

@@ +49,5 @@
> +  virtual void Post(GMPTask* task) = 0;
> +  virtual void Join() = 0;
> +};
> +
> +class GMPMutex {

There's no wait/notify here so users won't be able to do anything but busy-wait...

@@ +51,5 @@
> +};
> +
> +class GMPMutex {
> +public:
> +  virtual ~GMPMutex() {}

Similar concerns about deleting here.

::: content/media/gmp/gmp-api/gmp-video-codec.h
@@ +56,5 @@
> +                    // within a frame.
> +};
> +
> +// VP8 specific
> +struct GMPVideoCodecVP8

This needs to have some indication that it's manually serialized so that member additions are handled in the serialization helpers. All other manually-serialized structs like this too.

::: content/media/gmp/gmp-api/gmp-video-decode.h
@@ +38,5 @@
> +#include "gmp-video-frame-i420.h"
> +#include "gmp-video-frame-encoded.h"
> +#include "gmp-video-codec.h"
> +
> +// ALL METHODS MUST BE CALLED ON THE MAIN THREAD

Are these comments still accurate?

@@ +85,5 @@
> +                             bool aMissingFrames,
> +                             const GMPCodecSpecificInfo& aCodecSpecificInfo,
> +                             int64_t aRenderTimeMs = -1) = 0;
> +
> +  // Reset decoder state and prepare for a new call to Decode(...). Flushes the decoder pipeline.

Nit: What should subclasses do about the previous callback?

::: content/media/gmp/mozIGeckoMediaPluginService.idl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsISupports.idl"
> +#include "nsIThread.idl"

This can just be forward-declared.

@@ +17,5 @@
> +[ptr] native GMPVideoHost(GMPVideoHost);
> +[ptr] native MessageLoop(MessageLoop);
> +
> +[uuid(BF5A9086-70F5-4D38-832D-1609BBF963CD)]
> +interface mozIGeckoMediaPluginService : nsISupports

Need to document which thread (hopefully just main) this stuff can be called from.

Comment 13

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ok, all done!

Comment 14

[Josh Aas]

Attachment: impl v1.4

This updated patch should contain fixes for the issues bent pointed out, except for those that relate to the GMP API itself. Fixing that up should be done by media folks who now exactly how it should behave.

Thanks for the thorough review Ben, really helpful!

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8413015 [details] [diff] [review]
impl v1.4

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

Ok, this looks pretty good. A few comments from last round weren't addressed as far as I can tell (noted below) and the one I'm most concerned about now is the cleanup when a child process crashes. There are also a few style things noted below that need to be applied throughout.

::: content/media/gmp/GMPChild.cpp
@@ +34,5 @@
> +               MessageLoop* aIOLoop,
> +               IPC::Channel* aChannel)
> +{
> +  return LoadPluginLibrary(aPluginPath) &&
> +         Open(aChannel, aParentProcessHandle, aIOLoop);

This hasn't been addressed, what happens if Open fails? You need to undo things that LoadPluginLibrary did, right?

@@ +78,5 @@
> +    return false;
> +  }
> +
> +  auto platformAPI = new GMPPlatformAPI();
> +  if (!platformAPI) {

new is infallible.

::: content/media/gmp/GMPChild.h
@@ +5,5 @@
> +
> +#ifndef GMPChild_h_
> +#define GMPChild_h_
> +
> +#include "nsString.h"

Unused, looks like?

::: content/media/gmp/GMPMessageUtils.h
@@ +10,5 @@
> +
> +namespace IPC {
> +
> +template <>
> +struct ParamTraits<GMPVideoCodecComplexity>:

Nit: : is supposed to be on the following line for all these.

::: content/media/gmp/GMPParent.cpp
@@ +18,5 @@
> +namespace gmp {
> +
> +GMPParent::GMPParent()
> +: mState(GMPStateNotLoaded),
> +  mProcess(nullptr)

Nit: Constructor style is

  Foo::Foo()
    : mBar()
    , mBaz()

@@ +55,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsAutoCString path;
> +  mDirectory->GetNativePath(path);

MOZ_ALWAYS_TRUE(NS_SUCCEEDED()) would be nicer

@@ +88,5 @@
> +void
> +GMPParent::UnloadProcess()
> +{
> +  if (mState == GMPStateNotLoaded) {
> +    return;

Might be wise to assert |mVideoDecoders.IsEmpty() && mVideoEncoders.IsEmpty()| here.

@@ +133,5 @@
> +  NS_DispatchToCurrentThread(event);
> +}
> +
> +GMPState
> +GMPParent::State()

Nit: You could inline this in the header, and make it 'const'

@@ +172,5 @@
> +  return false;
> +}
> +
> +bool
> +GMPParent::EnsureProcessLoaded()

I'd like this to eventually return false after we've started shutdown, but I guess that could be done in a followup.

@@ +222,5 @@
> +
> +void
> +GMPParent::ActorDestroy(ActorDestroyReason why)
> +{
> +  UnloadProcess();

It doesn't look like my previous comment on this code was addressed. I think this will assert if the child dies while you've got active Encoder/Decoder objects alive.

@@ +281,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +GMPParent::ReadGMPMetaData()

We should get a followup filed to have some fuzzing of this file format.

::: content/media/gmp/GMPParent.h
@@ +9,5 @@
> +#include "nscore.h"
> +#include "nsISupports.h"
> +#include "nsString.h"
> +#include "nsTArray.h"
> +#include "nsIFile.h"

Forward declare.

@@ +14,5 @@
> +#include "GMPProcessParent.h"
> +#include "mozilla/gmp/PGMPParent.h"
> +#include "nsCOMPtr.h"
> +#include "GMPVideoDecoderParent.h"
> +#include "GMPVideoEncoderParent.h"

Nit: Alphabetizing these includes (here and everywhere, actually) would be nice.

@@ +45,5 @@
> +  nsresult Init(nsIFile* pluginDir);
> +  nsresult LoadProcess();
> +  void MaybeUnloadProcess();
> +  void UnloadProcess();
> +  bool SupportsAPI(const nsCString &aAPI, const nsCString &aTag);

Nit: & on the left, here and everywhere.

@@ +46,5 @@
> +  nsresult LoadProcess();
> +  void MaybeUnloadProcess();
> +  void UnloadProcess();
> +  bool SupportsAPI(const nsCString &aAPI, const nsCString &aTag);
> +  nsresult GetGMPVideoDecoder(GMPVideoDecoderParent** gmpVD);

Nit: Params should be named 'aFoo' (like on the previous line) here and everywhere.

@@ +69,5 @@
> +  nsString            mName; // base name of plugin on disk, UTF-16 because used for paths
> +  nsCString           mDisplayName; // name of plugin displayed to users
> +  nsCString           mDescription; // description of plugin for display to users
> +  nsCString           mVersion;
> +  nsTArray<nsAutoPtr<GMPCapability>> mCapabilities;

Nit: I don't see the point in trying to align member names, especially since it won't work with longer types.

::: content/media/gmp/GMPPlatform.cpp
@@ +11,5 @@
> +
> +static MessageLoop* sMainLoop = nullptr;
> +
> +// We just need a refcounted wrapper for GMPTask objects.
> +class Runnable

MOZ_FINAL, no need for virtual destructor.

@@ +24,5 @@
> +  }
> +
> +  void Run()
> +  {
> +    mTask->Run();

You should explicitly null out mTask here, otherwise the thread on which the task is destroyed will be arbitrary.

@@ +35,5 @@
> +
> +  nsAutoPtr<GMPTask> mTask;
> +};
> +
> +class SyncRunnable

Ditto.

@@ +54,5 @@
> +  {
> +    // We assert here for two reasons.
> +    // 1) Nobody should be blocking the main thread.
> +    // 2) This prevents deadlocks when doing sync calls to main which if the
> +    //    main thread tries to do a sycn call back to the calling thread.

Nit: typo "sycn"

@@ +57,5 @@
> +    // 2) This prevents deadlocks when doing sync calls to main which if the
> +    //    main thread tries to do a sycn call back to the calling thread.
> +    MOZ_ASSERT(MessageLoop::current() != sMainLoop);
> +
> +    mozilla::MonitorAutoLock lock(mMonitor);

Nit: You're already inside the mozilla namespace so all of these 'mozilla::' should be unnecessary.

@@ +59,5 @@
> +    MOZ_ASSERT(MessageLoop::current() != sMainLoop);
> +
> +    mozilla::MonitorAutoLock lock(mMonitor);
> +    mMessageLoop->PostTask(FROM_HERE, NewRunnableMethod(this, &SyncRunnable::Run));
> +    lock.Wait();

This code is technically correct so I won't insist that you change it, but I recommend that you change it anyway! You're holding a lock while calling out into another module and that's always risky.

The safe way is to do this:

  class SyncRunnable
  {
    // ...
    bool mDone;
    // ...
    SyncRunnable(...)
      : // ...
      , mDone(true)
    {
      // ...
    }
    // ...
    void Post()
    {
      // ...
      mMessageLoop->PostTask(...);
      MonitorAutoLock lock(mMonitor);
      while (!mDone) {
        lock.Wait();
      }
    }
    void Run()
    {
      mTask->Run();
      mTask = nullptr;
      MonitorAutoLock lock(mMonitor);
      mDone = true;
      lock.Notify();
    }
  // ...
  };

@@ +64,5 @@
> +  }
> +
> +  void Run()
> +  {
> +    mTask->Run();

Null out mTask here too.

@@ +87,5 @@
> +  }
> +
> +  auto thread = new GMPThreadImpl();
> +
> +  *aThread = thread;

Nit: Nix the temporary

@@ +100,5 @@
> +    return GMPGenericErr;
> +  }
> +
> +  nsRefPtr<Runnable> r = new Runnable(aTask);
> +  if (!r) {

Nit: new is infallible.

@@ +117,5 @@
> +    return GMPGenericErr;
> +  }
> +
> +  nsRefPtr<SyncRunnable> r = new SyncRunnable(aTask, sMainLoop);
> +  if (!r) {

Ditto.

@@ +135,5 @@
> +  }
> +
> +  auto mutex = new GMPMutexImpl();
> +
> +  *aMutex = mutex;

Nit: Nix the temporary

@@ +178,5 @@
> +    }
> +  }
> +
> +  nsRefPtr<Runnable> r = new Runnable(aTask);
> +  if (!r) {

Nit: new is infallible

@@ +189,5 @@
> +
> +void
> +GMPThreadImpl::Join()
> +{
> +  if (mThread.IsRunning()) {

I think you need to hold mMutex while calling this.

::: content/media/gmp/GMPProcessChild.h
@@ +11,5 @@
> +
> +namespace mozilla {
> +namespace gmp {
> +
> +class GMPProcessChild : public mozilla::ipc::ProcessChild {

MOZ_FINAL, no need for virtual dtor.

::: content/media/gmp/GMPProcessParent.h
@@ +18,5 @@
> +
> +namespace mozilla {
> +namespace gmp {
> +
> +class GMPProcessParent : public mozilla::ipc::GeckoChildProcessHost

MOZ_FINAL, no need for virtual dtor.

::: content/media/gmp/GMPService.cpp
@@ +28,5 @@
> +
> +StaticRefPtr<GeckoMediaPluginService> GeckoMediaPluginService::sService;
> +
> +already_AddRefed<GeckoMediaPluginService>
> +GeckoMediaPluginService::GetGeckoMediaPluginService()

This needs a lot of work to be made threadsafe in the way you're envisioning. I sent a private email with an example of the kind of approach I think you'll need. Basically block and call to the main thread before running anything here.

@@ +46,5 @@
> +  return result.forget();
> +}
> +
> +nsresult
> +GeckoMediaPluginService::Init()

Seems like this should be a void method since you're only ever returning success.

@@ +49,5 @@
> +nsresult
> +GeckoMediaPluginService::Init()
> +{
> +  nsCOMPtr<nsIObserverService> obsService = mozilla::services::GetObserverService();
> +  if (obsService) {

I would assert this rather than release-check.

@@ +59,5 @@
> +
> +NS_IMETHODIMP
> +GeckoMediaPluginService::Observe(nsISupports *aSubject,
> +                                 const char *aTopic,
> +                                 const char16_t *someData)

Nit: * on the left

@@ +62,5 @@
> +                                 const char *aTopic,
> +                                 const char16_t *someData)
> +{
> +  if (!strcmp(NS_XPCOM_SHUTDOWN_OBSERVER_ID, aTopic)) {
> +    mShuttingDown = true;

You need to hold the mutex before touching mGMPThread or mShuttingDown. Otherwise another thread could be creating it at the same time.

Also, let's make sure we have a followup on file to set a flag on all existing GMPParent instances to keep them from launching a new process after this point.

@@ +131,5 @@
> +GeckoMediaPluginService::GetGMPVideoEncoderVP8(GMPVideoHost** outVideoHost, GMPVideoEncoder** gmpVE)
> +{
> +  MOZ_ASSERT(NS_GetCurrentThread() == mGMPThread);
> +
> +  if (mShuttingDown) {

You need to find all your mShuttingDown checks and make sure to synchronize with the mutex.

@@ +187,5 @@
> +{
> +  MOZ_ASSERT(NS_GetCurrentThread() == mGMPThread);
> +
> +  for (uint32_t i = 0; i < mPlugins.Length(); i++) {
> +    GMPParent *gmp = mPlugins[i];

Nit: * on the left

@@ +243,5 @@
> +    return rv;
> +  }
> +
> +  uint32_t childCount = 0;
> +  regKey->GetChildCount(&childCount);

MOZ_ALWAYS_TRUE(NS_SUCCEEDED())

::: content/media/gmp/GMPService.h
@@ +6,5 @@
> +#ifndef GMPService_h_
> +#define GMPService_h_
> +
> +#include "mozIGeckoMediaPluginService.h"
> +#include "GMPParent.h"

Forward declare.

@@ +9,5 @@
> +#include "mozIGeckoMediaPluginService.h"
> +#include "GMPParent.h"
> +#include "nsIObserver.h"
> +#include "nsTArray.h"
> +#include "nsIFile.h"

Forward declare.

@@ +11,5 @@
> +#include "nsIObserver.h"
> +#include "nsTArray.h"
> +#include "nsIFile.h"
> +#include "mozilla/Mutex.h"
> +#include "nsIThread.h"

Forward declare.

@@ +12,5 @@
> +#include "nsTArray.h"
> +#include "nsIFile.h"
> +#include "mozilla/Mutex.h"
> +#include "nsIThread.h"
> +#include "mozilla/ClearOnShutdown.h"

Not needed here, move to cpp.

@@ +18,5 @@
> +namespace mozilla {
> +namespace gmp {
> +
> +class GeckoMediaPluginService MOZ_FINAL : public mozIGeckoMediaPluginService,
> +                                          public nsIObserver

Our style guide wants this nowadays:

  class Foo
    : public Base1
    , public Base2

@@ +21,5 @@
> +class GeckoMediaPluginService MOZ_FINAL : public mozIGeckoMediaPluginService,
> +                                          public nsIObserver
> +{
> +public:
> +  GeckoMediaPluginService();

This should be private.

@@ +24,5 @@
> +public:
> +  GeckoMediaPluginService();
> +
> +  static already_AddRefed<GeckoMediaPluginService> GetGeckoMediaPluginService();
> +  static mozilla::StaticRefPtr<GeckoMediaPluginService> sService;

This shouldn't be public... In fact, I think you should just make it a global in the cpp rather than a static member.

@@ +26,5 @@
> +
> +  static already_AddRefed<GeckoMediaPluginService> GetGeckoMediaPluginService();
> +  static mozilla::StaticRefPtr<GeckoMediaPluginService> sService;
> +
> +  nsresult Init();

This should be private.

@@ +43,5 @@
> +  void RefreshPluginList();
> +  void ProcessPossiblePlugin(nsIFile* aDir);
> +  nsresult SearchDirectory(nsIFile* aSearchDir);
> +  nsresult GetPossiblePlugins(nsTArray<nsCOMPtr<nsIFile>> &aDirs);
> +  nsresult GetDirectoriesToSearch(nsTArray<nsCOMPtr<nsIFile>> &aDirs);

Nit: & on the left for these two funcs.

::: content/media/gmp/GMPVideoDecoderChild.cpp
@@ +37,5 @@
> +  return mVideoHost;
> +}
> +
> +void
> +GMPVideoDecoderChild::Decoded(GMPVideoi420Frame* decodedFrame)

There's no protection against null being passed here, is that ok? I think I would force-crash maybe.

@@ +117,5 @@
> +    return false;
> +  }
> +
> +  auto f = new GMPVideoEncodedFrameImpl(inputFrame, &mVideoHost);
> +  if (!f) {

new is infallible

::: content/media/gmp/GMPVideoDecoderParent.cpp
@@ +82,5 @@
> +  MOZ_ASSERT(mPlugin->GMPThread() == NS_GetCurrentThread());
> +
> +  if (!mCanSendMessages) {
> +    NS_WARNING("Trying to use an invalid GMP video decoder!");
> +    aInputFrame->Destroy();

I think you should use nsAutoRef for this (you've got three Destroy()'s here already). All you have to do is specialize nsAutoRefTraits.

::: content/media/gmp/GMPVideoDecoderParent.h
@@ +17,5 @@
> +namespace gmp {
> +
> +class GMPParent;
> +
> +class GMPVideoDecoderParent : public GMPVideoDecoder,

ActorDestroy is now required ;)

Also, MOZ_FINAL, no need for virtual dtor.

::: content/media/gmp/GMPVideoEncodedFrameImpl.h
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* Copyright (c) 2014, Mozilla Corporation

Er, wait, what's this license? Shouldn't everything be MPL?

::: content/media/gmp/gmp-api/gmp-entrypoints.h
@@ +55,5 @@
> +//   plugin is valid. It is owned by the host, plugin should not attempt to delete.
> +//   May be null.
> +// - 'aPluginAPI' is for returning the requested API. Destruction of the requsted
> +//   API object is defined by the API.
> +typedef GMPErr (*GMPGetAPIFunc)(const char* aAPIName, void* aHostAPI, void** aPluginAPI);

This needs to say something about the type of pointer that is being returned...

::: content/media/gmp/mozIGeckoMediaPluginService.idl
@@ +14,5 @@
> +
> +[ptr] native GMPVideoDecoder(GMPVideoDecoder);
> +[ptr] native GMPVideoEncoder(GMPVideoEncoder);
> +[ptr] native GMPVideoHost(GMPVideoHost);
> +[ptr] native MessageLoop(MessageLoop);

This shouldn't be needed now.

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +274,3 @@
>    // PerformAysncLaunchInternal/GetPathToBinary may be called on the IO thread,
>    // and they want to use the directory service, which needs to happen on the
>    // main thread (in the event that its implemented in JS). So we grab

Can you fix the assertions and comments (maybe just remove them) here to make sure we always assert that we're called on the main thread now?

::: toolkit/xre/nsEmbedFunctions.cpp
@@ +604,4 @@
>        NS_ENSURE_SUCCESS(rv, rv);
>      }
>  
> +    GeckoChildProcessHost::CacheGreDir();

I can't remember if this is supposed to be in a different bug or not, but this looks good.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8413015 [details] [diff] [review]
impl v1.4

Oh, and the threadsafety around GMPService creation has to be fixed up.

Comment 17

[Josh Aas]

Attachment: impl v1.5

Comment 18

[Josh Aas]

Attachment: impl v1.6

Comment 19

[Josh Aas]

Attachment: fix v1.7

Better solution to the CacheGREDir issue.

Comment 20

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8420171 [details] [diff] [review]
fix v1.7

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

All of the comments I made for GMPVideoDecoderParent seem to apply to GMPVideoEncoderParent too!

::: content/media/gmp/GMPChild.cpp
@@ +19,5 @@
> +
> +GMPChild::GMPChild()
> +: mLib(nullptr),
> +  mGetAPIFunc(nullptr),
> +  mGMPMessageLoop(MessageLoop::current())

Nit: constructor style is off

::: content/media/gmp/GMPService.cpp
@@ +18,5 @@
> +namespace gmp {
> +
> +NS_IMPL_ISUPPORTS(GeckoMediaPluginService, mozIGeckoMediaPluginService, nsIObserver)
> +
> +GeckoMediaPluginService::GeckoMediaPluginService()

You can assert that you're on the main thread here now.

@@ +19,5 @@
> +
> +NS_IMPL_ISUPPORTS(GeckoMediaPluginService, mozIGeckoMediaPluginService, nsIObserver)
> +
> +GeckoMediaPluginService::GeckoMediaPluginService()
> +  : mMutex("gmp-thread")

Nit: Might as well call this "GeckoMediaPluginService::mMutex" so it's more understandable

@@ +24,5 @@
> +  , mShuttingDown(false)
> +{
> +}
> +
> +GeckoMediaPluginService::~GeckoMediaPluginService()

You should be able to assert here that mPlugins is empty.

@@ +30,5 @@
> +}
> +
> +static StaticRefPtr<GeckoMediaPluginService> sSingletonService;
> +
> +class GMPServiceCreateHelper MOZ_FINAL : public nsRunnable

Nit: Generally globals and helper classes go at the top of the file after includes.

@@ +116,5 @@
> +  }
> +
> +  nsCOMPtr<nsIObserverService> obsService = mozilla::services::GetObserverService();
> +  MOZ_ASSERT(obsService);
> +  obsService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);

You also need to register for NS_XPCOM_SHUTDOWN_THREADS_OBSERVER_ID so that you can clean up your thread. You'll need to call Shutdown() on it and then null it out. And with careful locking since nsThread::Shutdown() spins the event loop, see below.

@@ +129,5 @@
> +    MutexAutoLock lock(mMutex);
> +    mShuttingDown = true;
> +    if (mGMPThread) {
> +      mGMPThread->Dispatch(NS_NewRunnableMethod(this, &GeckoMediaPluginService::UnloadPlugins),
> +                           NS_DISPATCH_SYNC);

You shouldn't hold your mutex while spinning the event loop (NS_DISPATCH_SYNC == spinning). That's a recipe for deadlock.

Instead do:

  nsCOMPtr<nsIThread> gmpThread;
  {
    MutexAutoLock lock(mMutex);
    mShuttingDown = true;
    gmpThread = mGMPThread;
  }
  if (gmpThread) {
    gmpThread->Dispatch(...);
  } else {
    ...
  }

@@ +170,5 @@
> +  MOZ_ASSERT(NS_GetCurrentThread() == mGMPThread);
> +
> +  {
> +    MutexAutoLock lock(mMutex);
> +    if (mShuttingDown) {

So, this is completely safe, but still racy... What if the main thread is just about to set this flag?

I kinda think you shouldn't check here (or in the other Get below) but you'll need to protect against repopulating your plugin array somehow. You probably want a 'mShuttingDownOnGMPThread' flag or something that gets set in UnloadPlugins. Then you can replace this check for |mShuttingDown| with that flag (no locking needed) and refuse to re-populate |mPlugins| if it is set. You'd also want to assert that the new flag is not set in all those methods relating to re-populating the array.

@@ +400,5 @@
> +{
> +  MOZ_ASSERT(NS_GetCurrentThread() == mGMPThread);
> +
> +  if (!aDir) {
> +    return;

It seems like you should be able to assert that this is non-null.

::: content/media/gmp/GMPService.h
@@ +24,5 @@
> +public:
> +  static already_AddRefed<GeckoMediaPluginService> GetGeckoMediaPluginService();
> +
> +  GeckoMediaPluginService();
> +  void Init();

If you make GMPServiceCreateHelper a nested class then you won't have to expose the ctor/Init as public.

::: content/media/gmp/GMPVideoDecoderParent.cpp
@@ +18,5 @@
> +
> +namespace mozilla {
> +namespace gmp {
> +
> +GMPVideoDecoderParent::GMPVideoDecoderParent(GMPParent *aPlugin)

Nit: * on the left and initializer list style is off

@@ +86,5 @@
> +                              bool aMissingFrames,
> +                              const GMPCodecSpecificInfo& aCodecSpecificInfo,
> +                              int64_t aRenderTimeMs)
> +{
> +  nsAutoRef<GMPVideoEncodedFrame> frameRef(aInputFrame);

Nit: I'd call the variable 'autoDestroy' to make it clear what it does

@@ +167,5 @@
> +  unused << SendDecodingComplete();
> +}
> +
> +void
> +GMPVideoDecoderParent::ActorDestroy(ActorDestroyReason aWhy)

It's a little unfortunate that this code path looks so different from DecodingComplete... It means that people who modify this code will need to be aware of both and will have to understand the differences... Maybe add a big comment at the top of each saying that the methods need to stay synced up?

@@ +175,5 @@
> +    mPlugin->VideoDecoderDestroyed(this);
> +    mPlugin = nullptr;
> +  }
> +  mCanSendMessages = false;
> +  mCallback = nullptr;

Is this sufficient here? Do you not need to notify the callback that it's never going to receive more notifications?

@@ +187,5 @@
> +    return false;
> +  }
> +
> +  auto f = new GMPVideoi420FrameImpl(aDecodedFrame, &mVideoHost);
> +  if (!f) {

new is infallible.

::: xpcom/build/nsXPComInit.cpp
@@ +702,5 @@
>  #ifdef MOZ_VISUAL_EVENT_TRACER
>      mozilla::eventtracer::Init();
>  #endif
>  
> +    // TODO: Cache the GRE dir here instead of telling GeckoChildProcessHost to do it.

Nit: What's "to do"?

Comment 21

[Josh Aas]

Attachment: fix v1.8

This doesn't address all of Ben's latest comments yet, just saving my work at the end of the day.

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8420171 [details] [diff] [review]
fix v1.7

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

::: content/media/gmp/GMPParent.cpp
@@ +16,5 @@
> +
> +namespace mozilla {
> +namespace gmp {
> +
> +GMPParent::GMPParent()

This whole file could really use some thread assertions since none of it really happens on the main thread.

@@ +55,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsAutoCString path;
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(mDirectory->GetNativePath(path)));

I'm not sure we can guarantee that this always returns success. The window implementation looks like it actually can fail.

::: xpcom/build/nsXPComInit.cpp
@@ +704,5 @@
>  #endif
>  
> +    // TODO: Cache the GRE dir here instead of telling GeckoChildProcessHost to do it.
> +    //       Then have GeckoChildProcessHost get the dir from XPCOM::GetGREPath().
> +    mozilla::ipc::GeckoChildProcessHost::CacheGreDir();

Also, you should get bsmedberg to sign off on this part.

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8420171 [details] [diff] [review]
fix v1.7

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

::: content/media/gmp/GMPVideoEncodedFrameImpl.h
@@ +51,5 @@
> +  // when a consumer is finished or during XPCOM shutdown.
> +  void DoneWithAPI();
> +  // This is called when something has gone wrong - specicifically,
> +  // a child process has crashed. Does not attempt to release Shmem,
> +  // as the Shmem has already been released.

This comment isn't really correct... ActorDestroy is *always* called. It's DoneWithAPI() that may not be called.

Comment 24

[Josh Aas]

Attachment: fix v1.9

Comment 25

[Josh Aas]

Attachment: fix v2.0

Compile fixes for Linux and Windows.

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8423200 [details] [diff] [review]
fix v2.0

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

Ok, I think this looks good!

We need a followup to add tests of some kind.

::: content/media/gmp/GMPMessageUtils.h
@@ +136,5 @@
> +
> +  static void Write(Message* aMsg, const paramType& aParam)
> +  {
> +    WriteParam(aMsg, aParam.mCodecType);
> +    WriteParam(aMsg, nsAutoCString(aParam.mPLName));

This didn't get fixed from comment 12.

@@ +170,5 @@
> +      return false;
> +    }
> +    memcpy(aResult->mPLName, plName.get(), plName.Length());
> +    memset(aResult->mPLName + plName.Length(), 0, kGMPPayloadNameSize - plName.Length());
> +    aResult->mPLName[kGMPPayloadNameSize - 1] = 0;

Actually, this should be redundant given the previous memset(0)

::: content/media/gmp/GMPParent.h
@@ +51,5 @@
> +  void VideoDecoderDestroyed(GMPVideoDecoderParent* aDecoder);
> +  nsresult GetGMPVideoEncoder(GMPVideoEncoderParent** aGMPVE);
> +  void VideoEncoderDestroyed(GMPVideoEncoderParent* aEncoder);
> +  GMPState State() const;
> +  nsIThread* GMPThread();

This needs to be DEBUG-only or something... The cached member too.

::: content/media/gmp/GMPService.cpp
@@ +121,5 @@
> +
> +  nsCOMPtr<nsIObserverService> obsService = mozilla::services::GetObserverService();
> +  MOZ_ASSERT(obsService);
> +  obsService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
> +  obsService->AddObserver(this, NS_XPCOM_SHUTDOWN_THREADS_OBSERVER_ID, false);

These need MOZ_ALWAYS_TRUE(NS_SUCCEEDED(...))

@@ +129,5 @@
> +GeckoMediaPluginService::Observe(nsISupports* aSubject,
> +                                 const char* aTopic,
> +                                 const char16_t* aSomeData)
> +{
> +  if (!strcmp(NS_XPCOM_SHUTDOWN_OBSERVER_ID, aTopic)) {

Here I'd assert that mShuttingDown is false

@@ +143,5 @@
> +                           NS_DISPATCH_SYNC);
> +    } else {
> +      MOZ_ASSERT(mPlugins.IsEmpty());
> +    }
> +  } else if (!strcmp(NS_XPCOM_SHUTDOWN_THREADS_OBSERVER_ID, aTopic)) {

And here I'd assert that mShuttingDown is true.

Comment 27

[Josh Aas]

Attachment: fix v2.1

A lot of B2G compile fixes.

Comment 28

[Josh Aas]

Attachment: fix v2.2

One last B2G compile fix in this patch. Pushed to mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4b51aff4b3c

Comment 29

[Phil Ringnalda (:philor)]

Backed out in https://tbpl.mozilla.org/php/getParsedLog.php?id=39888474&tree=Mozilla-Inbound - there's probably some clear meaning in the pattern of "busted on Linux debug, b2g Mac desktop debug, b2g Win desktop debug" but I don't know what it is.

Comment 30

[Josh Aas]

Round 2, with a couple more fixes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fda65b2f990a
https://hg.mozilla.org/integration/mozilla-inbound/rev/246aa86a5e0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb98302ae5d

Seems to have stuck this time.

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/fda65b2f990a
https://hg.mozilla.org/mozilla-central/rev/246aa86a5e0e
https://hg.mozilla.org/mozilla-central/rev/5bb98302ae5d

Comment 32

[Josh Aas]

Attachment: Rough GMP Test Code

When I was working on the patches here I used this code to test. It's hooked into the media decoder code, test by hitting play and pause on the following HTML5 video:

http://www.w3.org/2010/05/video/mediaevents.html

It might be out of date by now, just wanted to save it somewhere in case someone finds it useful in the future.

Comment 33

[Steven Michaud [:smichaud] (Retired)]

Attachment: Rough GMP Test Code, updated to current trunk

I've banged my head against this for several hours, but it still doesn't work.

I get the following errors from the OpenH264 plugin:

[OpenH264] Error:ParamValidationExt(), invalid 0 x 0 in dependency layer settings!
[OpenH264] Error:WelsInitEncoderExt(), ParamValidationExt failed return 2.
[OpenH264] Error:CWelsH264SVCEncoder::Initialize(), WelsInitEncoderExt failed.
Assertion failed: (false), function Encode_w, file module/gmp-openh264.cpp, line 421.

Any ideas what's wrong, Josh?

Comment 34

[Steven Michaud [:smichaud] (Retired)]

I forgot to mention that MediaEncoder::Notify() finishes successfully 20 times in a row, but MediaDecoder::Decoded() never gets called.

Comment 35

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

Steven: to test OpenH264, point Nightly at http://mozilla.github.io/webrtc-landing/pc_test.html and check the "Require H.264" box.

It's a bit more complicated to test the GMPs from the MediaDecoder stack ATM, I'm still in the process of landing my patches, and I only have a test GMP for Win32 at the moment.

Comment 36

[Steven Michaud [:smichaud] (Retired)]

> http://mozilla.github.io/webrtc-landing/pc_test.html

Yes, I already know about this testcase.  It works fine for me, but I just want a greater variety of testcases to work with.

Specifically, I'd like to test the OpenH264 plugin with any HTML5 video, as Josh's old patch seems to promise (and if that's possible).  Should I just wait for you to finish your patches?  What's the bug number?