[Android] Fix a flicker in stopping Chrome

[Android] Fix a flicker in stopping Chrome

Currently, we draw a blank frame as we release resources in onStop().
This can be prevented if we hide the tab when the window gets hidden instead of
on activity stop.

BUG=434401
Committed: https://crrev.com/e4b1990c9d49fd9734940fa863513ad43b82c6e7
Cr-Commit-Position: refs/heads/master@{#311809}

[Changwan Ryu, 2015-01-09 00:33:09]

changwan@chromium.org changed reviewers:
+ dtrainor@chromium.org, tedchoc@chromium.org, yusufo@chromium.org

[David Trainor- moved to gerrit, 2015-01-09 18:07:52]

A few questions.  I'm not quite sure what the answer should be but I wanted to
ask them before approving.

https://chromiumcodereview.appspot.com/843883002/diff/20001/chrome/android/ja...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://chromiumcodereview.appspot.com/843883002/diff/20001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1092: }, 500);
Does this only happen on L and above?  Should we only do it on those devices? 
Hmm I wonder if there's a callback for when we actually finish the animation so
we know the exact time to hide.

[Ted C, 2015-01-09 18:10:40]

On 2015/01/09 18:07:52, David Trainor wrote:
> A few questions.  I'm not quite sure what the answer should be but I wanted to
> ask them before approving.
> 
>
https://chromiumcodereview.appspot.com/843883002/diff/20001/chrome/android/ja...
> File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):
> 
>
https://chromiumcodereview.appspot.com/843883002/diff/20001/chrome/android/ja...
> chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1092: }, 500);
> Does this only happen on L and above?  Should we only do it on those devices? 
> Hmm I wonder if there's a callback for when we actually finish the animation
so
> we know the exact time to hide.

I was wondering the same.  I'm curious if window focus happens before or after
stop.  I also wonder if we could add a draw listener and use a combination of
X number of draw calls after stop w/ a combination of a timeout to handle this
case.

[Changwan Ryu, 2015-01-12 03:31:53]

On 2015/01/09 18:10:40, Ted C wrote:
> On 2015/01/09 18:07:52, David Trainor wrote:
> > A few questions.  I'm not quite sure what the answer should be but I wanted
to
> > ask them before approving.
> > 
> >
>
https://chromiumcodereview.appspot.com/843883002/diff/20001/chrome/android/ja...
> > File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/843883002/diff/20001/chrome/android/ja...
> > chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1092: }, 500);
> > Does this only happen on L and above?  Should we only do it on those
devices? 
> > Hmm I wonder if there's a callback for when we actually finish the animation
> so
> > we know the exact time to hide.
> 
> I was wondering the same.  I'm curious if window focus happens before or after
> stop.  I also wonder if we could add a draw listener and use a combination of
> X number of draw calls after stop w/ a combination of a timeout to handle this
> case.

Thanks for the feedback. While seeking answers for these questions, I realized
that I need to
pause the compositor in stopping the activity. It seems that Android allows
drawing extra frames even after Activity#onStop() has been finished (probably
only in L). Hope this saves a bit more of battery as well. Please take another
look.

[Changwan Ryu, 2015-01-12 03:54:59]

changwan@chromium.org changed reviewers:
+ aelias@chromium.org

[David Trainor- moved to gerrit, 2015-01-12 18:58:24]

dtrainor@chromium.org changed reviewers:
+ sievers@chromium.org

[David Trainor- moved to gerrit, 2015-01-12 18:58:24]

https://chromiumcodereview.appspot.com/843883002/diff/80001/content/browser/r...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://chromiumcodereview.appspot.com/843883002/diff/80001/content/browser/r...
content/browser/renderer_host/compositor_impl_android.cc:266: if (paused_)
Does bypassing this cause side effects?  I know some render processes wait for
the render ack and I don't know if simply skipping the Composite call would
cause issues.  CC'ing sievers@ who would know more about the scheduling here.

[no sievers, 2015-01-12 19:56:49]

https://codereview.chromium.org/843883002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/843883002/diff/80001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:266: if (paused_)
TLDR: We should either unify our code more with Aura and adopt the mechanism of
compositing locks for inhibiting compositing during intermittent transitions
(like resize) if that applies here. Otherwise (anything that's not
intermittent), we should keep the tree in a state that it generates
non-corrupt-looking or flickering content (keep in mind that
CopyFromCompositingSurface is also supposed to eventually lead to a composite
and produce a good frame) *while the compositor surface is visible/available*.
Let's be especially careful what other notions we expose especially to the
embedder (ideally none other than visibility/surface availability).

