748718 - Screenshot checkerboard layer causes permanent drop in frame-rate

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Lord [:cwiiis]]

The screenshot checkerboard layer isn't respecting the clip mask set by LayerRenderer, as it overrides SingleTileLayer's draw function.

This causes a permanent drop to < 60fps on the Galaxy Nexus due to overdraw. This could be fixed by altering the class to use SingleTileLayer's draw function, or adding the necessary code to draw the masked area.

I think it's important we fix this before beta / demoing any builds. Didn't notice this last night due to tired eyes :)

Comment 1

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

Noming, this seems pretty important. It'll be interesting to see how this affects the eideticker canvas framerate test.

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Fix SingleTileLayer.java

This fixes the drawing in SingleTileLayer (which was really, totally broken (my fault))

Comment 3

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Tidy up TileLayer.java slightly

This isn't really needed for this bug, but I ran into it at some stages during fixing it, so figure it's worth getting this in.

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Make ScreenshotLayer use SingleTileLayer's draw function

This makes the necessary changes so that ScreenshotLayer can use SingleTileLayer's drawing function and take advantage of masking so that we don't have unnecessary overdraw.

Comment 5

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

Comment on attachment 618647 [details] [diff] [review]
Part 1 - Fix SingleTileLayer.java

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

A few minor things. r- because I don't understand why you do the divide/multiply by the same thing but if you can address that I'll r+ it.

::: mobile/android/base/gfx/SingleTileLayer.java
@@ +89,3 @@
>          RectF viewport = context.viewport;
> +        RectF bounds = getBounds(context);
> +        RectF textureBounds = bounds;

I think I would prefer leaving these null here and assigning them in a separate "else" clause of the condition below. Right now they get set and clobbered in both branches of the condition which seems wasteful.

@@ +95,5 @@
> +            // the texture repeats the correct number of times when drawn at
> +            // the size of the viewport.
> +            textureBounds = new RectF(0.0f, 0.0f,
> +                                      (bounds.width() / viewport.width()) * viewport.width(),
> +                                      (bounds.height() / viewport.height()) * viewport.height());

Why divide and multiply by the same thing? This seems wrong.

@@ +130,2 @@
>                                           Math.round(subRectF.right - bounds.left),
> +                                         Math.round(bounds.bottom - subRectF.bottom) };

