media: Quit MojoMediaApplication when no service is bound.

media: Quit MojoMediaApplication when no service is bound.

After 30 seconds without any service bound, we'll quit the mojo media
application to release resources. In particular, when we host the app in the
utility process, this is needed so that the utility process can be killed when
no service is bound for long time.

Also changes AppRefCount to supported delayed release.

BUG=479935
TEST=Tested with the mojo media app hosted in the utility process.

[xhwang, 2015-06-23 22:31:06]

xhwang@chromium.org changed reviewers:
+ sandersd@chromium.org, sky@chromium.org

[xhwang, 2015-06-23 22:31:06]

xhwang@chromium.org changed required reviewers:
+ sandersd@chromium.org

[xhwang, 2015-06-23 22:31:07]

I am basically copying the logic in device/devices_app/devices_app.cc.

sandersd: PTAL
sky: Since rockot@ is OOO, adding you for due diligence review.

[sandersd (OOO until July 31), 2015-06-23 23:46:53]

Can we ask/file a bug for this to be implemented in the shell? It doesn't seem
like an application should be implementing this logic, and it looks like there
may be race conditions where clients will unnecessarily get connection errors.

lgtm.

[sky, 2015-06-24 03:03:32]

How about some test coverage?

Also, I don't think a general policy like this belongs in the shell. The shell
won't know all the connections you may have or what makes sense for your app.

https://codereview.chromium.org/1200323003/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_application.cc (right):

https://codereview.chromium.org/1200323003/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.cc:36: app_impl_ = app;
Add to member initializers and set to null.

https://codereview.chromium.org/1200323003/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.cc:65:
&MojoMediaApplication::OnConnectionError, base::Unretained(this)));
I'm not familiar with this code. Who owns mojo_cmd_service? And who owns
MojoMedaiApplication?

https://codereview.chromium.org/1200323003/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_application.h (right):

https://codereview.chromium.org/1200323003/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.h:55: };
DISALLOW_...

[xhwang, 2015-06-25 18:09:45]

Comments only:

On 2015/06/24 03:03:32, sky wrote:
> How about some test coverage?

I'll try to add a test for this.

> Also, I don't think a general policy like this belongs in the shell. The shell
> won't know all the connections you may have or what makes sense for your app.

What about AppLifetimeHelper [3]? It seems to do exactly what we want (without
delayed destruction though). But I don't see it used anywhere in the code
base...

[3]
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/application/p...

[xhwang, 2015-06-25 18:09:52]

https://codereview.chromium.org/1200323003/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_application.cc (right):

https://codereview.chromium.org/1200323003/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.cc:65:
&MojoMediaApplication::OnConnectionError, base::Unretained(this)));
On 2015/06/24 03:03:31, sky wrote:
> I'm not familiar with this code. Who owns mojo_cmd_service? And who owns
> MojoMedaiApplication?

mojo_cmd_service is owned by the pipe since it uses the StrongBinding.

MojoMedaiApplication is owned by the ApplicationRunner [1], which has the same
lifetime as the RunnerThread [2]

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/application/p...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/shell/static_...

[xhwang, 2015-06-25 18:15:31]

xhwang@chromium.org changed reviewers:
+ jam@chromium.org

[xhwang, 2015-06-25 18:15:32]

+jam, who added AppLifetimeHelper.

jam: Shall we try to use AppLifetimeHelper for application lifetime management?
I am currently following what the Devices App is doing but it seems like there
are some duplication of work here.

[xhwang, 2015-06-26 16:30:07]

sky: I added a unittest and addressed all comments. PTAL again.

https://codereview.chromium.org/1200323003/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_application.cc (right):

https://codereview.chromium.org/1200323003/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.cc:36: app_impl_ = app;
On 2015/06/24 03:03:31, sky wrote:
> Add to member initializers and set to null.

Added Class Member Initializers.

https://codereview.chromium.org/1200323003/diff/1/media/mojo/services/mojo_me...
File media/mojo/services/mojo_media_application.h (right):

https://codereview.chromium.org/1200323003/diff/1/media/mojo/services/mojo_me...
media/mojo/services/mojo_media_application.h:55: };
On 2015/06/24 03:03:31, sky wrote:
> DISALLOW_...

Done.

[sky, 2015-06-29 14:49:50]

On 2015/06/25 18:15:32, xhwang wrote:
> +jam, who added AppLifetimeHelper.
> 
> jam: Shall we try to use AppLifetimeHelper for application lifetime
management?
> I am currently following what the Devices App is doing but it seems like there
> are some duplication of work here.

I think you should. Is there a reason not to?

[xhwang, 2015-06-29 16:38:33]

