680082 - Texture Size check in ThebesLayerOGL is wrong for platforms that use TiledTextureImage

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Florian Hänel [:heeen]]

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ThebesLayerOGL.cpp#439

This check is wrong since the introduction of TiledTextureImage allows us to ahve TextureImages bigger than an individual texture. It causes unpainted buffers if you try to use a displayport bigger than 2k for example.

On the other hand, we don't use TiledTI on desktop, so there the check may still be required. I don't know what the worst case scenario is on desktop, probably it just fails with GL errors, which would be no worse than not doing any painting.

The check was introduced due to a pecularity of the SGX driver that caused a system lock or reboot when you fed it too large textures if I remember correctly.

I propose dropping this check or replacing it with a check that takes into account how big a Texture*Image* can be on the used backend.

Comment 1

[Florian Hänel [:heeen]]

Attachment: just drop the check

Comment 2

[Matt Woodrow (:mattwoodrow)]

It sounds better to add a GLContext::MaxTextureImageSize() (or similar) and have that return an appropriate value for each platform.

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Add mMaxTextureImageSize

I added 'mMaxTextureImageSize' instead of adding a virtual call. We're trading off GLint of storage to save a virtual call.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 568534 [details] [diff] [review]
Add mMaxTextureImageSize

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

Looks good in general.

::: gfx/thebes/GLContextProviderEGL.cpp
@@ +729,5 @@
> +
> +        mMaxTextureImageSize = mMaxTextureSize * 4;
> +        if (mMaxTextureImageSize < 2048) {
> +            mMaxTextureImageSize = 2048;
> +        }

I don't really understand this part.

Why are our tiled texture images limited to 4x the normal width? I can't see anywhere in the TiledTextureImage code that would create this limitation. If it does exist, it probably should be exposed through a static ::GetMaxTiles() func, or some sort of static var.

Why are we clamping to a minimum of 2048? I thought we required mMaxTextureSize to be at least 2048, this this should always be 4x that?
This should probably just be an assertion rather than adjusting the var.

Comment 5

[Benoit Girard (:BenWa)]

Right, now that I think more about it this code is useless. I meant to limit this to a reasonable size such as 8192x8192.

I'm going to remove this check.

Comment 6

[Benoit Girard (:BenWa)]

What would be an appropriate value for MaxTextureImageSize? Should we return int_max?

Comment 7

[Matt Woodrow (:mattwoodrow)]

That seems reasonable to me.

Comment 8

[Benoit Girard (:BenWa)]

Attachment: Add mMaxTextureImageSize v2

Comment 9

[Matt Woodrow (:mattwoodrow)]

Forgot to qref?

Comment 10

[Benoit Girard (:BenWa)]

Attachment: patch

Yea :(

Comment 11

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc3a32957e85

Comment 12

[Benoit Girard (:BenWa)]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dffaf14f66d3

Didn't know we used ProviderEGL on windows.

gfx/thebes/GLContextProviderEGL.cpp(731) : error C2065: 'INT32_MAX' : undeclared identifier

Comment 13

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97a7011b2670

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/97a7011b2670