1048107 - [EME] Implement GMPTimer and clock

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

Implement GMPTimer interfaces, and the GMPGetCurrentTime function that lives in GMPPlatformAPI.

Comment 1

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

Attachment: Patch v1

Implements GMPTimer. Service is requested and instantiated on the child process side first, unlike our other interfaces.

Also adds a placeholder implementation of GMPGetCurrentTime.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8466877 [details] [diff] [review]
Patch v1

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

::: content/media/gmp/GMPChild.h
@@ +62,5 @@
>  
>    virtual void ActorDestroy(ActorDestroyReason aWhy) MOZ_OVERRIDE;
>    virtual void ProcessingError(Result aWhat) MOZ_OVERRIDE;
>  
> +  nsRefPtr<GMPTimerChild> mTimerChild;  

spaces

::: content/media/gmp/GMPTimerChild.cpp
@@ +41,5 @@
> +    return GMPQuotaExceededErr;
> +  }
> +  uint32_t timerId = mTimerCount;
> +  mTimers.Put(timerId, aTask);
> +  mTimerCount++;

thread assertion, since we're not locking? However, maybe irrelevant since we call SendSetTimer(), and I think the IPC code asserts on threads.

Comment 3

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

(In reply to Randell Jesup [:jesup] from comment #2)
> ::: content/media/gmp/GMPTimerChild.cpp
> @@ +41,5 @@
> > +    return GMPQuotaExceededErr;
> > +  }
> > +  uint32_t timerId = mTimerCount;
> > +  mTimers.Put(timerId, aTask);
> > +  mTimerCount++;
> 
> thread assertion, since we're not locking? However, maybe irrelevant since
> we call SendSetTimer(), and I think the IPC code asserts on threads.


We return failure on line 35 if this is called on a non-main thread. We could assert as well, so that the issue is more visible to plugin authors?

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/83d8226768ee

Comment 5

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7b2c2cc747db for build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=45200545&tree=Mozilla-Inbound

Comment 6

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

Comment on attachment 8466877 [details] [diff] [review]
Patch v1

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

::: content/media/gmp/GMPParent.cpp
@@ +627,5 @@
> +
> +PGMPTimerParent*
> +GMPParent::AllocPGMPTimerParent()
> +{
> +  GMPTimerParent* p = new GMPTimerParent(GMPThread());

Bustage fix: GMPThread() is only defined in debug builds. It needs to be present in opt builds too, and I think we're better off naming it GetGMPThread(), so that it doesn't conflict with the name of the threads that we expose to the GMPs; GMPThread.

::: content/media/gmp/GMPTimerParent.cpp
@@ +64,5 @@
> +void
> +GMPTimerParent::GMPTimerExpired(nsITimer *aTimer, void *aClosure)
> +{
> +  MOZ_ASSERT(aClosure);
> +  nsAutoPtr<Context> ctx = static_cast<Context*>(aClosure);

Bustage fix:

nsAutoPtr<Context> ctx(static_cast<Context*>(aClosure));

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/238e2c78035c

Comment 8

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

Somehow I managed to not include my bustage fix in my push when I pushed to inbound...
Fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb757252c04

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/238e2c78035c
https://hg.mozilla.org/mozilla-central/rev/7cb757252c04