Closed Bug 733596 Opened 8 years ago Closed 8 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 14
Tracking Status
firefox13 --- affected
blocking-fennec1.0 --- beta+

People

(Reporter: joe, Assigned: ajuma)

References

Details

(Whiteboard: maple [gfx])

Attachments

(1 file, 1 obsolete file)

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.
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → beta+
Priority: -- → P2
Duplicate of this bug: 724571
Whiteboard: maple → maple [gfx]
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.
I think this might be the case when we update from an already running Fennec (w/ runfield webpage up and running)?
OS: Mac OS X → Android
Hardware: x86 → ARM
This is a horrible jarring experience on Adreno GPU's, can this be prioritized higher?
Assignee: joe → chrislord.net
Ali, probably best if you can take a look.
Assignee: chrislord.net → ajuma
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.
Attachment #616727 - Flags: review?(bgirard)
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.
Attachment #616727 - Flags: review?(bgirard)
(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.
Attachment #616727 - Attachment is obsolete: true
Attachment #616979 - Flags: review?(bgirard)
Attachment #616979 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/27a52fed91a5
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/27a52fed91a5
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.