749630 - Single-color 'checkerboard' doesn't render correctly and mask is incorrectly sized

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Lord [:cwiiis]]

So, my last bug (bug 748718) to fix the screenshot layer overdraw was broken and accidentally masking too little - This issue was it was taking the right/bottom of the page-rect isntead of width/height. This broke, as drawing is relative to the page-rect already, so it would cut off too early as you scrolled down/right.

Also, it broke single-colour layer drawing - I'm not entirely sure this worked fully before, but I've simplified this by just glClear'ing to the right colour instead of relying on drawing a stretched texture. This should be considerably faster (though likely makes no perceptible difference).

I've done better testing this time, so I'm sure that both features work correctly. Maybe.

Comment 1

[Chris Lord [:cwiiis]]

Attachment: Fix screenshot layer and background colour drawing

Comment 2

[Chris Lord [:cwiiis]]

Nominating for blocking fennec beta - If we don't take this, we'll have performance issues (due to overdraw) and you may momentarily see a ginormous pixel or two before the first content draw.

Comment 3

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

Comment on attachment 619040 [details] [diff] [review]
Fix screenshot layer and background colour drawing

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

Nice, much cleaner now.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +819,5 @@
>  
> +  // If the Java compositor is being used, this clear will be done in
> +  // DrawWindowUnderlay. Make sure the bits used here match up with those used
> +  // in mobile/android/base/gfx/LayerRenderer.java
> +#ifndef MOZ_JAVA_COMPOSITOR

This is just an optimization, right? If DrawWindowUnderlay does a clear too it will just clobber this clear anyway. Is this call expensive enough to be worth the ifndef?

::: mobile/android/base/gfx/ScreenshotLayer.java
@@ +92,5 @@
>      }
>  
>      /** A Cairo image that simply saves a buffer of pixel data. */
>      static class ScreenshotImage extends CairoImage {
> +        public ByteBuffer mBuffer;

Make this private? I don't think it needs to be public.

Comment 4

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (:kats) from comment #3)
> Comment on attachment 619040 [details] [diff] [review]
> Fix screenshot layer and background colour drawing
> 
> Review of attachment 619040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, much cleaner now.
> 
> ::: gfx/layers/opengl/LayerManagerOGL.cpp
> @@ +819,5 @@
> >  
> > +  // If the Java compositor is being used, this clear will be done in
> > +  // DrawWindowUnderlay. Make sure the bits used here match up with those used
> > +  // in mobile/android/base/gfx/LayerRenderer.java
> > +#ifndef MOZ_JAVA_COMPOSITOR
> 
> This is just an optimization, right? If DrawWindowUnderlay does a clear too
> it will just clobber this clear anyway. Is this call expensive enough to be
> worth the ifndef?

I wouldn't trust any of the drivers to be clever about clearing and every little helps - Thought about this for a bit and decided to keep it in.

> ::: mobile/android/base/gfx/ScreenshotLayer.java
> @@ +92,5 @@
> >      }
> >  
> >      /** A Cairo image that simply saves a buffer of pixel data. */
> >      static class ScreenshotImage extends CairoImage {
> > +        public ByteBuffer mBuffer;
> 
> Make this private? I don't think it needs to be public.

Right, fixed.

Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/cf6ee1243e0d

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 619040 [details] [diff] [review]
Fix screenshot layer and background colour drawing

[Approval Request Comment]
Regression caused by (bug #): Rendering glitches during first page-load and frame-rate drops when scrolling beyond the top-left of the page.
User impact if declined: Lower performance and visible glitches when loading pages.
Testing completed (on m-c, etc.): Works fine locally, landed on inbound.
Risk to taking this patch (and alternatives if risky): Small risk I got something wrong with the screenshot layer (which may result in it not being drawn, or worse) - don't think I have though.
String changes made by this patch: None

Comment 6

[William Lachance (:wlach)]

So this patch fixes things so that the body background color is drawn, which is necessary for eideticker's checkerboard detection to work correctly. I'd say that weighs heavily in favour of this patch being landed ASAP.

Comment 7

[Chris Lord [:cwiiis]]

Landed on aurora:

http://hg.mozilla.org/releases/mozilla-aurora/rev/5c321d7740dd

Comment 8

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-central/rev/cf6ee1243e0d