Refactoring to introduce refcount to WebMediaPlayerImpl...

Refactoring to introduce refcount to WebMediaPlayerImpl
WebMediaPlayerImpl interacts with multiple threads that it
becomes necessary to make it refcounted so we can post task
on different threads' message loop.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=20038

[Alpha Left Google, 2009-06-26 20:48:14]

darin: webkit/api/* & webkit/glue/webmediaplayer_impl.{cc, h}
scherkus: webkit/glue/webmediaplayer_impl.{cc, h} and webkit/glue/media/*

This change is to add refcounting to WebMediaPlayerImpl, which is needed to do a
lot of multi-thread operations.

[Alpha Left Google, 2009-06-26 23:26:02]

+ajwong

[Alpha Left Google, 2009-06-26 23:26:15]

+ajwong

[awong, 2009-06-26 23:42:29]

Some comments regarding commenting. :)

Also, one fairly serious change in API design for accessing client_.

http://codereview.chromium.org/147225/diff/1011/21
File webkit/api/src/WebMediaPlayerClientImpl.cpp (right):

http://codereview.chromium.org/147225/diff/1011/21#newcode113
Line 113: // Destroy the last WebMediaPlayer.
oh wow...I'm scared of this code.  When will this ever happen?

Can we just assert(!m_webMediaPlayer)?

http://codereview.chromium.org/147225/diff/1011/22
File webkit/api/src/WebMediaPlayerClientImpl.h (right):

http://codereview.chromium.org/147225/diff/1011/22#newcode50
Line 50: ~WebMediaPlayerClientImpl();
virtual?

http://codereview.chromium.org/147225/diff/1011/22#newcode103
Line 103: WebMediaPlayer* m_webMediaPlayer;
Document that you must call Create/Destroy as a pair for this otherwise it may
leak?

http://codereview.chromium.org/147225/diff/1011/19
File webkit/glue/temporary_glue.cc (right):

http://codereview.chromium.org/147225/diff/1011/19#newcode23
Line 23: // TODO(hclam): this is a hackish way of getting to the type of
refcount-
First sentence is better written as: "This is a hack to get the refcount-aware
type."

http://codereview.chromium.org/147225/diff/1011/19#newcode25
Line 25: webkit_glue::WebMediaPlayerImpl* player_impl =
At the top of this file, we need to explain the memory ownership semantics and
the interplay between the objects in some detail since it is non-standard and
non-trivial. Please fill in?

http://codereview.chromium.org/147225/diff/1011/19#newcode29
Line 29: // Increment a reference to the media player.
Stating why in the comment is better.  So something like

"Increment the reference to simulate WebMediaPlayerClient taking a reference on
this object."

http://codereview.chromium.org/147225/diff/1011/19#newcode38
Line 38: // Instruct the media player to destroy internal resources so it stops
rather than "stops activities" how about "stop trying to use the associated
WebMediaPlayerClient"

http://codereview.chromium.org/147225/diff/1011/19#newcode42
Line 42: // Decrement a reference to it, so it is ready to be destroyed.
Similarily, say something like:

"Decrement the reference to simulate WebMediaPlayerClient releasing this
object."

http://codereview.chromium.org/147225/diff/1011/17
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/147225/diff/1011/17#newcode23
Line 23: // Limits the maximum outstanding repaints posted on render thread.
Explain why 50 is a sane number.

http://codereview.chromium.org/147225/diff/1011/17#newcode71
Line 71: DCHECK(client_);
For this type of semantic to be safe, we should create an accessor client() that
doe the following:

WebMediaPlayerClient* client() {
  DCHECK(PlatformThread::GetCurrentThreadId() == readner_thread);  // or
whatever the exact syntax is
  return client_;
}

And then always reference client_ through this accessor.

Our threading assumption very strongly expects that client is only accessed on
one thread.

For clarity, I would also use a set_client() function with the same dcheck.

http://codereview.chromium.org/147225/diff/1011/17#newcode317
Line 317: void WebMediaPlayerImpl::Destroy() {
Destroy is too generic.  Pick a name that implies, strongly, that we'll stop
using the WebMediaPlayer that we were initialized with.

We want this because our lifetime semantics are very tricky between these two
objects, and we want to code in the destroy call to explicitly express that "we
will no longer call the WebMediaPlayerClient object."

http://codereview.chromium.org/147225/diff/1011/18
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/147225/diff/1011/18#newcode9
Line 9: // of this class, so we need to be extra careful about concurrent access
of
Add a comment here again about the ownership relation to the
WebMediaPlayerClient, and tell peopel to read the comment in the temporary glue
(or vice versa if you put the main ownership comment here).

http://codereview.chromium.org/147225/diff/1011/18#newcode169
Line 169: void Destroy();
Do you mean Destory, or DetachWebMediaPlayerClient()?

[Alpha Left Google, 2009-06-29 18:07:48]

http://codereview.chromium.org/147225/diff/1011/21
File webkit/api/src/WebMediaPlayerClientImpl.cpp (right):

http://codereview.chromium.org/147225/diff/1011/21#newcode113
Line 113: // Destroy the last WebMediaPlayer.
On 2009/06/26 23:42:29, awong wrote:
> oh wow...I'm scared of this code.  When will this ever happen?
> 
> Can we just assert(!m_webMediaPlayer)?

You are allowed in js to do:
<video id=v></video>

var v = document.getElementById('v');
v.src = "a.mp4";
v.load();
v.src = "b.mp4";
v.load();

It is only because our media player can be loaded once that we have this logic.
This was handled by OwnPtr that it deletes the previous object.

http://codereview.chromium.org/147225/diff/1011/22
File webkit/api/src/WebMediaPlayerClientImpl.h (right):

http://codereview.chromium.org/147225/diff/1011/22#newcode50
Line 50: ~WebMediaPlayerClientImpl();
On 2009/06/26 23:42:29, awong wrote:
> virtual?

Done.

http://codereview.chromium.org/147225/diff/1011/22#newcode103
Line 103: WebMediaPlayer* m_webMediaPlayer;
On 2009/06/26 23:42:29, awong wrote:
> Document that you must call Create/Destroy as a pair for this otherwise it may
> leak?

Done.

http://codereview.chromium.org/147225/diff/1011/19
File webkit/glue/temporary_glue.cc (right):

http://codereview.chromium.org/147225/diff/1011/19#newcode23
Line 23: // TODO(hclam): this is a hackish way of getting to the type of
refcount-
On 2009/06/26 23:42:29, awong wrote:
> First sentence is better written as: "This is a hack to get the refcount-aware
> type."

Done.

http://codereview.chromium.org/147225/diff/1011/19#newcode25
Line 25: webkit_glue::WebMediaPlayerImpl* player_impl =
On 2009/06/26 23:42:29, awong wrote:
> At the top of this file, we need to explain the memory ownership semantics and
> the interplay between the objects in some detail since it is non-standard and
> non-trivial. Please fill in?

This file should be used not just for media, so i put it on top of the create
method.

http://codereview.chromium.org/147225/diff/1011/19#newcode29
Line 29: // Increment a reference to the media player.
On 2009/06/26 23:42:29, awong wrote:
> Stating why in the comment is better.  So something like
> 
> "Increment the reference to simulate WebMediaPlayerClient taking a reference
on
> this object."

Done.

http://codereview.chromium.org/147225/diff/1011/19#newcode38
Line 38: // Instruct the media player to destroy internal resources so it stops
On 2009/06/26 23:42:29, awong wrote:
> rather than "stops activities" how about "stop trying to use the associated
> WebMediaPlayerClient"

Done.

http://codereview.chromium.org/147225/diff/1011/19#newcode42
Line 42: // Decrement a reference to it, so it is ready to be destroyed.
On 2009/06/26 23:42:29, awong wrote:
> Similarily, say something like:
> 
> "Decrement the reference to simulate WebMediaPlayerClient releasing this
> object."

Done.

http://codereview.chromium.org/147225/diff/1011/17
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/147225/diff/1011/17#newcode23
Line 23: // Limits the maximum outstanding repaints posted on render thread.
On 2009/06/26 23:42:29, awong wrote:
> Explain why 50 is a sane number.

Done.

http://codereview.chromium.org/147225/diff/1011/17#newcode71
Line 71: DCHECK(client_);
On 2009/06/26 23:42:29, awong wrote:
> For this type of semantic to be safe, we should create an accessor client()
that
> doe the following:
> 
> WebMediaPlayerClient* client() {
>   DCHECK(PlatformThread::GetCurrentThreadId() == readner_thread);  // or
> whatever the exact syntax is
>   return client_;
> }
> 
> And then always reference client_ through this accessor.
> 
> Our threading assumption very strongly expects that client is only accessed on
> one thread.
> 
> For clarity, I would also use a set_client() function with the same dcheck.

Done. client_ is only set in construct, so we can skip the set* function.

http://codereview.chromium.org/147225/diff/1011/17#newcode317
Line 317: void WebMediaPlayerImpl::Destroy() {
On 2009/06/26 23:42:29, awong wrote:
> Destroy is too generic.  Pick a name that implies, strongly, that we'll stop
> using the WebMediaPlayer that we were initialized with.
> 
> We want this because our lifetime semantics are very tricky between these two
> objects, and we want to code in the destroy call to explicitly express that
"we
> will no longer call the WebMediaPlayerClient object."

Done.

http://codereview.chromium.org/147225/diff/1011/18
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/147225/diff/1011/18#newcode9
Line 9: // of this class, so we need to be extra careful about concurrent access
of
On 2009/06/26 23:42:29, awong wrote:
> Add a comment here again about the ownership relation to the
> WebMediaPlayerClient, and tell peopel to read the comment in the temporary
glue
> (or vice versa if you put the main ownership comment here).

Done.

http://codereview.chromium.org/147225/diff/1011/18#newcode169
Line 169: void Destroy();
On 2009/06/26 23:42:29, awong wrote:
> Do you mean Destory, or DetachWebMediaPlayerClient()?

Done.

[darin (slow to review), 2009-06-29 19:51:54]

http://codereview.chromium.org/147225/diff/3011/2008
File webkit/api/src/TemporaryGlue.h (right):

http://codereview.chromium.org/147225/diff/3011/2008#newcode49
Line 49: static void destroyWebMediaPlayer(WebMediaPlayer*);
this looks like an improper use of TemporaryGlue.  please do not commit this!

[darin (slow to review), 2009-06-29 19:54:13]

WebMediaPlayer is supposed to be destroyed by having the WebKit API
implementation layer call 'delete' on the WebMediaPlayer pointer.

If the problem is that WebMediaPlayerImpl interacts with multiple threads, then
I think the correct solution is to move that multi-thread access to a new class
that WebMediaPlayerImpl can own a reference to and delegate certain methods to.

[darin (slow to review), 2009-06-29 19:54:35]

I think it is wrong for WebMediaPlayerImpl itself to be accessed on multiple
threads.

[scherkus (not reviewing), 2009-06-29 20:13:00]

On 2009/06/29 19:54:35, darin wrote:
> I think it is wrong for WebMediaPlayerImpl itself to be accessed on multiple
> threads.

Just WebMediaPlayerImpl itself, or even a refcounted class that can be delegated
to?

Video decoding+rendering runs on a separate thread+message loop from the render
thread, and my understanding is we need to use RefCountedThreadSafe in order to
post a task to the render thread from the video decoding thread.

If this is solved with another delegating layer, then so be it!

[darin (slow to review), 2009-06-29 21:14:54]

> If this is solved with another delegating layer, then so be it!

It doesn't have to be a heavy-weight delegate.  The thread-safe ref-counted
internal class can hold a pointer back to the WebMediaPlayer.  The
WebMediaPlayer upon destruction can clear that pointer and release its reference
to the inner class.

The inner class can PostTask using NewRunnableMethod(this...) to jump between
threads.  When it posts a task back to the WebKit thread, it will just have to
null check the WebMediaPlayer pointer before dereferencing it, given that it may
disappear at any time.

[scherkus (not reviewing), 2009-06-29 21:31:13]

On 2009/06/29 21:14:54, darin wrote:
> > If this is solved with another delegating layer, then so be it!
> 
> It doesn't have to be a heavy-weight delegate.  The thread-safe ref-counted
> internal class can hold a pointer back to the WebMediaPlayer.  The
> WebMediaPlayer upon destruction can clear that pointer and release its
reference
> to the inner class.
> 
> The inner class can PostTask using NewRunnableMethod(this...) to jump between
> threads.  When it posts a task back to the WebKit thread, it will just have to
> null check the WebMediaPlayer pointer before dereferencing it, given that it
may
> disappear at any time.

Great -- thanks for the pointers :P

[Alpha Left Google, 2009-06-29 21:32:28]

Thanks for the recommendations. I'll modify it as such.

[Alpha Left Google, 2009-07-01 01:47:33]

I modified the code so we only have a small ref-counted proxy class that
delegates call to/from different threads of the media pipeline.

[darin (slow to review), 2009-07-01 06:32:48]

nice!  i just gave this a quick look...

http://codereview.chromium.org/147225/diff/2023/3035
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/147225/diff/2023/3035#newcode90
Line 90: class Proxy : public base::RefCountedThreadSafe<Proxy> {
can this be moved into the private section?  or, can it be moved into the .cc? 
you can see examples of inner classes being defined in .cc files if you look at
net/base/file_stream_*

[Alpha Left Google, 2009-07-01 22:44:01]

http://codereview.chromium.org/147225/diff/2023/3035
File webkit/glue/webmediaplayer_impl.h (right):

http://codereview.chromium.org/147225/diff/2023/3035#newcode90
Line 90: class Proxy : public base::RefCountedThreadSafe<Proxy> {
On 2009/07/01 06:32:49, darin wrote:
> can this be moved into the private section?  or, can it be moved into the .cc?

> you can see examples of inner classes being defined in .cc files if you look
at
> net/base/file_stream_*

It cannot be made private or defined in the .cc file, because it is for the
media pipeline (from different threads) to delegate methods calls to
WebMediaPlayerImpl on render thread. It works as a public interface for the
media pipeline to completely isolate WebMediaPlayerImpl from media   threads.

[darin (slow to review), 2009-07-02 05:51:49]

> It cannot be made private or defined in the .cc file, because it is for the
> media pipeline (from different threads) to delegate methods calls to
> WebMediaPlayerImpl on render thread. It works as a public interface for the
> media pipeline to completely isolate WebMediaPlayerImpl from media   threads.

Ah, sorry... I should have reviewed further before commenting :(

[scherkus (not reviewing), 2009-07-02 22:01:18]

http://codereview.chromium.org/147225/diff/2023/3034
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/147225/diff/2023/3034#newcode37
Line 37: WebKit::WebMediaPlayerClient* client,
so this confuses me a bit... I thought WMPImpl was going to still hold the
client, and that the Proxy would be more lightweight (i.e., forwards method
calls between WMPImpl and video renderer, but WMPImpl still talks to client).

For example the network/ready state logic is sort of split up between the proxy
and WMPImpl (proxy calls set on WMPImpl and then calls the client)

Is there an advantage to having the client inside Proxy vs. WMPImpl?

http://codereview.chromium.org/147225/diff/2023/3034#newcode84
Line 84: DCHECK(MessageLoop::current() == render_loop_);
do you know of a way we can set current()?

I'm thinking about unit tests where we wouldn't have created a thread+message
loop

not a big deal just curious

http://codereview.chromium.org/147225/diff/2023/3034#newcode126
Line 126: void WebMediaPlayerImpl::Proxy::SetVideoRenderer(
should we DCHECK the message loop here?

http://codereview.chromium.org/147225/diff/2023/3034#newcode132
Line 132: void WebMediaPlayerImpl::Proxy::Paint(skia::PlatformCanvas* canvas,
should we DCHECK the message loop here?

http://codereview.chromium.org/147225/diff/2023/3034#newcode139
Line 139: void WebMediaPlayerImpl::Proxy::SetSize(const gfx::Rect& rect) {
should we DCHECK the message loop here?

http://codereview.chromium.org/147225/diff/2023/3034#newcode213
Line 213: DCHECK(main_loop_ && MessageLoop::current() == main_loop_);
all these DCHECKs verify main_loop_ exists but from what I can tell it never
gets changed after it is set inside the contructor

http://codereview.chromium.org/147225/diff/2023/3034#newcode388
Line 388: Destroy();
should we set message_loop_ to NULL?

http://codereview.chromium.org/147225/diff/2023/3034#newcode407
Line 407: pipeline_.Stop();
since this can get called via WillDestroyCurrentMessageLoop I'd add a TODO
saying this could block for a long time

[Alpha Left Google, 2009-07-03 01:50:56]

done!

http://codereview.chromium.org/147225/diff/2023/3034
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/147225/diff/2023/3034#newcode37
Line 37: WebKit::WebMediaPlayerClient* client,
On 2009/07/02 22:01:18, scherkus wrote:
> so this confuses me a bit... I thought WMPImpl was going to still hold the
> client, and that the Proxy would be more lightweight (i.e., forwards method
> calls between WMPImpl and video renderer, but WMPImpl still talks to client).
> 
> For example the network/ready state logic is sort of split up between the
proxy
> and WMPImpl (proxy calls set on WMPImpl and then calls the client)
> 
> Is there an advantage to having the client inside Proxy vs. WMPImpl?

Your comment makes sense to me, I'll do the change.

http://codereview.chromium.org/147225/diff/2023/3034#newcode84
Line 84: DCHECK(MessageLoop::current() == render_loop_);
On 2009/07/02 22:01:18, scherkus wrote:
> do you know of a way we can set current()?
> 
> I'm thinking about unit tests where we wouldn't have created a thread+message
> loop
> 
> not a big deal just curious

MessageLoop::current() is in thread local, so when you instantiate a MessageLoop
in the current thread, current() is set.

http://codereview.chromium.org/147225/diff/2023/3034#newcode126
Line 126: void WebMediaPlayerImpl::Proxy::SetVideoRenderer(
On 2009/07/02 22:01:18, scherkus wrote:
> should we DCHECK the message loop here?

Not in this method, this method is a hack to allow VideoRenderer to hook itself
to this class.

http://codereview.chromium.org/147225/diff/2023/3034#newcode132
Line 132: void WebMediaPlayerImpl::Proxy::Paint(skia::PlatformCanvas* canvas,
On 2009/07/02 22:01:18, scherkus wrote:
> should we DCHECK the message loop here?

Done.

http://codereview.chromium.org/147225/diff/2023/3034#newcode139
Line 139: void WebMediaPlayerImpl::Proxy::SetSize(const gfx::Rect& rect) {
On 2009/07/02 22:01:18, scherkus wrote:
> should we DCHECK the message loop here?

Done.

http://codereview.chromium.org/147225/diff/2023/3034#newcode213
Line 213: DCHECK(main_loop_ && MessageLoop::current() == main_loop_);
On 2009/07/02 22:01:18, scherkus wrote:
> all these DCHECKs verify main_loop_ exists but from what I can tell it never
> gets changed after it is set inside the contructor

ok :) I'll change them.

http://codereview.chromium.org/147225/diff/2023/3034#newcode388
Line 388: Destroy();
On 2009/07/02 22:01:18, scherkus wrote:
> should we set message_loop_ to NULL?

Done.

http://codereview.chromium.org/147225/diff/2023/3034#newcode407
Line 407: pipeline_.Stop();
On 2009/07/02 22:01:18, scherkus wrote:
> since this can get called via WillDestroyCurrentMessageLoop I'd add a TODO
> saying this could block for a long time

Done.

[scherkus (not reviewing), 2009-07-06 19:57:46]

tiny nits

http://codereview.chromium.org/147225/diff/4002/4003
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/147225/diff/4002/4003#newcode87
Line 87: if (webmediaplayer_ && webmediaplayer_->GetClient())
nit: I don't think Proxy should know about the client at all

proxy should delegate to webmediaplayer_, even if webmediaplayer_ ends up
delegating to the client :)

http://codereview.chromium.org/147225/diff/4002/4003#newcode93
Line 93: if (webmediaplayer_ && webmediaplayer_->GetClient())
same here, just delegate to a time changed method in webmediaplayer_, who is
responsible for notifying the client

[Alpha Left Google, 2009-07-06 20:14:56]

http://codereview.chromium.org/147225/diff/4002/4003
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/147225/diff/4002/4003#newcode87
Line 87: if (webmediaplayer_ && webmediaplayer_->GetClient())
On 2009/07/06 19:57:47, scherkus wrote:
> nit: I don't think Proxy should know about the client at all
> 
> proxy should delegate to webmediaplayer_, even if webmediaplayer_ ends up
> delegating to the client :)

Done.

http://codereview.chromium.org/147225/diff/4002/4003#newcode93
Line 93: if (webmediaplayer_ && webmediaplayer_->GetClient())
On 2009/07/06 19:57:47, scherkus wrote:
> same here, just delegate to a time changed method in webmediaplayer_, who is
> responsible for notifying the client

Done.

[scherkus (not reviewing), 2009-07-06 20:19:54]

the heck... rietveld is crapping out on me

lgtm!