848023 - max texture size inconsistency

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

We do not just take the fGetIntegerv(LOCAL_GL_MAX_TEXTURE_SIZE,...) at face value - we adjust it because of bugs like bug 737182.  However, we keep that adjusted value to ourselves, and the rest of the code doesn't know about the adjustments we made.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Take the adjusted max texture size instead

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 721343 [details] [diff] [review]
Take the adjusted max texture size instead

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

::: gfx/gl/GLContext.h
@@ +1014,5 @@
>      static bool ListHasExtension(const GLubyte *extensions,
>                                   const char *extension);
>  
>      GLint GetMaxTextureImageSize() { return mMaxTextureImageSize; }
> +    GLint GetMaxTextureSize() { MOZ_ASSERT(mMaxTextureSize>0); return mMaxTextureSize; }

We actually backed out from having special getters for this kind of thing, and instead, we want to have everybody go through GLContext::fGetIntegerv and have that do the special-casing. See fGetIntegerv in GLContext.h --- just add a special case for MAX_TEXTURE_SIZE and friends there.

I thought that such special cases already existed -- surprised to see that they're apparently not here.

In this way you won't need to change the existing callers of fGetIntegerv, they'll just work. That's good, because otherwise we'd have a fGetIntegerv that lies to us.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Argh! That's another regression from Bug 716859! That patch took out the existing MAX_TEXTURE_SIZE and other cases from fGetIntegerv. I Did I r+ that? Was there a rationale? +CC jgilbert.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

The consensus is that either we didn't have reasons to remove the special cases in fGetIntegerv, or don't remember what they were.  Either way, the suggested solution is just to back out that part of the change.

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #3)
> Argh! That's another regression from Bug 716859! That patch took out the
> existing MAX_TEXTURE_SIZE and other cases from fGetIntegerv. I Did I r+
> that? Was there a rationale? +CC jgilbert.

Yeah, that shouldn't have been removed. Let's add it back in.

We should also do better checks in OGL layers init code for this sort of thing, but that's beyond the scope of this bug.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: (go back to) overriding fGetIntegerv for max texture sizes

Put back the code to override the fGetIntegerv for the three max texture sizes.  Assert that the values are available, previously having been set in the Init* method.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 721705 [details] [diff] [review]
(go back to) overriding fGetIntegerv for max texture sizes

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

looks good!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc115ba15bb6
https://hg.mozilla.org/integration/mozilla-inbound/rev/376272970f90

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Ryan: bc115ba15bb6 shouldn't have landed, it was r-'d. Can you please back it out?

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

My bad, I should have obsoleted that first patch.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

We don't systematically obsolete r-'d patches. The primary purpose of obsoleting is when we have multiple iterations of the same patch, which isn't really the case here.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bc115ba15bb6
https://hg.mozilla.org/mozilla-central/rev/376272970f90

Comment 13

[Kelsey Gilbert [:jgilbert]]

It looks like we'll need to pull out the r-'d patch.

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Yes - Benoit/Jeff, can you make sure that happens, I'm heading out for > week.

Comment 15

[Kelsey Gilbert [:jgilbert]]

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

Comment 16

[Kelsey Gilbert [:jgilbert]]

Comment 15 is the backout of the r-'d patch.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

The right code is in, the wrong code is out.