Add a comment above this saying that it is left/top/right/bottom relative to the bottom-left of the bounds (if I'm understanding this correctly)

@@ +132,4 @@
>  
>              float height = subRectF.height();
>              float left = subRectF.left - viewport.left;
>              float top = viewport.height() - (subRectF.top + height - viewport.top);

Can you change this to:
  float top = viewport.bottom - subRectF.bottom;

I think it's equivalent and easier to read.

@@ +147,3 @@
>  
>                  (left+subRectF.width())/viewport.width(), top/viewport.height(), 0,
> +                cropRect[2]/textureBounds.width(), cropRect[3]/textureBounds.height()

Might be a bit easier to read if you create local variables for "right" and "bottom" and use those instead of "top+height" and "left+subRectF.width()"

Comment 6

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

Comment on attachment 618648 [details] [diff] [review]
Part 2 - Tidy up TileLayer.java slightly

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

::: mobile/android/base/gfx/TileLayer.java
@@ +183,5 @@
> +                                mSize.height, 0, glInfo.format, glInfo.type, imageBuffer);
> +        } else {
> +            // Our texture has been expanded to the next power of two.
> +            // XXX We probably never want to take this path, so throw an exception.
> +            throw new RuntimeException("Buffer/image size mismatch in TileLayer!");

Until bug 748531 is fixed this is pretty risky, because the exception will propagate into some JNI wrapper and disappear. This will lead to hard-to-find errors if it ever enters this code path. Maybe for now doing a Log.e is good enough?

Comment 7

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

Comment on attachment 618649 [details] [diff] [review]
Part 3 - Make ScreenshotLayer use SingleTileLayer's draw function

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

A few nits, nothing major.

::: mobile/android/base/GeckoAppShell.java
@@ +2223,1 @@
>          // source width and height to screenshot

Remove log? Or make it Log.v. It seems like this will get called a fair amount.

::: mobile/android/base/gfx/LayerRenderer.java
@@ +190,1 @@
>          try {

Move the reset call into the try block

@@ +609,5 @@
> +                    rootMask.right = mPageRect.right + 1;
> +                }
> +                if (rootMask.bottom >= mPageRect.bottom - 2) {
> +                    rootMask.bottom = mPageRect.bottom + 1;
> +                }

This entire block seems like a heuristic. I'd prefer if it were moved into a separate function, so that this code ends up looking like
  rootMask = calculateBestRootMask(rootLayer.getBounds(mPageContext), mPageRect);

where "calculateBestRootMask" should be named something more appropriate.

::: widget/android/AndroidBridge.h
@@ +184,5 @@
>      static void RemovePluginView(void* surface);
>  
> +    /* These are defined in mobile/android/base/GeckoAppShell.java */
> +    enum {
> +        SCREENSHOT_THUMBNAIL = 0,

Add a corresponding comment in GeckoAppShell saying that these should be kept in sync.

Comment 8

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (:kats) from comment #5)
> Comment on attachment 618647 [details] [diff] [review]
> Part 1 - Fix SingleTileLayer.java
> 
> Review of attachment 618647 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few minor things. r- because I don't understand why you do the
> divide/multiply by the same thing but if you can address that I'll r+ it.
> 
> ::: mobile/android/base/gfx/SingleTileLayer.java
> @@ +89,3 @@
> >          RectF viewport = context.viewport;
> > +        RectF bounds = getBounds(context);
> > +        RectF textureBounds = bounds;
> 
> I think I would prefer leaving these null here and assigning them in a
> separate "else" clause of the condition below. Right now they get set and
> clobbered in both branches of the condition which seems wasteful.

Done

> @@ +95,5 @@
> > +            // the texture repeats the correct number of times when drawn at
> > +            // the size of the viewport.
> > +            textureBounds = new RectF(0.0f, 0.0f,
> > +                                      (bounds.width() / viewport.width()) * viewport.width(),
> > +                                      (bounds.height() / viewport.height()) * viewport.height());
> 
> Why divide and multiply by the same thing? This seems wrong.

I just didn't notice the simplification... Whoops :)

> @@ +130,2 @@
> >                                           Math.round(subRectF.right - bounds.left),
> > +                                         Math.round(bounds.bottom - subRectF.bottom) };
> 
> Add a comment above this saying that it is left/top/right/bottom relative to
> the bottom-left of the bounds (if I'm understanding this correctly)

You are, done.

> @@ +132,4 @@
> >  
> >              float height = subRectF.height();
> >              float left = subRectF.left - viewport.left;
> >              float top = viewport.height() - (subRectF.top + height - viewport.top);
> 
> Can you change this to:
>   float top = viewport.bottom - subRectF.bottom;
> 
> I think it's equivalent and easier to read.

Done.

> @@ +147,3 @@
> >  
> >                  (left+subRectF.width())/viewport.width(), top/viewport.height(), 0,
> > +                cropRect[2]/textureBounds.width(), cropRect[3]/textureBounds.height()
> 
> Might be a bit easier to read if you create local variables for "right" and
> "bottom" and use those instead of "top+height" and "left+subRectF.width()"

Agreed, done.

Comment 9

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (:kats) from comment #6)
> Comment on attachment 618648 [details] [diff] [review]
> Part 2 - Tidy up TileLayer.java slightly
> 
> Review of attachment 618648 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/gfx/TileLayer.java
> @@ +183,5 @@
> > +                                mSize.height, 0, glInfo.format, glInfo.type, imageBuffer);
> > +        } else {
> > +            // Our texture has been expanded to the next power of two.
> > +            // XXX We probably never want to take this path, so throw an exception.
> > +            throw new RuntimeException("Buffer/image size mismatch in TileLayer!");
> 
> Until bug 748531 is fixed this is pretty risky, because the exception will
> propagate into some JNI wrapper and disappear. This will lead to
> hard-to-find errors if it ever enters this code path. Maybe for now doing a
> Log.e is good enough?

This is a RuntimeException, so it should crash (but with output in logcat, instead of semi-randomly)?

Comment 10

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (:kats) from comment #7)
> Comment on attachment 618649 [details] [diff] [review]
> Part 3 - Make ScreenshotLayer use SingleTileLayer's draw function
> 
> Review of attachment 618649 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few nits, nothing major.
> 
> ::: mobile/android/base/GeckoAppShell.java
> @@ +2223,1 @@
> >          // source width and height to screenshot
> 
> Remove log? Or make it Log.v. It seems like this will get called a fair
> amount.

We throttle quite heavily when we take whole-page screenshots, but I don't mind taking it out.

> ::: mobile/android/base/gfx/LayerRenderer.java
> @@ +190,1 @@
> >          try {
> 
> Move the reset call into the try block

Actually, this reset should be outside of the transaction, well caught.

> @@ +609,5 @@
> > +                    rootMask.right = mPageRect.right + 1;
> > +                }
> > +                if (rootMask.bottom >= mPageRect.bottom - 2) {
> > +                    rootMask.bottom = mPageRect.bottom + 1;
> > +                }
> 
> This entire block seems like a heuristic. I'd prefer if it were moved into a
> separate function, so that this code ends up looking like
>   rootMask = calculateBestRootMask(rootLayer.getBounds(mPageContext),
> mPageRect);
> 
> where "calculateBestRootMask" should be named something more appropriate.

Good idea, will do.

> ::: widget/android/AndroidBridge.h
> @@ +184,5 @@
> >      static void RemovePluginView(void* surface);
> >  
> > +    /* These are defined in mobile/android/base/GeckoAppShell.java */
> > +    enum {
> > +        SCREENSHOT_THUMBNAIL = 0,
> 
> Add a corresponding comment in GeckoAppShell saying that these should be
> kept in sync.

Will do.

Comment 11

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

(In reply to Chris Lord [:cwiiis] from comment #9)
> This is a RuntimeException, so it should crash (but with output in logcat,
> instead of semi-randomly)?

No, it depends on who catches the RuntimeException and what they do with it. In this case it will get thrown on the compositor thread and there's nobody catching it, so it will propagate out into the JNI code that invokes it (probably DrawWindowUnderlay). It will set some flag in Dalvik but won't die until some random point in the future when some JNI code is invoked.

That being said, the correct fix is to modify the JNI code so I'm fine with you landing this as long as we make sure to fix bug 748531 too.

Comment 12

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Fix SingleTileLayer.java

Attaching for reference. Carrying r=kats.

Comment 13

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Make ScreenshotLayer use SingleTileLayer's draw function

Attaching for reference, carrying r=kats.

Comment 14

[Chris Lord [:cwiiis]]

Pushed to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/83c1ca582ddf
http://hg.mozilla.org/integration/mozilla-inbound/rev/4f3a9a2bb4db
http://hg.mozilla.org/integration/mozilla-inbound/rev/11299c6ddc05

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/83c1ca582ddf
https://hg.mozilla.org/mozilla-central/rev/4f3a9a2bb4db
https://hg.mozilla.org/mozilla-central/rev/11299c6ddc05

Comment 16

[JP Rosevear [:jpr]]

Comment on attachment 618648 [details] [diff] [review]
Part 2 - Tidy up TileLayer.java slightly

Mobile only.

Comment 17

[JP Rosevear [:jpr]]

Comment on attachment 618714 [details] [diff] [review]
Part 1 - Fix SingleTileLayer.java

Mobile only.

Comment 18

[JP Rosevear [:jpr]]

Comment on attachment 618715 [details] [diff] [review]
Part 3 - Make ScreenshotLayer use SingleTileLayer's draw function

Mobile only.

Comment 19

[Chris Lord [:cwiiis]]

Landed on aurora:

http://hg.mozilla.org/releases/mozilla-aurora/rev/3f863ee66bef
http://hg.mozilla.org/releases/mozilla-aurora/rev/6929c774dc4c
http://hg.mozilla.org/releases/mozilla-aurora/rev/ee2082335e39