Decouple tile size from texture size

RESOLVED FIXED

Status

()

Firefox for Android
General
P2
blocker
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

unspecified
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
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 :)
Attachment #579743 - Flags: feedback?(pwalton)
(Assignee)

Updated

6 years ago
Summary: Decouple layer size from tile size → Decouple tile size from texture size
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.
Attachment #579743 - Flags: feedback?(pwalton) → feedback+
Assignee: nobody → chrislord.net
Priority: -- → P2
(Assignee)

Updated

5 years ago
Blocks: 705387
(Assignee)

Comment 3

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

Comment 4

5 years ago
Created attachment 580870 [details] [diff] [review]
Decouple tile size and texture size (revised)

Address feedback comments.
Attachment #579743 - Attachment is obsolete: true
Attachment #580870 - Flags: review?(pwalton)
(Assignee)

Comment 5

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

Comment 7

5 years ago
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).
Attachment #580870 - Attachment is obsolete: true
Attachment #580870 - Flags: review?(pwalton)
Attachment #580890 - Flags: review?(pwalton)
Blocks: 704515
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.
Attachment #580890 - Flags: feedback+
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?
Attachment #580890 - Flags: review?(pwalton) → review+
(Assignee)

Comment 10

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

Comment 11

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

Comment 13

5 years ago
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.
Attachment #580890 - Attachment is obsolete: true
Attachment #581573 - Flags: review?(pwalton)
(Assignee)

Updated

5 years ago
Blocks: 709152
(Assignee)

Comment 14

5 years ago
Created attachment 581603 [details] [diff] [review]
Decouple tile size and texture size (revised, rebased)
Attachment #581573 - Attachment is obsolete: true
Attachment #581573 - Flags: review?(pwalton)
Attachment #581603 - Flags: review?(pwalton)
(Assignee)

Comment 15

5 years ago
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.
Attachment #581603 - Attachment is obsolete: true
Attachment #581603 - Flags: review?(pwalton)
Attachment #581640 - Flags: review?(pwalton)
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()
(Assignee)

Comment 17

5 years ago
Created attachment 581649 [details] [diff] [review]
Decouple tile size and texture size (revised, rebased)

Thanks Brad - I'll get it right eventually :p
Attachment #581640 - Attachment is obsolete: true
Attachment #581640 - Flags: review?(pwalton)
Attachment #581649 - Flags: review?(pwalton)
(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 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.
Attachment #581649 - Flags: review?(pwalton) → review+
(Assignee)

Comment 20

5 years ago
http://hg.mozilla.org/mozilla-central/rev/3e972d3efc11
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Backed out because of test failures: https://hg.mozilla.org/mozilla-central/rev/d508455660d3
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 705387
Duplicate of this bug: 704515
So now it seems that all checker boards related bugs are duplictes of this one?
Checkerboard bugs caused by screens wider than 1024 pixels are duplicates of this one.
I understand, thanks!
So, whenever this bug is fixed I won't see any more checker boards in my tablet! Is this correct?
So if a blocker bug has been duped to this then I guess the severity should be copied also.
Severity: normal → blocker
(Assignee)

Comment 28

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

Comment 29

5 years ago
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).
Attachment #581649 - Attachment is obsolete: true
Attachment #581924 - Flags: review?(pwalton)
(Assignee)

Comment 30

5 years ago
Created attachment 581925 [details] [diff] [review]
Changes made in last revision

These are the changes made in the last revision, for ease of reviewing.
(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

5 years ago
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
Blocks: 670930

Comment 33

5 years ago
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 on attachment 581924 [details] [diff] [review]
Decouple tile size and texture size (fix test crashes)

r=me
Attachment #581924 - Flags: review?(pwalton) → review+
(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.
(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!
(Assignee)

Comment 37

5 years ago
Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/c2466854fa15
Whiteboard: [inbound]
(Assignee)

Comment 38

5 years ago
http://hg.mozilla.org/mozilla-central/rev/c2466854fa15
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]

Updated

5 years ago
Depends on: 711426
Looks good. My Galaxy Nexus has no more checkboarding in landscape mode.
I still see checkboarding to both sides of all sites in my 10.1 Tab though.
This did not make today's nightly build, should be there tomorrow.
Ah, well.... thanks Bill. I'll be looking forward to reading planet.mozilla.org and some newspapers in landscape mode tomorrow then!
Duplicate of this bug: 711813

Comment 44

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

Comment 45

5 years ago
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).

Updated

5 years ago
Depends on: 713774
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
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.
(Assignee)

Comment 47

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

Comment 48

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.