Closed Bug 680082 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: Graphics, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: heeen, Assigned: BenWa)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
Blocks: 657893
Attached patch just drop the check (obsolete) — Splinter Review
Attachment #554088 - Flags: review?(matt.woodrow)
No longer blocks: 657893
It sounds better to add a GLContext::MaxTextureImageSize() (or similar) and have that return an appropriate value for each platform.
Attached patch Add mMaxTextureImageSize (obsolete) — Splinter Review
I added 'mMaxTextureImageSize' instead of adding a virtual call. We're trading off GLint of storage to save a virtual call.
Attachment #554088 - Attachment is obsolete: true
Attachment #554088 - Flags: review?(matt.woodrow)
Attachment #568534 - Flags: review?(matt.woodrow)
Summary: Texture Size check in ThebesLayerOGL is wrong → Texture Size check in ThebesLayerOGL is wrong for platforms that use TiledTextureImage
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.
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.
What would be an appropriate value for MaxTextureImageSize? Should we return int_max?
That seems reasonable to me.
Attached patch Add mMaxTextureImageSize v2 (obsolete) — Splinter Review
Assignee: nobody → bgirard
Attachment #568534 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #568534 - Flags: review?(matt.woodrow)
Attachment #569756 - Flags: review?(matt.woodrow)
Forgot to qref?
Attached patch patchSplinter Review
Yea :(
Attachment #569756 - Attachment is obsolete: true
Attachment #569756 - Flags: review?(matt.woodrow)
Attachment #569804 - Flags: review?(matt.woodrow)
Attachment #569804 - Flags: review?(matt.woodrow) → review+
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
https://hg.mozilla.org/mozilla-central/rev/97a7011b2670
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Blocks: 880481
You need to log in before you can comment on or make changes to this bug.