Last Comment Bug 708307 - Decouple tile size from texture size
: Decouple tile size from texture size
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All Android
: P2 blocker (vote)
: ---
Assigned To: Chris Lord [:cwiiis]
:
Mentors:
: 704515 705387 711813 (view as bug list)
Depends on: 711426 713774
Blocks: 670930 704515 705387 709152
  Show dependency treegraph
 
Reported: 2011-12-07 10:34 PST by Chris Lord [:cwiiis]
Modified: 2012-02-22 01:38 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
Decouple tile size and texture size (27.58 KB, patch)
2011-12-07 10:37 PST, Chris Lord [:cwiiis]
pwalton: feedback+
Details | Diff | Review
Decouple tile size and texture size (revised) (34.71 KB, patch)
2011-12-12 03:26 PST, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Decouple tile size and texture size (revised) (35.05 KB, patch)
2011-12-12 06:07 PST, Chris Lord [:cwiiis]
pwalton: review+
bugmail.mozilla: feedback+
Details | Diff | Review
Decouple tile size and texture size (revised) (35.02 KB, patch)
2011-12-14 03:07 PST, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Decouple tile size and texture size (revised, rebased) (34.60 KB, patch)
2011-12-14 05:39 PST, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Decouple tile size and texture size (revised, rebased) (34.65 KB, patch)
2011-12-14 07:55 PST, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Decouple tile size and texture size (revised, rebased) (34.67 KB, patch)
2011-12-14 08:28 PST, Chris Lord [:cwiiis]
pwalton: review+
Details | Diff | Review
Decouple tile size and texture size (fix test crashes) (37.41 KB, patch)
2011-12-15 02:54 PST, Chris Lord [:cwiiis]
pwalton: review+
Details | Diff | Review
Changes made in last revision (4.07 KB, patch)
2011-12-15 02:59 PST, Chris Lord [:cwiiis]
no flags Details | Diff | Review

Description Chris Lord [:cwiiis] 2011-12-07 10:34:12 PST
Currently, we have a hard-coded tile-size of 1024x2048, which also corresponds to the layer size. It'd be good if we could have specify a buffer size that related to the screen-size somehow, and also that was independent of any texture-size limitations we may have.
Comment 1 Chris Lord [:cwiiis] 2011-12-07 10:37:41 PST
Created attachment 579743 [details] [diff] [review]
Decouple tile size and texture size

Looking for feedback - the changing of the danger zone is just some experimentation, please ignore :)
Comment 2 Patrick Walton (:pcwalton) 2011-12-08 09:17:32 PST
Comment on attachment 579743 [details] [diff] [review]
Decouple tile size and texture size

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