Exposing the notion of paused makes things more complicated, and it will break
with https://codereview.chromium.org/344743002/ since cc does not have such a
notion. Actually the latter is not entirely true: It's true that cc does not
have the notion but Aura has some sort of compositing locks for short
transitions during resize where the result from compositing would not look good
(wrong aspect ratio or so if scaled). But it should really only be used in
transitions. Is your use case like that?

Otherwise, some general thoughts:
A surface (to which the compositor draws) is either visible or not.
While it's visible you need to be able to generate its contents (because you
also have a readback API that is not expected to generate corrupted content,
i.e. it doesn't actually read back the surface but generates contents from the
layers).

You can certainly hide the surface in different ways (obscure or remove it at
the view hierarchy level, and then you can also set the compositor to hidden or
remove the surface).
While it's onscreen and visible you should have layers in the tree that do not
cause corruption (you can use actual contents, i.e. delegated layer from the
renderer, or some sort of placeholder).

[no sievers, 2015-01-12 20:05:38]

> Currently, we draw a blank frame as we release resources in onStop(), and by
introducing pause / resume of compositor we can prevent this.


Why don't we keep resources (which means at least a placeholder layer) until the
surface goes away?

[Changwan Ryu, 2015-01-13 02:20:42]

On 2015/01/12 20:05:38, sievers wrote:
> > Currently, we draw a blank frame as we release resources in onStop(), and by
> introducing pause / resume of compositor we can prevent this.
> 
> 
> Why don't we keep resources (which means at least a placeholder layer) until
the
> surface goes away?

This actually sounds like a good idea. Let me try it and get back to you soon.

[Changwan Ryu, 2015-01-13 02:24:55]

https://codereview.chromium.org/843883002/diff/80001/content/browser/renderer...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/843883002/diff/80001/content/browser/renderer...
content/browser/renderer_host/compositor_impl_android.cc:266: if (paused_)
On 2015/01/12 19:56:49, sievers wrote:
> TLDR: We should either unify our code more with Aura and adopt the mechanism
of
> compositing locks for inhibiting compositing during intermittent transitions
> (like resize) if that applies here. Otherwise (anything that's not
> intermittent), we should keep the tree in a state that it generates
> non-corrupt-looking or flickering content (keep in mind that
> CopyFromCompositingSurface is also supposed to eventually lead to a composite
> and produce a good frame) *while the compositor surface is visible/available*.
> Let's be especially careful what other notions we expose especially to the
> embedder (ideally none other than visibility/surface availability).
> 
> Exposing the notion of paused makes things more complicated, and it will break
> with https://codereview.chromium.org/344743002/ since cc does not have such a
> notion. Actually the latter is not entirely true: It's true that cc does not
> have the notion but Aura has some sort of compositing locks for short
> transitions during resize where the result from compositing would not look
good
> (wrong aspect ratio or so if scaled). But it should really only be used in
> transitions. Is your use case like that?
> 
> Otherwise, some general thoughts:
> A surface (to which the compositor draws) is either visible or not.
> While it's visible you need to be able to generate its contents (because you
> also have a readback API that is not expected to generate corrupted content,
> i.e. it doesn't actually read back the surface but generates contents from the
> layers).
> 
> You can certainly hide the surface in different ways (obscure or remove it at
> the view hierarchy level, and then you can also set the compositor to hidden
or
> remove the surface).
> While it's onscreen and visible you should have layers in the tree that do not
> cause corruption (you can use actual contents, i.e. delegated layer from the
> renderer, or some sort of placeholder).
> 

Thanks for pointing out the conflict, and CopyFromCompositingSurface case. I am
not sure how I can apply resizelock to generic compositing lock without
corruption handling. Let me work on the other idea first, and come back here if
that fails.

[Changwan Ryu, 2015-01-13 09:37:23]

