Last Comment Bug 621745 - [OGL] Duplicate urlbar flickers at bottom of awesomescreen when keyboard appears (Android + GL)
: [OGL] Duplicate urlbar flickers at bottom of awesomescreen when keyboard appe...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: ---
Assigned To: Benoit Girard (:BenWa)
:
Mentors:
http://people.mozilla.com/~mbrubeck/f...
: 622634 (view as bug list)
Depends on:
Blocks: 598864 622634
  Show dependency treegraph
 
Reported: 2010-12-28 11:51 PST by Matt Brubeck (:mbrubeck)
Modified: 2011-08-23 01:40 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
screenshot (320.07 KB, image/png)
2010-12-28 11:51 PST, Matt Brubeck (:mbrubeck)
no flags Details
Experiment: Black frame (2.06 KB, patch)
2011-08-17 08:21 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
Wait until next frame for resize (4.17 KB, patch)
2011-08-17 12:14 PDT, Benoit Girard (:BenWa)
jmuizelaar: review-
Details | Diff | Review
Wait until next frame for resize (4.26 KB, patch)
2011-08-17 15:58 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
patch v1 (3.92 KB, patch)
2011-08-18 12:28 PDT, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Review
patch v2 (6.54 KB, patch)
2011-08-22 09:58 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review

Description Matt Brubeck (:mbrubeck) 2010-12-28 11:51:57 PST
Created attachment 500077 [details]
screenshot

Steps to reproduce (tested on Samsung Galaxy Tab):
1. Set layers.accelerate-all to "true" and open Fennec.
2. Tap the titlebar to open the awesomescreen.
3. Press back to hide the keyboard, and tap the urlbar to show it again.

I made a video: http://people.mozilla.com/~mbrubeck/flicker.3gp