On 2015/06/29 14:49:50, sky wrote:
> On 2015/06/25 18:15:32, xhwang wrote:
> > +jam, who added AppLifetimeHelper.
> > 
> > jam: Shall we try to use AppLifetimeHelper for application lifetime
> management?
> > I am currently following what the Devices App is doing but it seems like
there
> > are some duplication of work here.
> 
> I think you should. Is there a reason not to?

AppLifetimeHelper is not used anywhere. I wonder whether it's ready. Also, it
doesn't provide delayed destruction.

+jam: kindly ping for comments.

[xhwang, 2015-06-30 07:53:56]

sky/jam: I guess I didn't really know how AddRefCount worked. Now I read it
again and it's actually pretty easy to use. Sorry for the noise/confusion.

I updated my CL to use AddRefCount. Also I changed AddRefCount a bit to support
delayed app destruction. PTAL!

sandersd: This CL has been dramatically changed. PTAL again!

[sandersd (OOO until July 31), 2015-06-30 21:32:48]

Still lgtm, and we're getting close to an overall satisfactory solution. Thanks!

https://codereview.chromium.org/1200323003/diff/80001/media/mojo/services/moj...
File media/mojo/services/mojo_media_application.cc (right):

https://codereview.chromium.org/1200323003/diff/80001/media/mojo/services/moj...
media/mojo/services/mojo_media_application.cc:20: const char
kFastAppDestruction[] = "fast-app-destruction";
Perhaps too generic a name for a media-specific flag. Some options:
    --fast-media-app-destruction
    --media-app-idle-timeout=0
    --disable-media-app-idle

They're all bad, because the default app behavior ("no idle") is to always run,
so every choice is confusing.

https://codereview.chromium.org/1200323003/diff/80001/media/mojo/services/moj...
media/mojo/services/mojo_media_application.cc:49: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(kFastAppDestruction))
It's a little strange to have this in Initialize(). Is it not safe to put in the
constructor? (Or option B: move it to a const or even static method.)

https://codereview.chromium.org/1200323003/diff/80001/mojo/application/public...
File mojo/application/public/cpp/application_impl.h (right):

https://codereview.chromium.org/1200323003/diff/80001/mojo/application/public...
mojo/application/public/cpp/application_impl.h:87: const base::Closure&
error_cb);
It's true that this is caused by a connection error, but it doesn't seem to be
an error state. |terminate_cb| instead?

[xhwang, 2015-07-07 20:11:53]

rebase only

[xhwang, 2015-07-07 20:29:10]

comments addressed

[xhwang, 2015-07-07 20:29:24]

https://codereview.chromium.org/1200323003/diff/80001/media/mojo/services/moj...
File media/mojo/services/mojo_media_application.cc (right):

https://codereview.chromium.org/1200323003/diff/80001/media/mojo/services/moj...
media/mojo/services/mojo_media_application.cc:20: const char
kFastAppDestruction[] = "fast-app-destruction";
On 2015/06/30 21:32:47, sandersd wrote:
> Perhaps too generic a name for a media-specific flag. Some options:
>     --fast-media-app-destruction
>     --media-app-idle-timeout=0
>     --disable-media-app-idle
> 
> They're all bad, because the default app behavior ("no idle") is to always
run,
> so every choice is confusing.

Picked fast-media-app-destruction to make it more media specific. And I agree
naming is hard ;)

https://codereview.chromium.org/1200323003/diff/80001/media/mojo/services/moj...
media/mojo/services/mojo_media_application.cc:49: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(kFastAppDestruction))
On 2015/06/30 21:32:47, sandersd wrote:
> It's a little strange to have this in Initialize(). Is it not safe to put in
the
> constructor? (Or option B: move it to a const or even static method.)

No. Added comment to explain that.

https://codereview.chromium.org/1200323003/diff/80001/mojo/application/public...
File mojo/application/public/cpp/application_impl.h (right):

https://codereview.chromium.org/1200323003/diff/80001/mojo/application/public...
mojo/application/public/cpp/application_impl.h:87: const base::Closure&
error_cb);
On 2015/06/30 21:32:47, sandersd wrote:
> It's true that this is caused by a connection error, but it doesn't seem to be
> an error state. |terminate_cb| instead?

Done.

[xhwang, 2015-07-07 20:45:40]

rebase only

[xhwang, 2015-07-07 21:02:22]

jam: PTAL since sky@ is OOO.

rockot: FYI

[jam, 2015-07-08 21:15:37]

why do we need a delay in applifetime helper? i.e. are you sure this
optimization is necessary?

[xhwang, 2015-07-08 21:47:38]

On 2015/07/08 21:15:37, jam wrote:
> why do we need a delay in applifetime helper? i.e. are you sure this
> optimization is necessary?

Good question. Say we are running the mojo media application in the utility
process. So when mojo media application is destroyed, the utility process will
be terminated immediately [1].