On 2015/01/13 02:24:55, Changwan Ryu wrote:
>
https://codereview.chromium.org/843883002/diff/80001/content/browser/renderer...
> File content/browser/renderer_host/compositor_impl_android.cc (right):
> 
>
https://codereview.chromium.org/843883002/diff/80001/content/browser/renderer...
> content/browser/renderer_host/compositor_impl_android.cc:266: if (paused_)
> On 2015/01/12 19:56:49, sievers wrote:
> > TLDR: We should either unify our code more with Aura and adopt the mechanism
> of
> > compositing locks for inhibiting compositing during intermittent transitions
> > (like resize) if that applies here. Otherwise (anything that's not
> > intermittent), we should keep the tree in a state that it generates
> > non-corrupt-looking or flickering content (keep in mind that
> > CopyFromCompositingSurface is also supposed to eventually lead to a
composite
> > and produce a good frame) *while the compositor surface is
visible/available*.
> > Let's be especially careful what other notions we expose especially to the
> > embedder (ideally none other than visibility/surface availability).
> > 
> > Exposing the notion of paused makes things more complicated, and it will
break
> > with https://codereview.chromium.org/344743002/ since cc does not have such
a
> > notion. Actually the latter is not entirely true: It's true that cc does not
> > have the notion but Aura has some sort of compositing locks for short
> > transitions during resize where the result from compositing would not look
> good
> > (wrong aspect ratio or so if scaled). But it should really only be used in
> > transitions. Is your use case like that?
> > 
> > Otherwise, some general thoughts:
> > A surface (to which the compositor draws) is either visible or not.
> > While it's visible you need to be able to generate its contents (because you
> > also have a readback API that is not expected to generate corrupted content,
> > i.e. it doesn't actually read back the surface but generates contents from
the
> > layers).
> > 
> > You can certainly hide the surface in different ways (obscure or remove it
at
> > the view hierarchy level, and then you can also set the compositor to hidden
> or
> > remove the surface).
> > While it's onscreen and visible you should have layers in the tree that do
not
> > cause corruption (you can use actual contents, i.e. delegated layer from the
> > renderer, or some sort of placeholder).
> > 
> 
> Thanks for pointing out the conflict, and CopyFromCompositingSurface case. I
am
> not sure how I can apply resizelock to generic compositing lock without
> corruption handling. Let me work on the other idea first, and come back here
if
> that fails.

Now it hides on surface destruction instead of doing it on activity stop.
Please take another look.

[Ted C, 2015-01-14 00:35:54]

