The default bug view has changed. See this FAQ.

[OGL] Duplicate urlbar flickers at bottom of awesomescreen when keyboard appears (Android + GL)

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mbrubeck, Assigned: BenWa)

Tracking

Trunk
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+)

Details

(URL)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
tracking-fennec: --- → ?

Updated

6 years ago
tracking-fennec: ? → 2.0b4+

Updated

6 years ago
Summary: Duplicate urlbar flickers at bottom of awesomescreen when keyboard appears (Android + GL) → [OGL] Duplicate urlbar flickers at bottom of awesomescreen when keyboard appears (Android + GL)

Updated

6 years ago
tracking-fennec: 2.0b4+ → 2.0next+
tracking-fennec: 2.0next+ → 6+
tracking-fennec: 6+ → 7+

Comment 1

6 years ago
I can reproduce this on a nightly build on a Nexus S, with layers acceleration enabled (and not when layers acceleration is disabled).
(Assignee)

Comment 2

6 years ago
This seems like it may be related to the underlying issue behind bug 621745.

Comment 3

6 years ago
Bug 622634, comment 4 also seems relevant.
Blocks: 622634

Updated

6 years ago
Assignee: nobody → ajuma
tracking-fennec: 7+ → +
(Assignee)

Comment 4

6 years ago
(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.
(Assignee)

Updated

6 years ago
Assignee: ajuma → bgirard
(Assignee)

Updated

6 years ago
Duplicate of this bug: 622634
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
(In reply to Benoit Girard (:BenWa) from comment #6)
Disregard this log, my event flushes were forcing this sequence.
(Assignee)

Comment 8

6 years ago
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.
(Assignee)

Comment 9

6 years ago
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);
(Assignee)

Comment 10

6 years ago
Created attachment 553866 [details] [diff] [review]
Wait until next frame for resize
(Assignee)

Updated

6 years ago
Attachment #553866 - Flags: review?(jmuizelaar)
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.
Attachment #553866 - Flags: review?(jmuizelaar) → review-
(Assignee)

Comment 12

6 years ago
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.
Attachment #553940 - Flags: review?(jmuizelaar)
(Assignee)

Updated

6 years ago
Attachment #553866 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
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.
Attachment #553940 - Flags: review?(jmuizelaar)
(Assignee)

Comment 14

6 years ago
Created attachment 554175 [details] [diff] [review]
patch v1
Attachment #554175 - Flags: review?(jmuizelaar)
(Assignee)

Comment 15

6 years ago
(In reply to Benoit Girard (:BenWa) from comment #14)
> Created attachment 554175 [details] [diff] [review]
> patch v1

line 174: before -> because
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.
Attachment #554175 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 17

6 years ago
Created attachment 554891 [details] [diff] [review]
patch v2

review comments + fix rot, carrying forward r=jmuizelaar.
Attachment #554175 - Attachment is obsolete: true
(Assignee)

Comment 18

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/ff19c78bded4
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/ff19c78bded4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.