733596 - Maple: Back buffer (?) uninitialized before first paint (on Adreno devices)?

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Joe Drew (not getting mail)]

It might be Adreno-specific, but I've also seen something very similar on my Galaxy Nexus, so it might not be.

https://bug724571.bugzilla.mozilla.org/attachment.cgi?id=595049 is an example of what this looks like. It happens frequently, including on startup and when we rotate as a page is loading.

I don't think that this is related to bug 724571.

Comment 2

[Joe Drew (not getting mail)]

We just aren't clearing the entire back buffer when we resize it (just the part covered by the scissor rect). On some devices, this paints black. It seems that on Adrenos, this paints uninitialized memory.

Comment 3

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I think this might be the case when we update from an already running Fennec (w/ runfield webpage up and running)?

Comment 4

[Aaron Train [:aaronmt]]

This is a horrible jarring experience on Adreno GPU's, can this be prioritized higher?

Comment 5

[JP Rosevear [:jpr]]

Ali, probably best if you can take a look.

Comment 6

[Ali Juma [:ajuma]]

Attachment: Tell the compositor about the new surface size on surface change

The problem is that when we get a surface changed event, we're not directly passing the new width and height to the compositor; instead, the compositor's layer manager pulls the current width/height from the widget. However, the widget's information can lag behind right after a surface changed event, since the widget is notified about the new size via an event sent from Java to Gecko. When we render using the old size on Adreno, we see artifacts on the screen.

The fix is for Java to send the new width and height to the compositor as part of the scheduleResumeComposition() call it makes when a surface changed event occurs. The compositor can pass this on to its layer manager.

This means that the layer manager should no longer call into the widget to find out about the width and height (since that information is now being pushed to the layer manager whenever the size changes). As a result, this patch has the added benefit of fixing the Android part of Bug 717925.

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 616727 [details] [diff] [review]
Tell the compositor about the new surface size on surface change

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

Great work, just a few edits to make it easier to understand what this patch does.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +97,2 @@
>  {
> +  if (aSurfaceWidth >= 0) {

This doesn't feel natural, maybe this wants a different constructor or a factory method.

::: gfx/layers/opengl/LayerManagerOGL.h
@@ +436,5 @@
>    nsIntRegion mClippingRegion;
>  
>    /** Misc */
>    bool mHasBGRA;
> +  bool mIsRenderingToSurface;

Lets get a comment here explain what rendering to surface means. Maybe we should rename this to EGLSurface as it's ambigious what kind of render to surface this does.

Comment 8

[Ali Juma [:ajuma]]

Attachment: Tell the compositor about the new surface size on surface change, v2

(In reply to Benoit Girard (:BenWa) from comment #7)
> Comment on attachment 616727 [details] [diff] [review]
> Tell the compositor about the new surface size on surface change
> 
> Review of attachment 616727 [details] [diff] [review]:
> ----------------------------------------------------------------- 
> ::: gfx/layers/opengl/LayerManagerOGL.cpp
> @@ +97,2 @@
> >  {
> > +  if (aSurfaceWidth >= 0) {
> 
> This doesn't feel natural, maybe this wants a different constructor or a
> factory method.

I added another argument to the constructor so that the caller can explicitly say whether we are rendering to an EGL surface.


> ::: gfx/layers/opengl/LayerManagerOGL.h
> @@ +436,5 @@
> >    nsIntRegion mClippingRegion;
> >  
> >    /** Misc */
> >    bool mHasBGRA;
> > +  bool mIsRenderingToSurface;
> 
> Lets get a comment here explain what rendering to surface means.

Done.

> Maybe we
> should rename this to EGLSurface as it's ambigious what kind of render to
> surface this does.

Done.

Comment 9

[Ali Juma [:ajuma]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27a52fed91a5

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/27a52fed91a5

Comment 11

[Adrian Tamas (:AdrianT)]

Verified on:
Nightly 15.0a1 2012-04-03/ Aurora 14.0a2 2012-04-02
Device: HTC Desire / Motorola Droid Pro
OS: Android 2.2 / Android 2.3.4