::: mobile/android/base/gfx/IntSize.java
@@ +80,5 @@
> +
> +    public IntSize nextPowerOfTwo() {
> +        double log2 = Math.log(2);
> +        return new IntSize((int)Math.pow(2, Math.ceil(Math.log(width)/log2)),
> +                           (int)Math.pow(2, Math.ceil(Math.log(height)/log2)));

Here's a faster way to calculate the next power of two:

http://acius2.blogspot.com/2007/11/calculating-next-power-of-2.html

But could we just move to GLES 2.0, which supports NPOT textures? We're already linking against GLES 2.0.

::: mobile/android/base/gfx/LayerClient.java
@@ +78,5 @@
>      }
> +
> +    public IntSize getSize() {
> +        Log.e(LOGTAG, "LayerClient.getSize() called on a class that doesn't override it!");
> +        return new IntSize(0, 0);

Could we just make this function abstract? LayerClient is already an abstract class.
Comment 3 Chris Lord [:cwiiis] 2011-12-12 02:53:52 PST
(In reply to Patrick Walton (:pcwalton) from comment #2)
> Comment on attachment 579743 [details] [diff] [review]
> Decouple tile size and texture size
> 
> Review of attachment 579743 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/gfx/IntSize.java
> @@ +80,5 @@
> > +
> > +    public IntSize nextPowerOfTwo() {
> > +        double log2 = Math.log(2);
> > +        return new IntSize((int)Math.pow(2, Math.ceil(Math.log(width)/log2)),
> > +                           (int)Math.pow(2, Math.ceil(Math.log(height)/log2)));
> 
> Here's a faster way to calculate the next power of two:
> 
> http://acius2.blogspot.com/2007/11/calculating-next-power-of-2.html

Aware of this, but wasn't sure you could do shifting in Java - having just looked it up, I see that you can though (and not sure why I assumed you couldn't), so will replace.

> But could we just move to GLES 2.0, which supports NPOT textures? We're
> already linking against GLES 2.0.

I think this ought to be a separate patch, but yes, we should do. Will write one and file a bug. I've also changed the code so that it removes the wastage for now, until we use NPOT textures (and to not change behaviour until I have metrics that say it's a good idea).

> ::: mobile/android/base/gfx/LayerClient.java
> @@ +78,5 @@
> >      }
> > +
> > +    public IntSize getSize() {
> > +        Log.e(LOGTAG, "LayerClient.getSize() called on a class that doesn't override it!");
> > +        return new IntSize(0, 0);
> 
> Could we just make this function abstract? LayerClient is already an
> abstract class.

Yes, somehow missed that LayerClient was abstract, done.
Comment 4 Chris Lord [:cwiiis] 2011-12-12 03:26:37 PST
Created attachment 580870 [details] [diff] [review]
Decouple tile size and texture size (revised)

Address feedback comments.
Comment 5 Chris Lord [:cwiiis] 2011-12-12 03:43:59 PST
Actually, I didn't realise how trivial NPOT textures are with GLES20 and I've let some debug logging slip into this patch, so patch incoming with NPOT support + no debug log.
Comment 6 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2011-12-12 05:16:21 PST
> > Here's a faster way to calculate the next power of two:
> > 
> > http://acius2.blogspot.com/2007/11/calculating-next-power-of-2.html
> 
> Aware of this, but wasn't sure you could do shifting in Java - having just
> looked it up, I see that you can though (and not sure why I assumed you
> couldn't), so will replace.
> 

FYI, I added this code to Layer.java in my patch for scrollbars.
Comment 7 Chris Lord [:cwiiis] 2011-12-12 06:07:12 PST
Created attachment 580890 [details] [diff] [review]
Decouple tile size and texture size (revised)

Here's another revision that uses NPOT textures if they're available and removes the logging that I accidentally left in.

I've also changed the default buffer-x/y so that on an 800x480 screen, we end up with a 1024x2048 buffer, as I assumed this would be the common case and it'd be nice not to alter behaviour too much without metrics (also, I'd assume that a device that doesn't support GLES2.0, but that is supported, is likely to have an 800x480 screen - a lot of our UI seems to assume that that's the minimum resolution).
Comment 8 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2011-12-13 07:53:56 PST
Comment on attachment 580890 [details] [diff] [review]
Decouple tile size and texture size (revised)

Overall it looks good to me, but I think pcwalton should still review.

>diff --git a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
>-        mWidth = LayerController.TILE_WIDTH;
>-        mHeight = LayerController.TILE_HEIGHT;
>+        mScreenSize = new IntSize(0, 0);
>+        mBufferSize = new IntSize(0, 0);
>         mFormat = CairoImage.FORMAT_RGB16_565;
> 
>-        mScreenSize = new IntSize(1, 1);

In general I'm not a fan of what we've been doing with respect to initializing these variables. I'd rather leave them at null than initialize them to invalid values. At least that way we can detect it better (via NPEs) and not run code that shouldn't be running and/or is a waste of time on startup. What do you think?

>+            mBufferSize = new IntSize(Math.min(maxSize, mScreenSize.width + LayerController.BUFFER_X),
>+                                      Math.min(maxSize, mScreenSize.height + LayerController.BUFFER_Y));
>+
>+            // * 2 because it's a 16-bit buffer (so 2bpp).
>+            mBuffer = ByteBuffer.allocateDirect(mBufferSize.getArea() * 2);

Because this is allocating a large chunk of memory, it might be better to first do mBuffer = null; followed by mBuffer = ByteBuffer.allocateDirect(...). This allows the VM to garbage-collect the old buffer before allocating the new one, and reduces the chance of OOMs.

>diff --git a/mobile/android/base/gfx/LayerController.java b/mobile/android/base/gfx/LayerController.java
>+    /* The extra area on the sides of the page that we want to buffer to help with
>+     * smooth, asynchronous scrolling.
>+     */
>+    public static final int BUFFER_X = 544;
>+    public static final int BUFFER_Y = 1248;

Where do these numbers come from?

>diff --git a/mobile/android/base/gfx/TileLayer.java b/mobile/android/base/gfx/TileLayer.java
>     public TileLayer(boolean repeat, CairoImage image) {
>         mRepeat = repeat;
>         mImage = image;
>-        mSize = new IntSize(image.getWidth(), image.getHeight());
>+        mSize = new IntSize(0, 0);

Same thing with initializing variables here.

>+        // Don't do any work if the image has an invalid size.
>+        if (mImage.getSize().equals(new IntSize(0, 0)))
>+            return;

This could be a null check instead if we changed the initialization code.

>diff --git a/mobile/android/base/gfx/ViewportMetrics.java b/mobile/android/base/gfx/ViewportMetrics.java
>     public ViewportMetrics() {
>-        mPageSize = new FloatSize(LayerController.TILE_WIDTH,
>-                                  LayerController.TILE_HEIGHT);
>+        mPageSize = new FloatSize(1, 1);
>         mViewportRect = new RectF(0, 0, 1, 1);

Ditto.
Comment 9 Patrick Walton (:pcwalton) 2011-12-13 09:30:28 PST
Comment on attachment 580890 [details] [diff] [review]
Decouple tile size and texture size (revised)

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

r+ with comments addressed

::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java
@@ +114,5 @@
>          mTileLayer = new SingleTileLayer(mCairoImage);
>      }
>  
> +    @Override
> +    public IntSize getSize() {

Could you add a comment here explaining what this size means?

@@ +243,5 @@
> +
> +            mBufferSize = new IntSize(Math.min(maxSize, mScreenSize.width + LayerController.BUFFER_X),
> +                                      Math.min(maxSize, mScreenSize.height + LayerController.BUFFER_Y));
> +
> +            // * 2 because it's a 16-bit buffer (so 2bpp).

nit: usually bpp is bits per pixel

::: mobile/android/base/gfx/LayerClient.java
@@ +44,5 @@
>      private LayerController mLayerController;
>  
>      public abstract void geometryChanged();
>      public abstract void viewportSizeChanged();
> +    public abstract IntSize getSize();

Could you call this getBufferSize() instead? "Size" is getting overloaded a lot.

::: mobile/android/base/gfx/LayerController.java
@@ +278,5 @@
>          return aboutToCheckerboard() && mPanZoomController.getRedrawHint();
>      }
>  
>      private RectF getTileRect() {
> +        if (mLayerClient == null)

When does this happen?

::: mobile/android/base/gfx/TileLayer.java
@@ +105,5 @@
> +         */
> +        IntSize bufferSize = mImage.getSize();
> +        IntSize textureSize = bufferSize;
> +
> +        if (mRepeat || !(gl instanceof GLES20))

GLES20 is a static class, so I'm surprised that gl would actually be an instance of GLES20. Have you verified that this is actually the case?
Comment 10 Chris Lord [:cwiiis] 2011-12-13 09:37:20 PST
(In reply to Kartikaya Gupta (:kats) from comment #8)
> Comment on attachment 580890 [details] [diff] [review]
> Decouple tile size and texture size (revised)
> 
> Overall it looks good to me, but I think pcwalton should still review.
> 
> >diff --git a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
> >-        mWidth = LayerController.TILE_WIDTH;
> >-        mHeight = LayerController.TILE_HEIGHT;
> >+        mScreenSize = new IntSize(0, 0);
> >+        mBufferSize = new IntSize(0, 0);
> >         mFormat = CairoImage.FORMAT_RGB16_565;
> > 
> >-        mScreenSize = new IntSize(1, 1);
> 
> In general I'm not a fan of what we've been doing with respect to
> initializing these variables. I'd rather leave them at null than initialize
> them to invalid values. At least that way we can detect it better (via NPEs)
> and not run code that shouldn't be running and/or is a waste of time on
> startup. What do you think?

It's an idea - I'm not really for or against one way or the other. I guess an argument against would be that not initialising them might make us less vigilant about checking for invalid values, but I have no strong feelings either way.

Maybe in another patch though? Not initialising them would probably touch a fair amount.

> >+            mBufferSize = new IntSize(Math.min(maxSize, mScreenSize.width + LayerController.BUFFER_X),
> >+                                      Math.min(maxSize, mScreenSize.height + LayerController.BUFFER_Y));
> >+
> >+            // * 2 because it's a 16-bit buffer (so 2bpp).
> >+            mBuffer = ByteBuffer.allocateDirect(mBufferSize.getArea() * 2);
> 
> Because this is allocating a large chunk of memory, it might be better to
> first do mBuffer = null; followed by mBuffer =
> ByteBuffer.allocateDirect(...). This allows the VM to garbage-collect the
> old buffer before allocating the new one, and reduces the chance of OOMs.

Will do, thanks.

> >diff --git a/mobile/android/base/gfx/LayerController.java b/mobile/android/base/gfx/LayerController.java
> >+    /* The extra area on the sides of the page that we want to buffer to help with
> >+     * smooth, asynchronous scrolling.
> >+     */
> >+    public static final int BUFFER_X = 544;
> >+    public static final int BUFFER_Y = 1248;
> 
> Where do these numbers come from?

They're 1024 - 480 and 2048 - 800... But I realise now the logic is flawed and we should probably just explicitly round up to the next nearest power of two for the screen size and have a minimum(/maximum?) buffer size.
Comment 11 Chris Lord [:cwiiis] 2011-12-13 09:41:09 PST
(In reply to Patrick Walton (:pcwalton) from comment #9)
> Comment on attachment 580890 [details] [diff] [review]
> Decouple tile size and texture size (revised)
> 
> Review of attachment 580890 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed
> 
> ::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java
> @@ +114,5 @@
> >          mTileLayer = new SingleTileLayer(mCairoImage);
> >      }
> >  
> > +    @Override
> > +    public IntSize getSize() {
> 
> Could you add a comment here explaining what this size means?

Sure.

> @@ +243,5 @@
> > +
> > +            mBufferSize = new IntSize(Math.min(maxSize, mScreenSize.width + LayerController.BUFFER_X),
> > +                                      Math.min(maxSize, mScreenSize.height + LayerController.BUFFER_Y));
> > +
> > +            // * 2 because it's a 16-bit buffer (so 2bpp).
> 
> nit: usually bpp is bits per pixel

Right, will clarify.

> ::: mobile/android/base/gfx/LayerClient.java
> @@ +44,5 @@
> >      private LayerController mLayerController;
> >  
> >      public abstract void geometryChanged();
> >      public abstract void viewportSizeChanged();
> > +    public abstract IntSize getSize();
> 
> Could you call this getBufferSize() instead? "Size" is getting overloaded a
> lot.

Good idea.

> ::: mobile/android/base/gfx/LayerController.java
> @@ +278,5 @@
> >          return aboutToCheckerboard() && mPanZoomController.getRedrawHint();
> >      }
> >  
> >      private RectF getTileRect() {
> > +        if (mLayerClient == null)
> 
> When does this happen?

If you called getRedrawHint before the layer client was set, this could happen (but we don't - shall I remove, or leave to err on the side of caution?)

> ::: mobile/android/base/gfx/TileLayer.java
> @@ +105,5 @@
> > +         */
> > +        IntSize bufferSize = mImage.getSize();
> > +        IntSize textureSize = bufferSize;
> > +
> > +        if (mRepeat || !(gl instanceof GLES20))
> 
> GLES20 is a static class, so I'm surprised that gl would actually be an
> instance of GLES20. Have you verified that this is actually the case?

I get NPOT textures on my device, so I assume it works - the Android docs recommend this as the way to check for versions (I was surprised too..)

Thanks for the review both, will address.
Comment 12 Patrick Walton (:pcwalton) 2011-12-13 14:19:14 PST
(In reply to Chris Lord [:cwiiis] from comment #11)
> If you called getRedrawHint before the layer client was set, this could
> happen (but we don't - shall I remove, or leave to err on the side of
> caution?)

I'm ok with it staying there.
Comment 13 Chris Lord [:cwiiis] 2011-12-14 03:07:15 PST
Created attachment 581573 [details] [diff] [review]
Decouple tile size and texture size (revised)

Ends up my GLES2.0 bits were entirely wrong and it isn't as trivial as I thought, so I'm going to file a separate bug for that. I've split that away and this is just the revised decoupling.
Comment 14 Chris Lord [:cwiiis] 2011-12-14 05:39:32 PST
Created attachment 581603 [details] [diff] [review]
Decouple tile size and texture size (revised, rebased)
Comment 15 Chris Lord [:cwiiis] 2011-12-14 07:55:18 PST
Created attachment 581640 [details] [diff] [review]
Decouple tile size and texture size (revised, rebased)

Erk, just noticed I wasn't freeing the old buffer correctly. Fixed, should be good now.
Comment 16 Brad Lassey [:blassey] (use needinfo?) 2011-12-14 08:09:49 PST
Comment on attachment 581640 [details] [diff] [review]
Decouple tile size and texture size (revised, rebased)

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

drive by, I'm surprised you're not seeing crashes with this patch because it looks like you'll double free these buffers

::: mobile/android/base/gfx/BufferedCairoImage.java
@@ +62,3 @@
>          mNeedToFreeBuffer = true;
> +        // XXX Why is this * 4? Shouldn't it depend on mFormat?
> +        mBuffer = ByteBuffer.allocateDirect(mSize.getArea() * 4);

use GeckoAppShell.allocateDirectBuffer()

::: mobile/android/base/gfx/GeckoSoftwareLayerClient.java
@@ +251,5 @@
> +                mBuffer = null;
> +            }
> +
> +            // * 2 because it's a 16-bit buffer (so 2 bytes per pixel).
> +            mBuffer = ByteBuffer.allocateDirect(mBufferSize.getArea() * 2);

use GeckoAppShell.allocateDirectBuffer()
Comment 17 Chris Lord [:cwiiis] 2011-12-14 08:28:06 PST
Created attachment 581649 [details] [diff] [review]
Decouple tile size and texture size (revised, rebased)

Thanks Brad - I'll get it right eventually :p
Comment 18 Patrick Walton (:pcwalton) 2011-12-14 11:15:31 PST
(In reply to Chris Lord [:cwiiis] from comment #17)
> Created attachment 581649 [details] [diff] [review]
> Decouple tile size and texture size (revised, rebased)
> 
> Thanks Brad - I'll get it right eventually :p

Could you post an interdiff between the last patch I reviewed and this one?
Comment 19 Patrick Walton (:pcwalton) 2011-12-14 11:27:55 PST
Comment on attachment 581649 [details] [diff] [review]
Decouple tile size and texture size (revised, rebased)

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

r=me, but let's move to GLES 2.0 soon. NPOT textures are going to be important to conserve memory.
Comment 20 Chris Lord [:cwiiis] 2011-12-14 11:46:19 PST
http://hg.mozilla.org/mozilla-central/rev/3e972d3efc11
Comment 21 :Ehsan Akhgari (busy, don't ask for review please) 2011-12-14 12:56:54 PST
Backed out because of test failures: https://hg.mozilla.org/mozilla-central/rev/d508455660d3
Comment 22 Matt Brubeck (:mbrubeck) 2011-12-14 14:44:28 PST
*** Bug 705387 has been marked as a duplicate of this bug. ***
Comment 23 Matt Brubeck (:mbrubeck) 2011-12-14 14:45:37 PST
*** Bug 704515 has been marked as a duplicate of this bug. ***
Comment 24 Gabriela [:gaby2300] 2011-12-14 15:02:05 PST
So now it seems that all checker boards related bugs are duplictes of this one?
Comment 25 Matt Brubeck (:mbrubeck) 2011-12-14 15:12:28 PST
Checkerboard bugs caused by screens wider than 1024 pixels are duplicates of this one.
Comment 26 Gabriela [:gaby2300] 2011-12-14 15:29:45 PST
I understand, thanks!
So, whenever this bug is fixed I won't see any more checker boards in my tablet! Is this correct?
Comment 27 Bill Gianopoulos [:WG9s] 2011-12-14 15:31:33 PST
So if a blocker bug has been duped to this then I guess the severity should be copied also.
Comment 28 Chris Lord [:cwiiis] 2011-12-15 02:14:39 PST
(In reply to Gabriela from comment #26)
> I understand, thanks!
> So, whenever this bug is fixed I won't see any more checker boards in my
> tablet! Is this correct?

This isn't quite correct - you'll stop seeing permanent checkerboard on the sides of your device (for any device with a screen dimension > 1024 on the x-axis), but you'll still see checkerboard occasionally, or a lot, while panning. This second issue is being tracked in bug #709152.

I have a fix for the test failures this patch caused - I'll ask for review and I'll wait for a try run to complete before pushing to inbound, however.
Comment 29 Chris Lord [:cwiiis] 2011-12-15 02:54:35 PST
Created attachment 581924 [details] [diff] [review]
Decouple tile size and texture size (fix test crashes)

Don't know why you don't see the crashes running on a device, but this ought to fix those crashes (the stack trace indicated they were caused by not checking the return value of LockBuffer in LockBufferBits).
Comment 30 Chris Lord [:cwiiis] 2011-12-15 02:59:15 PST
Created attachment 581925 [details] [diff] [review]
Changes made in last revision

These are the changes made in the last revision, for ease of reviewing.
Comment 31 Gabriela [:gaby2300] 2011-12-15 03:13:27 PST
(In reply to Chris Lord [:cwiiis] from comment #28)
>> 
> This isn't quite correct - you'll stop seeing permanent checkerboard on the
> sides of your device (for any device with a screen dimension > 1024 on the
> x-axis), but you'll still see checkerboard occasionally, or a lot, while
> panning. This second issue is being tracked in bug #709152.
> 
> I have a fix for the test failures this patch caused - I'll ask for review
> and I'll wait for a try run to complete before pushing to inbound, however.

Thanks for your comment!
Not having permanent checkerboard on the sides of my tab will be just awesome because I'll be able to use it much more often!!! The other issue doesn't bother me as much as the first one.
Comment 32 Mozilla RelEng Bot 2011-12-15 07:20:36 PST
Try run for a94c398086c0 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a94c398086c0
Results (out of 42 total builds):
    success: 32
    warnings: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/chrislord.net@gmail.com-a94c398086c0
Comment 33 Mozilla RelEng Bot 2011-12-15 09:50:51 PST
Try run for a94c398086c0 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a94c398086c0
Results (out of 42 total builds):
    success: 32
    warnings: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/chrislord.net@gmail.com-a94c398086c0
Comment 34 Patrick Walton (:pcwalton) 2011-12-15 10:47:33 PST
Comment on attachment 581924 [details] [diff] [review]
Decouple tile size and texture size (fix test crashes)

r=me
Comment 35 Bill Gianopoulos [:WG9s] 2011-12-15 11:27:47 PST
(In reply to Chris Lord [:cwiiis] from comment #28)
> (In reply to Gabriela from comment #26)
> > I understand, thanks!
> > So, whenever this bug is fixed I won't see any more checker boards in my
> > tablet! Is this correct?
> 
> This isn't quite correct - you'll stop seeing permanent checkerboard on the
> sides of your device (for any device with a screen dimension > 1024 on the
> x-axis), but you'll still see checkerboard occasionally, or a lot, while
> panning. This second issue is being tracked in bug #709152.
> 
> I have a fix for the test failures this patch caused - I'll ask for review
> and I'll wait for a try run to complete before pushing to inbound, however.

Well that is really NOT an issue.  The checker boarding during panning is temporary and as soon as you stop panning it gets redrawn with content.  The permanent checkerboarding on left and right is a quite different issue.  It obliterates part of the page content with no real way to ever display the part of the page that is not being shown, making Firefox being entirely unusable on tablets in landscape mode.  So this patch is really sufficient to fix the issue that should block the Aurara uplift.
Comment 36 Gabriela [:gaby2300] 2011-12-15 11:40:23 PST
(In reply to Bill Gianopoulos from comment #35)
> (In reply to Chris Lord [:cwiiis] from comment #28)

Right, I'll be looking forward to this patch's landing then!
Comment 37 Chris Lord [:cwiiis] 2011-12-15 15:47:16 PST
Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/c2466854fa15
Comment 38 Chris Lord [:cwiiis] 2011-12-16 04:26:12 PST
http://hg.mozilla.org/mozilla-central/rev/c2466854fa15
Comment 39 Chris Peterson [:cpeterson] 2011-12-16 11:52:33 PST
Looks good. My Galaxy Nexus has no more checkboarding in landscape mode.
Comment 40 Gabriela [:gaby2300] 2011-12-16 12:46:12 PST
I still see checkboarding to both sides of all sites in my 10.1 Tab though.
Comment 41 Bill Gianopoulos [:WG9s] 2011-12-16 13:17:54 PST
This did not make today's nightly build, should be there tomorrow.
Comment 42 Gabriela [:gaby2300] 2011-12-16 13:22:36 PST
Ah, well.... thanks Bill. I'll be looking forward to reading planet.mozilla.org and some newspapers in landscape mode tomorrow then!
Comment 43 Kevin Brosnan [:kbrosnan] 2011-12-19 15:40:28 PST
*** Bug 711813 has been marked as a duplicate of this bug. ***
Comment 44 Mozilla RelEng Bot 2011-12-22 12:30:40 PST
Try run for a94c398086c0 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=a94c398086c0
Results (out of 51 total builds):
    exception: 1
    success: 32
    warnings: 10
    failure: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/chrislord.net@gmail.com-a94c398086c0
Comment 45 Chris Lord [:cwiiis] 2011-12-22 12:40:29 PST
Not sure where this try run came from, but just to note that the failures are unrelated to this patch (this patch only changes Java and parts where MOZ_JAVA_COMPOSITOR is defined and the failures are in xul fennec).
Comment 46 Benoit Girard (:BenWa) 2012-02-21 13:41:28 PST
Comment on attachment 581924 [details] [diff] [review]
Decouple tile size and texture size (fix test crashes)

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

::: mobile/android/base/gfx/TileLayer.java
@@ -130,9 +171,9 @@
> > -        gl.glTexImage2D(gl.GL_TEXTURE_2D, 0, glInfo.internalFormat, mSize.width, mSize.height,
> > +        if (newlyCreated || dirtyRect.equals(bufferRect)) {
> > -                        0, glInfo.format, glInfo.type, mImage.getBuffer());
> > +            if (mSize.equals(bufferSize)) {
NaN more ...
> > +                return;
> > -        if (mTextureIDs == null)
> > +            } else {
> > -            throw new RuntimeException("uploadDirtyRect() called with null texture ID!");
NaN more ...

Why are you doing TexImage2D+TexSubImage2D? TexImage2D with the buffer should be sufficient.
Comment 47 Chris Lord [:cwiiis] 2012-02-22 00:57:36 PST
(In reply to Benoit Girard (:BenWa) from comment #46)
> Comment on attachment 581924 [details] [diff] [review]
> Decouple tile size and texture size (fix test crashes)
> 
> Review of attachment 581924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/gfx/TileLayer.java
> @@ -130,9 +171,9 @@
> > > -        gl.glTexImage2D(gl.GL_TEXTURE_2D, 0, glInfo.internalFormat, mSize.width, mSize.height,
> > > +        if (newlyCreated || dirtyRect.equals(bufferRect)) {
> > > -                        0, glInfo.format, glInfo.type, mImage.getBuffer());
> > > +            if (mSize.equals(bufferSize)) {
> NaN more ...
> > > +                return;
> > > -        if (mTextureIDs == null)
> > > +            } else {
> > > -            throw new RuntimeException("uploadDirtyRect() called with null texture ID!");
> NaN more ...

I don't understand these comments?

> Why are you doing TexImage2D+TexSubImage2D? TexImage2D with the buffer
> should be sufficient.

This situation is for when the texture size differs from the image size - as we can't set the stride, a TexImage2D wouldn't be sufficient.
Comment 48 Chris Lord [:cwiiis] 2012-02-22 01:38:56 PST
In fact, I notice this code has been removed - this breaks the situation where our texture size and buffer size are different. I don't think we ever have this situation anymore (or before, in fact), but I'd feel better if this removal was coupled with an assert or if it was backed out.

I'll leave this up to you.

Note You need to log in before you can comment on or make changes to this bug.