Now assume the JS app is playing episode 2 after episode 1. The JS app first
destroys the media element for episode 1, then creates a new media element for
episode 2. When the first media element is destroyed, it will disconnect the
mojo media service. Without delayed app destruction, we'll destroyed the app
immediately, which will destroy the utility process. Then when we create the
second media element, we'll restart a utility process and reload the mojo media
app. I felt this is inefficient.

An alternative would be to delay the destruction of the utility process. But I
felt the current approach is better since it also increases the chance to reuse
the app (not just the process).

Currently we have 30s delay in destructing the PPAPI process when no PPAPI
instance is created. This change is consistent with that as well.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/utility/ut...

[Ken Rockot(use gerrit already), 2015-07-08 22:05:55]

On 2015/07/08 21:47:38, xhwang wrote:
> On 2015/07/08 21:15:37, jam wrote:
> > why do we need a delay in applifetime helper? i.e. are you sure this
> > optimization is necessary?
> 
> Good question. Say we are running the mojo media application in the utility
> process. So when mojo media application is destroyed, the utility process will
> be terminated immediately [1].
> 
> Now assume the JS app is playing episode 2 after episode 1. The JS app first
> destroys the media element for episode 1, then creates a new media element for
> episode 2. When the first media element is destroyed, it will disconnect the
> mojo media service. Without delayed app destruction, we'll destroyed the app
> immediately, which will destroy the utility process. Then when we create the
> second media element, we'll restart a utility process and reload the mojo
media
> app. I felt this is inefficient.
> 
> An alternative would be to delay the destruction of the utility process. But I
> felt the current approach is better since it also increases the chance to
reuse
> the app (not just the process).
> 
> Currently we have 30s delay in destructing the PPAPI process when no PPAPI
> instance is created. This change is consistent with that as well.
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/utility/ut...

It seems likely to me that frequent app startup/shutdown cycling will become a
common problem without some kind of timeout window like this.

Thinking about it some more though, maybe this is something that can be
addressed by ApplicationManager policy rather than each individual app? i.e. the
shell could impose a grace period between receiving QuitApplication from an app
and issuing OnQuitRequested back to the app.

[jam, 2015-07-09 16:31:48]

On 2015/07/08 22:05:55, Ken Rockot wrote:
> On 2015/07/08 21:47:38, xhwang wrote:
> > On 2015/07/08 21:15:37, jam wrote:
> > > why do we need a delay in applifetime helper? i.e. are you sure this
> > > optimization is necessary?
> > 
> > Good question. Say we are running the mojo media application in the utility
> > process. So when mojo media application is destroyed, the utility process
will
> > be terminated immediately [1].
> > 
> > Now assume the JS app is playing episode 2 after episode 1. The JS app first
> > destroys the media element for episode 1, then creates a new media element
for
> > episode 2. When the first media element is destroyed, it will disconnect the
> > mojo media service. Without delayed app destruction, we'll destroyed the app
> > immediately, which will destroy the utility process. Then when we create the
> > second media element, we'll restart a utility process and reload the mojo
> media
> > app. I felt this is inefficient.
> > 
> > An alternative would be to delay the destruction of the utility process. But
I
> > felt the current approach is better since it also increases the chance to
> reuse
> > the app (not just the process).
> > 
> > Currently we have 30s delay in destructing the PPAPI process when no PPAPI
> > instance is created. This change is consistent with that as well.
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/utility/ut...
> 
> It seems likely to me that frequent app startup/shutdown cycling will become a
> common problem without some kind of timeout window like this.
> 
> Thinking about it some more though, maybe this is something that can be
> addressed by ApplicationManager policy rather than each individual app? i.e.
the
> shell could impose a grace period between receiving QuitApplication from an
app
> and issuing OnQuitRequested back to the app.


Yep I'm familiar with the NPAPI (and later PPAPI) case since I wrote that after
we noticed performance problems. Here I'm looking for data that we need extra
complexity before adding it. The plugin case was different because Flash was
doing a lot of startup initialization. It's not clear to me that our apps will
be slow to load (and if so, we need to make them load faster).

[xhwang, 2015-07-09 17:07:23]

