The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: heeen, Assigned: BenWa)

Tracking

(Blocks: 1 bug)

Trunk
mozilla10
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Updated

6 years ago
Blocks: 607684
(Reporter)

Updated

6 years ago
Blocks: 657893
(Reporter)

Comment 1

6 years ago
Created attachment 554088 [details] [diff] [review]
just drop the check
Attachment #554088 - Flags: review?(matt.woodrow)
(Reporter)

Updated

6 years ago
No longer blocks: 657893
It sounds better to add a GLContext::MaxTextureImageSize() (or similar) and have that return an appropriate value for each platform.
(Assignee)

Comment 3

6 years ago
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.
Attachment #554088 - Attachment is obsolete: true
Attachment #554088 - Flags: review?(matt.woodrow)
Attachment #568534 - Flags: review?(matt.woodrow)
(Assignee)

Updated

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

Comment 5

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

Comment 6

6 years ago
What would be an appropriate value for MaxTextureImageSize? Should we return int_max?
That seems reasonable to me.
(Assignee)

Comment 8

6 years ago
Created attachment 569756 [details] [diff] [review]
Add mMaxTextureImageSize v2
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?
(Assignee)

Comment 10

6 years ago
Created attachment 569804 [details] [diff] [review]
patch

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+
(Assignee)

Comment 11

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc3a32957e85
(Assignee)

Comment 12

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

Comment 13

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/97a7011b2670
https://hg.mozilla.org/mozilla-central/rev/97a7011b2670
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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.