Add OnRemoved() in VideoCapture::EventHandler API

Add OnRemoved() in VideoCapture::EventHandler API
This is to allow client to know when the event handler can be deleted.

BUG=none
TEST=trybots
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=103016

[viettrungluu, 2011-09-27 00:04:57]

LGTM, though maybe expand some comments as indicated.

http://codereview.chromium.org/8037055/diff/1/media/video/capture/video_captu...
File media/video/capture/video_capture.h (right):

http://codereview.chromium.org/8037055/diff/1/media/video/capture/video_captu...
media/video/capture/video_capture.h:69: // Notify client that the client has
been removed.
"[...] has been removed and no more calls will be received."?

http://codereview.chromium.org/8037055/diff/1/media/video/capture/video_captu...
media/video/capture/video_capture.h:99: virtual void StartCapture(EventHandler*
handler,
Maybe add here: "|handler| must remain valid until it has received
|OnRemoved()|.".

[wjia(left Chromium), 2011-09-27 00:26:17]

Need stamp from media/ owner.

http://codereview.chromium.org/8037055/diff/1/media/video/capture/video_captu...
File media/video/capture/video_capture.h (right):

http://codereview.chromium.org/8037055/diff/1/media/video/capture/video_captu...
media/video/capture/video_capture.h:69: // Notify client that the client has
been removed.
On 2011/09/27 00:04:58, viettrungluu wrote:
> "[...] has been removed and no more calls will be received."?

Done.

http://codereview.chromium.org/8037055/diff/1/media/video/capture/video_captu...
media/video/capture/video_capture.h:99: virtual void StartCapture(EventHandler*
handler,
On 2011/09/27 00:04:58, viettrungluu wrote:
> Maybe add here: "|handler| must remain valid until it has received
> |OnRemoved()|.".

Done.

[Ami GONE FROM CHROMIUM, 2011-09-27 03:21:47]

I see no problems with the code that's here, but I'm confused.
Is the expectation that |handler| deletes itself when its OnRemoved() method is
called?
I'm having a hard time figuring out who has the authority to call a method named
"OnRemoved()" that doesn't alternatively have the authority to simply delete
|handler|, instead.  In other words why not use ~EventHandler() instead of
OnRemoved?

Might be easier to understand this change if there was at least one
not-NOTIMPLEMENTED version of OnRemoved().  Or a test.

Note that NOTIMPLEMENTED is a LOG(ERROR) by default.  Do you really want that?

[Ami GONE FROM CHROMIUM, 2011-09-27 03:22:44]

BTW CL description needs BUG/TEST lines.  Maybe there's a bug that explains the
answers to my motivational questions?

[wjia(left Chromium), 2011-09-27 17:12:54]

Thanks, Ami! Reply is inline.

On Mon, Sep 26, 2011 at 8:21 PM, <fischman@chromium.org> wrote:

> I see no problems with the code that's here, but I'm confused.
> Is the expectation that |handler| deletes itself when its OnRemoved()
> method is
> called?
>
OnRemoved() gives a signal to |handler| that it won't be used or called in
VideoCapture, i.e., |handler| has been removed from VideoCapture's client
list. The |handler| has the freedom to do whatever is right for it.


> I'm having a hard time figuring out who has the authority to call a method
> named
> "OnRemoved()" that doesn't alternatively have the authority to simply
> delete
> |handler|, instead.  In other words why not use ~EventHandler() instead of
> OnRemoved?
>
VideoCapture (actually VideoCaptureImpl) doesn't own |handler|. It expects
|handler| live till OnRemoved() call. Due to constraints from different
clients, |handler| can't be ref counted.


> Might be easier to understand this change if there was at least one
> not-NOTIMPLEMENTED version of OnRemoved().  Or a test.
>
Sorry about that. That's on my TODO list. Hopefully, the above explanation
clarifies it a bit.


>
> Note that NOTIMPLEMENTED is a LOG(ERROR) by default.  Do you really want
> that?

I will remove all NOTIMPLEMENTED as we finalize the code. That just serves a
reminder to me when I run video capture preview.


>
>
>
>
>
http://codereview.chromium.**org/8037055/<http://codereview.chromium.org/8037...
>

[wjia(left Chromium), 2011-09-27 17:15:53]

On Mon, Sep 26, 2011 at 8:22 PM, <fischman@chromium.org> wrote:

> BTW CL description needs BUG/TEST lines.  Maybe there's a bug that explains
> the
> answers to my motivational questions?
>
> Unfortunately, there is no bug filed for this. It's an email request from
Pepper team. Currently, Pepper camera calls StopCapture in its dtor. But
StopCapture() is designed as an asynchronous interface. Therefore |handler|
is deleted before OnStopped() is called. With OnRemoved(), Pepper camera can
know when it can delete |handler|.


>
>
http://codereview.chromium.**org/8037055/<http://codereview.chromium.org/8037...
>

[Ami GONE FROM CHROMIUM, 2011-09-27 17:44:04]

IIUC I had to deal with a similar set of constraints in
OmxVideoDecodeAccelerator, where the PPAPI side wanted to be able to call
Destroy() and have that be synchronous (so that when it returned it was safe to
delete the ppapi-side client with a guarantee that no further callbacks would be
run) but the underlying implementation (OpenMAX IL client) had no synchronous
shutdown API. 

My experience there makes me think that your OnRemoved() actually wants to be a
verb, not an OnVerbed, because it isn't a callback or the like.  If that's
right, then wouldn't the implementation of Remove() work just as well in
StopCapture()?
In other words, if you need to implement a synchronous shutdown path, does
having an asynchronous shutdown still make sense?

[wjia(left Chromium), 2011-09-27 18:02:58]

On Tue, Sep 27, 2011 at 10:44 AM, <fischman@chromium.org> wrote:

> IIUC I had to deal with a similar set of constraints in
> OmxVideoDecodeAccelerator, where the PPAPI side wanted to be able to call
> Destroy() and have that be synchronous (so that when it returned it was
> safe to
> delete the ppapi-side client with a guarantee that no further callbacks
> would be
> run) but the underlying implementation (OpenMAX IL client) had no
> synchronous
> shutdown API.
>
> My experience there makes me think that your OnRemoved() actually wants to
> be a
> verb, not an OnVerbed, because it isn't a callback or the like.  If that's
> right, then wouldn't the implementation of Remove() work just as well in
> StopCapture()?
>
As a matter of fact, OnRemoved() is a logic OR of OnError() and OnStopped().
Pepper team asked for a simpler event so that they don't have to handle
multiple events. The current VideoCaptureImpl has ensured that it won't call
|handler| after OnError() or OnStopped() is called.

In other words, if you need to implement a synchronous shutdown path, does
> having an asynchronous shutdown still make sense?

Indeed, I had another patch to make StopCapture() as synchronous shutdown (
http://codereview.chromium.org/8036005/). But the concern is that the "Wait"
is on main thread.


>
>
>
>
>
http://codereview.chromium.**org/8037055/<http://codereview.chromium.org/8037...
>

[Ami GONE FROM CHROMIUM, 2011-09-27 18:16:16]

> Indeed, I had another patch to make StopCapture() as synchronous shutdown (
> http://codereview.chromium.org/8036005/). But the concern is that the "Wait"
> is on main thread.

Yes, that would be a problem :)
But I'm not seeing how this CL is helping you solve that problem; while the
Wait() is gone, nothing is replacing it...

An example of how to reconcile the constraints you describe is to explicitly
manage the lifecycle of the object in question.
As an example, in OVDA I make an extra artificial AddRef() call during
initialization to keep the object alive until shutdown is complete, and
Release() it during the async callback when shutdown is indeed complete.  There
is extra trickery needed to make sure the thread's messageloop doesn't get
deleted out from under my object during shutdown; see AddRef(), Release(), and
HORROR in this file:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/gpu/me...

[viettrungluu, 2011-09-27 20:18:30]

On 2011/09/27 18:16:16, Ami Fischman wrote:
> > Indeed, I had another patch to make StopCapture() as synchronous shutdown (
> > http://codereview.chromium.org/8036005/). But the concern is that the "Wait"
> > is on main thread.
> 
> Yes, that would be a problem :)
> But I'm not seeing how this CL is helping you solve that problem; while the
> Wait() is gone, nothing is replacing it...
> 
> An example of how to reconcile the constraints you describe is to explicitly
> manage the lifecycle of the object in question.
> As an example, in OVDA I make an extra artificial AddRef() call during
> initialization to keep the object alive until shutdown is complete, and
> Release() it during the async callback when shutdown is indeed complete. 
There
> is extra trickery needed to make sure the thread's messageloop doesn't get
> deleted out from under my object during shutdown; see AddRef(), Release(), and
> HORROR in this file:
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/gpu/me...

The idea is that this helps in managing the lifetime of the EventHandler,
whatever that is. If it happens to be refcounted, one should do an AddRef() on
the EventHandler before calling StartCapture() and the EventHandler can do a
Release() upon receiving OnRemoved() (or, depending on where the EventHandler
needs to be destroyed, perhaps post a task to e.g. the main thread that will
call Release()).

This is essentially equivalent to refcounting the EventHandler (and having the
video capture thread take a reference to the EventHandler), without making it
refcounted (which poses other problems).

[Ami GONE FROM CHROMIUM, 2011-09-27 21:05:35]

LGTM

On 2011/09/27 20:18:30, viettrungluu wrote:
> This is essentially equivalent to refcounting the EventHandler (and having the
> video capture thread take a reference to the EventHandler), without making it
> refcounted (which poses other problems).

I remain confused as to why this smells-like-a-refcounting scheme results in an
API method named "OnRemove" but Wei tells me you (or someone else from
pepper-team?) think this is an appropriate name, which makes me think I still
don't really grok the point of this API.  Since you're the consumer here I'll
let this go and look forward to understanding the point of this when the
interface is filled in in the various clients.

[viettrungluu, 2011-09-27 21:10:02]

On 2011/09/27 21:05:35, Ami Fischman wrote:
> LGTM
> 
> On 2011/09/27 20:18:30, viettrungluu wrote:
> > This is essentially equivalent to refcounting the EventHandler (and having
the
> > video capture thread take a reference to the EventHandler), without making
it
> > refcounted (which poses other problems).
> 
> I remain confused as to why this smells-like-a-refcounting scheme results in
an
> API method named "OnRemove" but Wei tells me you (or someone else from
> pepper-team?) think this is an appropriate name, which makes me think I still
> don't really grok the point of this API.  Since you're the consumer here I'll
> let this go and look forward to understanding the point of this when the
> interface is filled in in the various clients.

I'm not attached to "OnRemoved()". I'd be okay with "EventHandlerReleased()" or
whatever.

(Note that "EventHandlerAttached()"/"...Added()"/whatever would be useless,
since it'd be racy. If *I* were to design this API, I'd have an explicit
|AddEventHandler()| call, which would be required before passing the handler to
anything else.[*])

[*] I'd also probably have it return an opaque handle, which may or may not be a
pointer to the EventHandler, to be used for all the other calls.