827844 - Page jumps around/flickers when keyboard appears or disappears

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Chris Lord [:cwiiis]]

When the page size changes due to the keyboard appearing or disappearing, there is often a flicker of blank, or of the page rendered too far down or up before it settles on the correct rendering.

Other GL-based apps appear able to have the keyboard appear without flickering, so this is likely fixable, at least on a subset of devices.

A definite fix would be to stop the keyboard from resizing the UI, but I'm not sure if it's possible to ascertain how much of the screen an input method covers when doing this, so I'm suggesting to fix it some other way. I have working patches, which I'll attach once I've verified and tidied them up.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Fix flickering on surface resize

This splits up the resize into two phases - The resize of the UI container (where it makes sure a synchronous draw happens to prevent flickering), and the resize of the surface view (where the widget resize and surface reallocation happens).

Comment 2

[Chris Lord [:cwiiis]]

I was seeing deadlock without the patch in bug #826300 - I think I fixed the cause of it, but I'm not confident it won't happen without that patch, so marking it as a dependency.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 699345 [details] [diff] [review]
Fix flickering on surface resize

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

r- for API level requirements. I'll do a more thorough review on the next iteration since I suspect this will change a lot to work around that problem. Also it would be good to get BenWa to look at it for the compositor pieces.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +622,5 @@
> +    public void sizeChanged(int width, int height) {
> +        // We need to make sure a draw happens synchronously at this point,
> +        // but resizing the surface before the SurfaceView has resized will
> +        // cause a visible jump.
> +        compositionResumeRequested(mWindowSize.width, mWindowSize.height);

Could this not be done with a renderRequested() call instead?

::: mobile/android/base/gfx/LayerView.java
@@ +41,5 @@
>   * A view rendered by the layer compositor.
>   *
>   * Note that LayerView is accessed by Robocop via reflection.
>   */
> +public class LayerView extends FrameLayout implements View.OnLayoutChangeListener {

View.OnLayoutChangeListener is only in API level 11 and up. We'll need to do something for gingerbread too.

@@ +341,5 @@
>          }
>      }
>  
> +    private void onSizeChanged(int width, int height) {
> +        if (mListener != null) {

I'm not sure I fully understand this method, can you add some comments for what the different cases mean? I would have thought that the mGLController.surfaceChanged call would go into the first block.

Comment 4

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> ::: mobile/android/base/gfx/GeckoLayerClient.java
> @@ +622,5 @@
> > +    public void sizeChanged(int width, int height) {
> > +        // We need to make sure a draw happens synchronously at this point,
> > +        // but resizing the surface before the SurfaceView has resized will
> > +        // cause a visible jump.
> > +        compositionResumeRequested(mWindowSize.width, mWindowSize.height);
> 
> Could this not be done with a renderRequested() call instead?

No, the composition needs to happen synchronously (or there may be flicker) - renderRequested doesn't have the wait/lock on composition.

> ::: mobile/android/base/gfx/LayerView.java
> @@ +41,5 @@
> >   * A view rendered by the layer compositor.
> >   *
> >   * Note that LayerView is accessed by Robocop via reflection.
> >   */
> > +public class LayerView extends FrameLayout implements View.OnLayoutChangeListener {
> 
> View.OnLayoutChangeListener is only in API level 11 and up. We'll need to do
> something for gingerbread too.

Oh damn :| I guess we could sub-class SurfaceView and override the onLayout method instead. Little more tedious, but no biggie.

> @@ +341,5 @@
> >          }
> >      }
> >  
> > +    private void onSizeChanged(int width, int height) {
> > +        if (mListener != null) {
> 
> I'm not sure I fully understand this method, can you add some comments for
> what the different cases mean? I would have thought that the
> mGLController.surfaceChanged call would go into the first block.

Yeah, sorry, this function was the victim of iteration - it really just needs to do if (!mGLController.hasValidSurface()) mGLController.surfaceChanged(width, height); at the start, then the rest of it would read a lot more simply. I'll add comments as well though.

I'll ask for BenWa's review on the next patch.

Comment 5

[Chris Lord [:cwiiis]]

Attachment: Fix flickering on surface resize v2

r?Benwa for the CompositorParent changes (remove mWidgetSize, use FrameMetrics.mCompositionBounds instead).

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 699891 [details] [diff] [review]
Fix flickering on surface resize v2

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

Seems ok to me but the synchronous stuff scares me. I already get deadlocks all the time from bug 803154.

::: mobile/android/base/gfx/LayerView.java
@@ +415,5 @@
> +     */
> +    private class LayerSurfaceView extends SurfaceView {
> +        LayerView mParent;
> +
> +        public LayerSurfaceView(Context aContext, LayerView aParent) {

You don't really need a reference to the LayerView, private non-static classes have access to their parent instance's methods. Should be able to just call surfaceChanged() directly.

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 699891 [details] [diff] [review]
Fix flickering on surface resize v2

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

Great work! If this reduces the flickering across all version this is very nice. This was a very difficult problem last time I looked pre native fennec.

r+ given some additional comment clarification testing since I've seen this behavior change between versions.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +618,5 @@
>          }
>      }
>  
>      /** Implementation of LayerView.Listener */
> +    public void sizeChanged(int width, int height) {

I'm a fan of @Override. We get bit by having the parent signature change from time to time. Sadly this isn't enforced :(

::: mobile/android/base/gfx/LayerView.java
@@ +334,5 @@
> +     *
> +     * The second phase is the SurfaceView size change. At this point, the
> +     * backing GL surface is resized and another synchronous draw is performed.
> +     * Gecko is also sent the new window size, and this will likely cause an
> +     * extra draw a few frames later, after it's re-rendered and caught up.

It's not clear how you deal with the compositing that is already happening asynchronously on the compositor and pending updates in the Gecko thread or in transaction in the compositor's queue. Also how is the draw valid when the LayerView and the SurfaceView have a different notions of the size. Can you clarify these point?

I also found that the resize behavior varied between android 2,3,4 although I dealt with the problem very differently. Has this been tested on these versions?

@@ +415,5 @@
> +     */
> +    private class LayerSurfaceView extends SurfaceView {
> +        LayerView mParent;
> +
> +        public LayerSurfaceView(Context aContext, LayerView aParent) {

You can access all of the parent. You also don't need aContext. You can get the unambiguous instance of the enclosing class using 'LayerView.this'.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 699891 [details] [diff] [review]
Fix flickering on surface resize v2

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

r+ with the onLayout comments addressed and a green try run/basic testing on older android versions.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +622,5 @@
> +    public void sizeChanged(int width, int height) {
> +        // We need to make sure a draw happens synchronously at this point,
> +        // but resizing the surface before the SurfaceView has resized will
> +        // cause a visible jump.
> +        compositionResumeRequested(mWindowSize.width, mWindowSize.height);

Just to make sure I understand this correctly: you're using mWindowSize here instead of the parameters passed in because mWindowSize reflects the size of the surface at this point (which hasn't been resized yet) whereas the parameters reflect the size of the LayerView (which has been resized), right? I think that's what your comment implies, but just double-checking.

::: mobile/android/base/gfx/LayerView.java
@@ +420,5 @@
> +            super(aContext);
> +            mParent = aParent;
> +        }
> +
> +        protected void onLayout(boolean changed, int left, int top, int right, int bottom) {

Add an @Override tag on this. Also it should be calling super.onLayout(). I'm not sure if it's a no-op or not, but it may change in the future.

Comment 9

[Chris Lord [:cwiiis]]

Confirmed green on try, patch works for an armv6 Android 2.3 phone (Samsung Galaxy Y, I think) as well as an Intel Android 4.0 phone (Motorola Razr i).

Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/934e8b48a539

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f9ef21529956

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f9ef21529956

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f9ef21529956

Comment 13

[Benoit Girard (:BenWa)]

I've seen a big improvement since this landed on GN+JB.