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

RESOLVED FIXED in Firefox 14

Status

()

Firefox for Android
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

unspecified
Firefox 15
ARM
Android
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox14 fixed, blocking-fennec1.0 beta+)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

6 years ago
Created attachment 619040 [details] [diff] [review]
Fix screenshot layer and background colour drawing
Attachment #619040 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 2

6 years ago
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.
blocking-fennec1.0: --- → ?
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.
Attachment #619040 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 4

6 years ago
(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
Target Milestone: --- → Firefox 15
blocking-fennec1.0: ? → beta+
(Assignee)

Comment 5

6 years ago
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
Attachment #619040 - Flags: approval-mozilla-aurora?
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.
Attachment #619040 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 7

6 years ago
Landed on aurora:

http://hg.mozilla.org/releases/mozilla-aurora/rev/5c321d7740dd
status-firefox14: --- → fixed
http://hg.mozilla.org/mozilla-central/rev/cf6ee1243e0d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Duplicate of this bug: 749438
You need to log in before you can comment on or make changes to this bug.