Last Comment Bug 680082 - Texture Size check in ThebesLayerOGL is wrong for platforms that use TiledTextureImage
: Texture Size check in ThebesLayerOGL is wrong for platforms that use TiledTex...
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Linux
-- normal (vote)
: mozilla10
Assigned To: Benoit Girard (:BenWa)
: Milan Sreckovic [:milan]
Depends on:
Blocks: 880481 opengl-mobile
  Show dependency treegraph
Reported: 2011-08-18 07:37 PDT by Florian Hänel [:heeen]
Modified: 2013-06-06 15:17 PDT (History)
10 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

just drop the check (915 bytes, patch)
2011-08-18 07:56 PDT, Florian Hänel [:heeen]
no flags Details | Diff | Splinter Review
Add mMaxTextureImageSize (4.00 KB, patch)
2011-10-20 15:05 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Add mMaxTextureImageSize v2 (4.01 KB, patch)
2011-10-26 12:27 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
patch (3.96 KB, patch)
2011-10-26 14:38 PDT, Benoit Girard (:BenWa)
matt.woodrow: review+
Details | Diff | Splinter Review

Description User image Florian Hänel [:heeen] 2011-08-18 07:37:28 PDT

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 User image Florian Hänel [:heeen] 2011-08-18 07:56:54 PDT
Created attachment 554088 [details] [diff] [review]
just drop the check
Comment 2 User image Matt Woodrow (:mattwoodrow) 2011-08-18 13:24:39 PDT
It sounds better to add a GLContext::MaxTextureImageSize() (or similar) and have that return an appropriate value for each platform.
Comment 3 User image Benoit Girard (:BenWa) 2011-10-20 15:05:34 PDT
Created attachment 568534 [details] [diff] [review]
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 User image Matt Woodrow (:mattwoodrow) 2011-10-21 14:58:04 PDT
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 User image Benoit Girard (:BenWa) 2011-10-21 15:05:15 PDT
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 User image Benoit Girard (:BenWa) 2011-10-24 14:25:19 PDT
What would be an appropriate value for MaxTextureImageSize? Should we return int_max?
Comment 7 User image Matt Woodrow (:mattwoodrow) 2011-10-24 14:28:56 PDT
That seems reasonable to me.
Comment 8 User image Benoit Girard (:BenWa) 2011-10-26 12:27:46 PDT
Created attachment 569756 [details] [diff] [review]
Add mMaxTextureImageSize v2
Comment 9 User image Matt Woodrow (:mattwoodrow) 2011-10-26 14:14:54 PDT
Forgot to qref?
Comment 10 User image Benoit Girard (:BenWa) 2011-10-26 14:38:39 PDT
Created attachment 569804 [details] [diff] [review]

Yea :(
Comment 12 User image Benoit Girard (:BenWa) 2011-10-27 07:20:16 PDT

Didn't know we used ProviderEGL on windows.

gfx/thebes/GLContextProviderEGL.cpp(731) : error C2065: 'INT32_MAX' : undeclared identifier
Comment 14 User image Marco Bonardo [::mak] 2011-10-29 01:50:54 PDT

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