Closed Bug 1102607 Opened 8 years ago Closed 8 years ago

[EME] Proxy GMPStorage APIs to main thread if they're called off the main thread.

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: cpearce, Assigned: jwwang)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

The GMPStorage API asserts if it's called off the main thread. The APIs are async, so we could instead check if we're called on a non-main thread, and if we are, dispatch a task to the main thread to run the APIs.
JW: Can you take this bug?

We need to change GMPStorageChild and CreateRecord() and CreateRecordIterator() from GMPPlatform.cpp to be callable from non-main threads. They are async functions, so if they need to fail they can report failure through their results callbacks.

You may need to add a lock to GMPStorageChild to make its state threadsafe, and dispatch the calls to Send$IPDLMessage() as async dispatches.

We did this to GMPDecryptorChild, you can use that as an example. This uses a macro CALL_ON_GMP_THREAD which you can use to do async dispatches for the Send$IDPLMessage calls.
Assignee: nobody → jwwang
GMPStorageChild::CreateRecord doesn't look like an async function. It doesn't have IPDL transactions and has to return aOutRecord immediately. As for GMPStorageChild::EnumerateRecords, I am worried that the passed in pointers might become invalid by the time IPDL transaction is completed. Shouldn't we make GMPStorageChild::EnumerateRecords a sync function?
(In reply to JW Wang [:jwwang] from comment #2)
> GMPStorageChild::CreateRecord doesn't look like an async function. It
> doesn't have IPDL transactions and has to return aOutRecord immediately.

Yes, CreateRecord is synchronous. What I mean is we want it to be threadsafe. So making its internals covered by a monitor so that CreateRecord can be called from any thread.

> As
> for GMPStorageChild::EnumerateRecords, I am worried that the passed in
> pointers might become invalid by the time IPDL transaction is completed.

It's the caller's responsibility to ensure that doesn't happen. We also cancel the callbacks when we receive a shutdown notification.

> Shouldn't we make GMPStorageChild::EnumerateRecords a sync function?

EnumerateRecords has to do IPC as the storage is performed in the parent process, so it can't be synchronous unless we pre-populate the list at startup and then maintain a copy on the child side. I'm not super keen on keeping that second copy up to date.
Should we also make GMPStorageChild::Open, Read, Write, and Close callable off the main thread? It is strange to offload part of the APIs from the main thread.
Yes please.
Make GMPStorageChild APIs callable off the main thread:

1. Remove GMPRecordImpl::mIsClosed/MarkClosed so we don't have to maintain states across classes.

2. GMPStorageChild::Open/Read/Write forward the request to OpenAsync/ReadAsync/WriteAsync when not called in the gmp thread. Otherwise they process the request immediately. If GMPNoErr is returned, the client will receive results through GMPRecordClient::*Complete callbacks.

3. GMPStorageChild::Open/Read/Write/Recv*Complete don't close the record when error happens. This simplifies the internal state management of GMPStorageChild. The client is free to try again or just close the record. 

Try: https://tbpl.mozilla.org/?tree=Try&rev=78aa360beda6
Attachment #8529021 - Flags: review?(rjesup)
Comment on attachment 8529021 [details] [diff] [review]
1102607_off_main_thread_GMPStorage_calls-v1.patch

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

::: dom/media/gmp/GMPStorageChild.cpp
@@ -22,5 @@
>  
>  GMPErr
>  GMPRecordImpl::Open()
>  {
> -  if (!mIsClosed) {

Please don't remove the if (GMP_FAILED(err)) Close() checks; if a file operation fails, how can the client just trying again succeed? We should close the record on failure.

@@ +143,5 @@
> +  } else {
> +    auto task = NewRunnableMethod(
> +        this, &GMPStorageChild::CreateRecordInGMPThread,
> +        &rv, aRecordName, aOutRecord, aClient);
> +    SyncDispatch(task, mPlugin->GMPMessageLoop());

I'd prefer to not block waiting for the main thread to run the task. A poorly written GMP could deadlock due to this.

In the !ON_GMP_THREAD() case, please asynchronously dispatch a task to perform the create-record, and in that task call the GMPRecordClient's OpenComplete() with an error code if this fails.

The API has an async channel for reporting errors, we should use it.
Attachment #8529021 - Flags: review-
Comment on attachment 8529021 [details] [diff] [review]
1102607_off_main_thread_GMPStorage_calls-v1.patch

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

::: dom/media/gmp/GMPStorageChild.cpp
@@ -22,5 @@
>  
>  GMPErr
>  GMPRecordImpl::Open()
>  {
> -  if (!mIsClosed) {

I find it complicated to maintain the "closed" state across GMPRecordImpl and GMPStorageChild. I would prefer to forward the open/read/write request to GMPStorageChild which will decide whether the record is closed or not and return an error if necessary. By doing so, I am able to get rid of GMPRecordImpl::mIsClosed.

Since the client has to call GMPRecordImpl::Close to free the memory allocated for GMPRecordImpl in the end no matter GMPRecordImpl::Open/Read/Write succeed or not, I think it is OK to defer the GMPStorageChild::Close call until GMPRecordImpl::Close is called without proactively doing GMPStorageChild::Close in GMPStorageChild::Open/Read/Write errors.

@@ +122,3 @@
>    // Delete our self reference.
>    Release();
> +  return mOwner->Close(this);

GMPStorageChild::Close will do sync dispatch when GMPRecordImpl::Close not called on the GMP thread so we can get its return value. If we don't care about the return value of GMPStorageChild::Close (in fact there isn't much we can do when failing to close something), we can get rid of sync dispatch.

@@ +143,5 @@
> +  } else {
> +    auto task = NewRunnableMethod(
> +        this, &GMPStorageChild::CreateRecordInGMPThread,
> +        &rv, aRecordName, aOutRecord, aClient);
> +    SyncDispatch(task, mPlugin->GMPMessageLoop());

GMPStorageChild::CreateRecord is a sync function and has to return a newly allocated GMPRecord immediately. Or we change the API and have the client receive the newly allocated GMPRecord in the CreateComplete() (which is also a new function) callback?
Attachment #8529021 - Flags: review?(rjesup)
(In reply to JW Wang [:jwwang] from comment #8)
> Comment on attachment 8529021 [details] [diff] [review]

> Since the client has to call GMPRecordImpl::Close to free the memory
> allocated for GMPRecordImpl in the end no matter
> GMPRecordImpl::Open/Read/Write succeed or not, I think it is OK to defer the
> GMPStorageChild::Close call until GMPRecordImpl::Close is called without
> proactively doing GMPStorageChild::Close in GMPStorageChild::Open/Read/Write
> errors.

OK, fair enough.



> 
> @@ +122,3 @@
> >    // Delete our self reference.
> >    Release();
> > +  return mOwner->Close(this);
> 
> GMPStorageChild::Close will do sync dispatch when GMPRecordImpl::Close not
> called on the GMP thread so we can get its return value. If we don't care
> about the return value of GMPStorageChild::Close (in fact there isn't much
> we can do when failing to close something), we can get rid of sync dispatch.

I don't think there is much we can do if Close() fails, provided we're still cleaning up properly, so please get rid of the sync dispatch.


> 
> @@ +143,5 @@
> > +  } else {
> > +    auto task = NewRunnableMethod(
> > +        this, &GMPStorageChild::CreateRecordInGMPThread,
> > +        &rv, aRecordName, aOutRecord, aClient);
> > +    SyncDispatch(task, mPlugin->GMPMessageLoop());
> 
> GMPStorageChild::CreateRecord is a sync function and has to return a newly
> allocated GMPRecord immediately. Or we change the API and have the client
> receive the newly allocated GMPRecord in the CreateComplete() (which is also
> a new function) callback?

You don't need to do the dispatch if you protect GMPStorageChild's thread-shared state with a mutex.
Attachment #8529021 - Attachment is obsolete: true
Attachment #8529524 - Flags: review?(cpearce)
Comment on attachment 8529524 [details] [diff] [review]
1102607_off_main_thread_GMPStorage_calls-v2.patch

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

OK, but I think we can make it simpler...

::: dom/media/gmp/GMPStorageChild.cpp
@@ +148,5 @@
>      return GMPClosedErr;
>    }
> +
> +  if (!ContainRecord(aRecord->Name())) {
> +    // Record not opened.

"// Record not opened." is not a good comment here, since we're in the process of opening. Maybe "Trying to re-open a record that has already been closed."

@@ +153,4 @@
>      return GMPClosedErr;
>    }
> +
> +  if (!SendOpen(aRecord->Name())) {

You don't need to check for failure of the Send$Message functions. I wasn't sure if we needed to previously, so I was checking the return value, but I stepped through the code in the child process and forced GMPVideoDecoder::SendDecoded to fail, and the channel send error handling code ended up calling GMPChild::ProcessingError(), which MOZ_CRASH's.

So we can assume that if any of our Send$Message() calls fail, we'll abort, so we don't need to handle their returns value.

So, here you can just call:

CAL_ON_GMP_THREAD(SendOpen, aRecord->Name());

And then you don't need OpenAsync, provided you hold the lock for the body of this function.

The same thing applies to the Read and Write functions below.

That should make things simpler...
Attachment #8529524 - Flags: review?(cpearce) → review-
Attachment #8529524 - Attachment is obsolete: true
Attachment #8530054 - Flags: review?(cpearce)
Comment on attachment 8530054 [details] [diff] [review]
1102607_off_main_thread_GMPStorage_calls-v3.patch

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

Looks good.

::: dom/media/gmp/GMPStorageChild.cpp
@@ +343,5 @@
>    mPendingRecordIterators.pop();
>  
>    if (GMP_FAILED(aStatus)) {
> +    // Unlock before calling back to the client which might grab another lock.
> +    MonitorAutoUnlock unlock(mMonitor);

Just wrap the code above this in {} which restricts the monitor lock to the code above this, then you don't need the two unlocks here, and then the needless relock of the monitor only to unlock when the function exits.
Attachment #8530054 - Flags: review?(cpearce) → review+
We should also exercise this in a gtest. We can add a case to FakeDecryptor::TestStorage() in media/gmp-plugin/gmp-test-decryptor.cpp, and that will be exercised in the GMPStorageBasic test in TestGMPCrossOrigin.cpp.

We can do that in a separate patch and/or bug.
Comment on attachment 8530054 [details] [diff] [review]
1102607_off_main_thread_GMPStorage_calls-v3.patch

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

::: dom/media/gmp/GMPStorageChild.cpp
@@ +343,5 @@
>    mPendingRecordIterators.pop();
>  
>    if (GMP_FAILED(aStatus)) {
> +    // Unlock before calling back to the client which might grab another lock.
> +    MonitorAutoUnlock unlock(mMonitor);

I tried.

RecordIteratorContext ctx;
{
  MonitorAutoLock lock(mMonitor);
  ctx = mPendingRecordIterators.front();
}

But it failed for RecordIteratorContext has no default constructor. You want me to add one?
(In reply to Chris Pearce (:cpearce) from comment #14)
> We should also exercise this in a gtest. We can add a case to
> FakeDecryptor::TestStorage() in media/gmp-plugin/gmp-test-decryptor.cpp, and
> that will be exercised in the GMPStorageBasic test in TestGMPCrossOrigin.cpp.
> 
> We can do that in a separate patch and/or bug.

Sure. Any way to run FakeDecryptor::TestStorage on another thread?
Sure, add the default constructor.
(In reply to JW Wang [:jwwang] from comment #16)
> (In reply to Chris Pearce (:cpearce) from comment #14)
> > We should also exercise this in a gtest. We can add a case to
> > FakeDecryptor::TestStorage() in media/gmp-plugin/gmp-test-decryptor.cpp, and
> > that will be exercised in the GMPStorageBasic test in TestGMPCrossOrigin.cpp.
> > 
> > We can do that in a separate patch and/or bug.
> 
> Sure. Any way to run FakeDecryptor::TestStorage on another thread?

I think you should test both main and off-main thread access, so add a case that uses storage off-main thread.

You need to create a thread, post a task to it to use storage. Create and write, then read and verify or some thing like that. Then post a task to the main thread to set a flag, and call MaybeFinish.

So you don't need to make all of FakeDecryptor::TestStorage run off main thread, just one of its async cases.

For example, I tested that timers don't work OMT in my test plugin like so:
https://github.com/cpearce/gmp-clearkey/blob/master/src/Decryptor.cpp#L297

Obviously we want to be testing that storage *does* work.

And maybe we should make timers work OMT as well (in another bug!).
Fix nits in comment 13.
Attachment #8530054 - Attachment is obsolete: true
Attachment #8530148 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4e3209930220
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Blocks: 1107545
Mass update firefox-status to track EME uplift.
You need to log in before you can comment on or make changes to this bug.