On 2015/07/09 16:31:48, jam wrote:
> On 2015/07/08 22:05:55, Ken Rockot wrote:
> > On 2015/07/08 21:47:38, xhwang wrote:
> > > On 2015/07/08 21:15:37, jam wrote:
> > > > why do we need a delay in applifetime helper? i.e. are you sure this
> > > > optimization is necessary?
> > > 
> > > Good question. Say we are running the mojo media application in the
utility
> > > process. So when mojo media application is destroyed, the utility process
> will
> > > be terminated immediately [1].
> > > 
> > > Now assume the JS app is playing episode 2 after episode 1. The JS app
first
> > > destroys the media element for episode 1, then creates a new media element
> for
> > > episode 2. When the first media element is destroyed, it will disconnect
the
> > > mojo media service. Without delayed app destruction, we'll destroyed the
app
> > > immediately, which will destroy the utility process. Then when we create
the
> > > second media element, we'll restart a utility process and reload the mojo
> > media
> > > app. I felt this is inefficient.
> > > 
> > > An alternative would be to delay the destruction of the utility process.
But
> I
> > > felt the current approach is better since it also increases the chance to
> > reuse
> > > the app (not just the process).
> > > 
> > > Currently we have 30s delay in destructing the PPAPI process when no PPAPI
> > > instance is created. This change is consistent with that as well.
> > > 
> > > [1]
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/utility/ut...
> > 
> > It seems likely to me that frequent app startup/shutdown cycling will become
a
> > common problem without some kind of timeout window like this.
> > 
> > Thinking about it some more though, maybe this is something that can be
> > addressed by ApplicationManager policy rather than each individual app? i.e.
> the
> > shell could impose a grace period between receiving QuitApplication from an
> app
> > and issuing OnQuitRequested back to the app.
> 
> 
> Yep I'm familiar with the NPAPI (and later PPAPI) case since I wrote that
after
> we noticed performance problems. Here I'm looking for data that we need extra
> complexity before adding it. The plugin case was different because Flash was
> doing a lot of startup initialization. It's not clear to me that our apps will
> be slow to load (and if so, we need to make them load faster).

Thanks for the background info. 

Currently the media app is mostly a shell, it relies on platform specific media
renderers (including decoders) and CDMs to do the real work. So the load time
will mostly depend on the those platform specific implementations. I don't have
any real data yet, but I should be able to collect data for the desktop case
where we need to load the Widevine CDM. But note that the media app can be used
on different platforms where the platform specific implementations could be very
different. It's hard to make assumptions on the performance for all of them.

That being said, is the overhead of starting a new process a concern at all?

[jam, 2015-07-10 16:30:54]

On 2015/07/09 17:07:23, xhwang wrote:
> On 2015/07/09 16:31:48, jam wrote:
> > On 2015/07/08 22:05:55, Ken Rockot wrote:
> > > On 2015/07/08 21:47:38, xhwang wrote:
> > > > On 2015/07/08 21:15:37, jam wrote:
> > > > > why do we need a delay in applifetime helper? i.e. are you sure this
> > > > > optimization is necessary?
> > > > 
> > > > Good question. Say we are running the mojo media application in the
> utility
> > > > process. So when mojo media application is destroyed, the utility
process
> > will
> > > > be terminated immediately [1].
> > > > 
> > > > Now assume the JS app is playing episode 2 after episode 1. The JS app
> first
> > > > destroys the media element for episode 1, then creates a new media
element
> > for
> > > > episode 2. When the first media element is destroyed, it will disconnect
> the
> > > > mojo media service. Without delayed app destruction, we'll destroyed the
> app
> > > > immediately, which will destroy the utility process. Then when we create
> the
> > > > second media element, we'll restart a utility process and reload the
mojo
> > > media
> > > > app. I felt this is inefficient.
> > > > 
> > > > An alternative would be to delay the destruction of the utility process.
> But
> > I
> > > > felt the current approach is better since it also increases the chance
to
> > > reuse
> > > > the app (not just the process).
> > > > 
> > > > Currently we have 30s delay in destructing the PPAPI process when no
PPAPI
> > > > instance is created. This change is consistent with that as well.
> > > > 
> > > > [1]
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/utility/ut...
> > > 
> > > It seems likely to me that frequent app startup/shutdown cycling will
become
> a
> > > common problem without some kind of timeout window like this.
> > > 
> > > Thinking about it some more though, maybe this is something that can be
> > > addressed by ApplicationManager policy rather than each individual app?
i.e.
> > the
> > > shell could impose a grace period between receiving QuitApplication from
an
> > app
> > > and issuing OnQuitRequested back to the app.
> > 
> > 
> > Yep I'm familiar with the NPAPI (and later PPAPI) case since I wrote that
> after
> > we noticed performance problems. Here I'm looking for data that we need
extra
> > complexity before adding it. The plugin case was different because Flash was
> > doing a lot of startup initialization. It's not clear to me that our apps
will
> > be slow to load (and if so, we need to make them load faster).
> 
> Thanks for the background info. 
> 
> Currently the media app is mostly a shell, it relies on platform specific
media
> renderers (including decoders) and CDMs to do the real work. So the load time
> will mostly depend on the those platform specific implementations. I don't
have
> any real data yet, but I should be able to collect data for the desktop case
> where we need to load the Widevine CDM. But note that the media app can be
used
> on different platforms where the platform specific implementations could be
very
> different. It's hard to make assumptions on the performance for all of them.
> 
> That being said, is the overhead of starting a new process a concern at all?

having some numbers to know the magnitude of the overhead is needed to reason
about this..