Closed Bug 708307 Opened 8 years ago Closed 8 years ago

Decouple tile size from texture size

Categories

(Firefox for Android :: General, defect, P2, blocker)

All
Android
defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox11 --- fixed
fennec 11+ ---

People

(Reporter: cwiiis, Assigned: cwiiis)

References

Details

Attachments

(2 files, 7 obsolete files)

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.
Looking for feedback - the changing of the danger zone is just some experimentation, please ignore :)
Attachment #579743 - Flags: feedback?(pwalton)
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
Blocks: 705387
(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.
Address feedback comments.
Attachment #579743 - Attachment is obsolete: true
Attachment #580870 - Flags: review?(pwalton)
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.
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+
(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.
(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.
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)
Blocks: 709152
Attachment #581573 - Attachment is obsolete: true
Attachment #581573 - Flags: review?(pwalton)
Attachment #581603 - Flags: review?(pwalton)
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()
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+
http://hg.mozilla.org/mozilla-central/rev/3e972d3efc11
Status: NEW → RESOLVED
Closed: 8 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
(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.
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)
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.
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
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!
Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/c2466854fa15
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/c2466854fa15
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
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
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
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).
Depends on: 713774
tracking-fennec: --- → 11+
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.
(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.
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.