The attached screenshot is a frame from the video showing the incorrect region.  Things also seem to jump around sometimes when the keyboard hides, but it's very quick and intermittent, and even on video I can't see exactly what happens.
Comment 1 Ali Juma [:ajuma] 2011-07-13 08:45:06 PDT
I can reproduce this on a nightly build on a Nexus S, with layers acceleration enabled (and not when layers acceleration is disabled).
Comment 2 Benoit Girard (:BenWa) 2011-07-14 07:27:38 PDT
This seems like it may be related to the underlying issue behind bug 621745.
Comment 3 Ali Juma [:ajuma] 2011-07-20 08:12:24 PDT
Bug 622634, comment 4 also seems relevant.
Comment 4 Benoit Girard (:BenWa) 2011-07-25 13:44:52 PDT
(In reply to comment #2)
> This seems like it may be related to the underlying issue behind bug 621745.

Tried the patch in bug 621745. It is not related.
Comment 5 Benoit Girard (:BenWa) 2011-08-09 08:42:16 PDT
*** Bug 622634 has been marked as a duplicate of this bug. ***
Comment 6 Benoit Girard (:BenWa) 2011-08-11 14:29:35 PDT
I've caught the problem with logging. It happens when we send a SIZE_CHANGED event when there is already a paint message on the event queue:

W/GeckoAppJava(19733): BENLOG STARTING SURFACE CHANGED!    <-- surfaceChanged
I/GeckoAppJava(19733): surfaceChanged: fmt: 4 dim: 480 762
W/GeckoAppJava(19733): BENLOG ENDING SURFACE CHANGED!      <-- Flush event queue
E/GeckoAppJava(19733): Begin draw with 480, 762
D/MBM_KEIF(14305): if stats done
E/GeckoAppJava(19733): BENLOG begin drawing                <-- Process event: Paint
I/BenDebug(19733): Draw with 480, 452
I/BenDebug(19733): Draw bounds 480, 452
I/BenDebug(19733): Draw with 480, 452                      <-- Wrong width since we haven't processed SIZE_CHANGED yet
I/BenDebug(19733): Draw bounds 480, 452
I/BenDebug(19733): no resize
I/BenDebug(19733): region 0, 452
I/BenDebug(19733): y offset 0
E/GeckoAppJava(19733): BENLOG end drawing
I/BenDebug(19733): SIZE CHANGED                            <-- Process event: SIZE_CHANGED
W/GeckoAppJava(19733): BENLOG ENDING SURFACE CHANGED sync! <-- surfaceChange return

I think our best bet is to make sure that we update the dimensions of GeckoSurfaceView at the same time the event is process.
Comment 7 Benoit Girard (:BenWa) 2011-08-17 08:16:35 PDT
(In reply to Benoit Girard (:BenWa) from comment #6)
Disregard this log, my event flushes were forcing this sequence.
Comment 8 Benoit Girard (:BenWa) 2011-08-17 08:21:48 PDT
Created attachment 553778 [details] [diff] [review]
Experiment: Black frame

This patch replaces bad frames after surfaceChange event to be black frames, not a pretty fix but this confirms that we can isolate problematic frames.
Comment 9 Benoit Girard (:BenWa) 2011-08-17 08:56:12 PDT
Success: The following changes fixes the problem but we can't accept this solution because the buffer content are not guaranteed. This however tells us exactly what we need to fix.

@@ -750,29 +751,37 @@ LayerManagerOGL::Render()
   GLint width = rect.width;
   GLint height = rect.height;
 
   // We can't draw anything to something with no area
   // so just return
   if (width == 0 || height == 0)
     return;
 
+  bool resizeFrame = false;
   // If the widget size changed, we have to force a MakeCurrent
   // to make sure that GL sees the updated widget size.
   if (mWidgetSize.width != width ||
       mWidgetSize.height != height)
   {
+    resizeFrame = true;
     MakeCurrent(PR_TRUE);
 
     mWidgetSize.width = width;
     mWidgetSize.height = height;
   } else {
     MakeCurrent();
   }
 
+  if( resizeFrame ) {
+    mGLContext->SwapBuffers();
+
+    MakeCurrent();
+  }
+
   SetupBackBuffer(width, height);
   SetupPipeline(width, height, ApplyWorldTransform);
 
   // Default blend function implements "OVER"
   mGLContext->fBlendFuncSeparate(LOCAL_GL_ONE, LOCAL_GL_ONE_MINUS_SRC_ALPHA,
                                  LOCAL_GL_ONE, LOCAL_GL_ONE);
   mGLContext->fEnable(LOCAL_GL_BLEND);
Comment 10 Benoit Girard (:BenWa) 2011-08-17 12:14:45 PDT
Created attachment 553866 [details] [diff] [review]
Wait until next frame for resize
Comment 11 Jeff Muizelaar [:jrmuizel] 2011-08-17 12:31:11 PDT
Comment on attachment 553866 [details] [diff] [review]
Wait until next frame for resize

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

I don't remember how Java's synchronization primitives work. Can you give a more indepth overview of what's going on here?

::: embedding/android/GeckoSurfaceView.java
@@ +736,5 @@
>      Address  mLastGeoAddress;
>  
>      final SynchronousQueue<Object> mSyncDraws = new SynchronousQueue<Object>();
> +
> +    // Android lets use wait finish our current frame before resizing.

This sentence doesn't make sense.

@@ +737,5 @@
>  
>      final SynchronousQueue<Object> mSyncDraws = new SynchronousQueue<Object>();
> +
> +    // Android lets use wait finish our current frame before resizing.
> +    // This make it easier since we are drawing on a different thread.

s/make/makes? Also makes what easier?

@@ +738,5 @@
>      final SynchronousQueue<Object> mSyncDraws = new SynchronousQueue<Object>();
> +
> +    // Android lets use wait finish our current frame before resizing.
> +    // This make it easier since we are drawing on a different thread.
> +    // We need to post pone the resize until the next frame is drawn,

postpone is one word. This should so why we need to postpone until the next frame.
Comment 12 Benoit Girard (:BenWa) 2011-08-17 15:58:36 PDT
Created attachment 553940 [details] [diff] [review]
Wait until next frame for resize

Brief overnight of java sync. You can synchronize over ANY object in java and only one thread can be synchronized on any given object instance at once. When a thread is synchronized over an object and its not in the state it wants it can 'wait()' to be notify of a change to that object (the thread releases the synchronization lock on that object while waiting). Likewise a thread that is synchronized on a object can 'notify()' a thread waiting (if any are) on it that the object has changed. The notified thread will wake up and require the lock once the notifying thread has woken up.

So what I'm doing with synchronization is making the thread sleep until we have painted the next frame (called swapBuffers).

Cause of the bug:
When we receive a surfaceChanged event the current back buffer doesn't change in dimensions, it is only after the next swap that the surface actually changes. This waits until the next call to endDrawing is made so we know that exactly one swapBuffer has occurred then we process the surfaceChanged event.

Fix for hang reported by ajuma:
Called GeckoAppShell.scheduleRedraw when there is no drawing operation in progress to prevent us from sleeping forever.
Comment 13 Benoit Girard (:BenWa) 2011-08-18 07:39:16 PDT
Comment on attachment 553940 [details] [diff] [review]
Wait until next frame for resize

Lets' hold up for now. I was thinking about this last night and I might have a better idea.
Comment 14 Benoit Girard (:BenWa) 2011-08-18 12:28:58 PDT
Created attachment 554175 [details] [diff] [review]
patch v1
Comment 15 Benoit Girard (:BenWa) 2011-08-18 12:30:09 PDT
(In reply to Benoit Girard (:BenWa) from comment #14)
> Created attachment 554175 [details] [diff] [review]
> patch v1

line 174: before -> because
Comment 16 Jeff Muizelaar [:jrmuizel] 2011-08-19 15:24:38 PDT
Comment on attachment 554175 [details] [diff] [review]
patch v1

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

::: embedding/android/GeckoSurfaceView.java
@@ +177,5 @@
> +        }
> +        GeckoAppShell.geckoEventSync();
> +        mDrawSingleFrame = false;
> +        mAbortDraw = false;
> +

You should give an explanation of the problem we're trying to solve. Something like:
The buffer size only changes after the next swap buffer. We need to make sure we make sure Gecko's belief about the size matches Android's.

Also, you can add some explanation like this:
When you have surfacechange event, we have 0 to n paint events waiting in the Gecko event queue.

@@ +233,5 @@
>              mSurfaceLock.unlock();
> +            if (mDrawMode == DRAW_GLES_2) {
> +                GeckoAppShell.scheduleRedraw();
> +                GeckoAppShell.geckoEventSync();
> +            }

What is this part for? Check if it's needed. Resize events should cause repaints?

Add a comment about weird behaviour when not Syncing.

@@ +316,5 @@
>  
> +        if (mAbortDraw) {
> +            return DRAW_ERROR;
> +        }
> +

This should return a different error because it's intentional.

@@ +348,5 @@
>              return;
>          }
>  
> +	if (mDrawSingleFrame)
> +            mAbortDraw = true;

Add a comment like:

Once we've done our one paint we can ignore all of the other ones until we resize.
Comment 17 Benoit Girard (:BenWa) 2011-08-22 09:58:58 PDT
Created attachment 554891 [details] [diff] [review]
patch v2

review comments + fix rot, carrying forward r=jmuizelaar.
Comment 19 Mounir Lamouri (:mounir) 2011-08-23 01:39:51 PDT
http://hg.mozilla.org/mozilla-central/rev/ff19c78bded4

Note You need to log in before you can comment on or make changes to this bug.