https://codereview.chromium.org/843883002/diff/120001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/843883002/diff/120001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:89:
public void surfaceDestroyed(SurfaceHolder holder) {
after talking with Grace, I think the correct call that should be hooked into
is:

View#onWindowVisibilityChanged

http://developer.android.com/reference/android/view/View.html#onWindowVisibil...

And if we hook into that, we should rename Tab#onActiivtyStop to be something
that reflects this new behavior and switch to using that instead of calling
.hide directly.

[Changwan Ryu, 2015-01-14 03:01:13]

https://codereview.chromium.org/843883002/diff/120001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://codereview.chromium.org/843883002/diff/120001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:89:
public void surfaceDestroyed(SurfaceHolder holder) {
On 2015/01/14 00:35:54, Ted C wrote:
> after talking with Grace, I think the correct call that should be hooked into
> is:
> 
> View#onWindowVisibilityChanged
> 
>
http://developer.android.com/reference/android/view/View.html#onWindowVisibil...

Makes sense. Done.

> 
> And if we hook into that, we should rename Tab#onActiivtyStop to be something
> that reflects this new behavior and switch to using that instead of calling
> .hide directly.

Done.

[David Trainor- moved to gerrit, 2015-01-15 01:16:07]

https://chromiumcodereview.appspot.com/843883002/diff/140001/chrome/android/s...
File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java
(right):

https://chromiumcodereview.appspot.com/843883002/diff/140001/chrome/android/s...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:100:
protected void onWindowHidden() {
Can we just override onWindowVisibilityChanged here instead and do this?  Then
we don't have to store the mWindowShown variable and we can just call hide on
GONE.  I don't think two subsequent calls to hide will do anything.

https://chromiumcodereview.appspot.com/843883002/diff/140001/content/public/a...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://chromiumcodereview.appspot.com/843883002/diff/140001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:160:
boolean mWindowShown;
private, move to top of file with other variables.

https://chromiumcodereview.appspot.com/843883002/diff/140001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:164:
boolean windowShown = visibility == View.VISIBLE;
See earlier comment in TabManager.

https://chromiumcodereview.appspot.com/843883002/diff/140001/content/shell/an...
File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
(right):

https://chromiumcodereview.appspot.com/843883002/diff/140001/content/shell/an...
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:83:
protected void onWindowHidden() {
We should just use the other method IMO.  Lets keep the visible API boundary as
small as possible since we want to eventually roll all these cases into a better
place (maybe Tab#getView()?).

https://chromiumcodereview.appspot.com/843883002/diff/140001/content/shell/an...
File
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java
(right):

https://chromiumcodereview.appspot.com/843883002/diff/140001/content/shell/an...
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java:176:
protected void onStop() {
Delete!

[Changwan Ryu, 2015-01-15 01:51:34]

https://chromiumcodereview.appspot.com/843883002/diff/140001/chrome/android/s...
File chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java
(right):

https://chromiumcodereview.appspot.com/843883002/diff/140001/chrome/android/s...
chrome/android/shell/java/src/org/chromium/chrome/shell/TabManager.java:100:
protected void onWindowHidden() {
On 2015/01/15 01:16:07, David Trainor wrote:
> Can we just override onWindowVisibilityChanged here instead and do this?  Then
> we don't have to store the mWindowShown variable and we can just call hide on
> GONE.  I don't think two subsequent calls to hide will do anything.

Done.

https://chromiumcodereview.appspot.com/843883002/diff/140001/content/public/a...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java
(right):

https://chromiumcodereview.appspot.com/843883002/diff/140001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:160:
boolean mWindowShown;
On 2015/01/15 01:16:07, David Trainor wrote:
> private, move to top of file with other variables.

Removed.

https://chromiumcodereview.appspot.com/843883002/diff/140001/content/public/a...
content/public/android/java/src/org/chromium/content/browser/ContentViewRenderView.java:164:
boolean windowShown = visibility == View.VISIBLE;
On 2015/01/15 01:16:07, David Trainor wrote:
> See earlier comment in TabManager.

Done.

https://chromiumcodereview.appspot.com/843883002/diff/140001/content/shell/an...
File content/shell/android/java/src/org/chromium/content_shell/ShellManager.java
(right):

https://chromiumcodereview.appspot.com/843883002/diff/140001/content/shell/an...
content/shell/android/java/src/org/chromium/content_shell/ShellManager.java:83:
protected void onWindowHidden() {
On 2015/01/15 01:16:07, David Trainor wrote:
> We should just use the other method IMO.  Lets keep the visible API boundary
as
> small as possible since we want to eventually roll all these cases into a
better
> place (maybe Tab#getView()?).

Done.

https://chromiumcodereview.appspot.com/843883002/diff/140001/content/shell/an...
File
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java
(right):

https://chromiumcodereview.appspot.com/843883002/diff/140001/content/shell/an...
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java:176:
protected void onStop() {
On 2015/01/15 01:16:07, David Trainor wrote:
> Delete!

Done.

[David Trainor- moved to gerrit, 2015-01-15 23:21:07]

lgtm

https://chromiumcodereview.appspot.com/843883002/diff/160001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://chromiumcodereview.appspot.com/843883002/diff/160001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1090: public void
onWindowHidden() {
Can we just call hide directly instead?  Does Tab have to know about the concept
of a window hiding?

[no sievers, 2015-01-15 23:45:01]

lgtm

[Changwan Ryu, 2015-01-16 00:03:35]

https://chromiumcodereview.appspot.com/843883002/diff/160001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://chromiumcodereview.appspot.com/843883002/diff/160001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1090: public void
onWindowHidden() {
On 2015/01/15 23:21:07, David Trainor wrote:
> Can we just call hide directly instead?  Does Tab have to know about the
concept
> of a window hiding?

Sure. Removed.

[Changwan Ryu, 2015-01-16 00:29:56]

The CQ bit was checked by changwan@chromium.org

[commit-bot: I haz the power, 2015-01-16 00:31:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/843883002/200001

[commit-bot: I haz the power, 2015-01-16 01:35:26]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-01-16 01:37:39]

Patchset 11 (id:??) landed as
https://crrev.com/e4b1990c9d49fd9734940fa863513ad43b82c6e7
Cr-Commit-Position: refs/heads/master@{#311809}

[jdduke (slow), 2015-03-09 19:33:36]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/990183004/ by jdduke@chromium.org.

The reason for reverting is: Caused regression with lockscreen battery
consumption..