Closed Bug 848023 Opened 9 years ago Closed 9 years ago

max texture size inconsistency

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: milan, Assigned: milan)

References

Details

Attachments

(2 files)

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 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.
Attachment #721343 - Flags: review?(bjacob) → review-
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.
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.
(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.
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.
Attachment #721705 - Flags: review?(bjacob)
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!
Attachment #721705 - Flags: review?(bjacob) → review+
Ryan: bc115ba15bb6 shouldn't have landed, it was r-'d. Can you please back it out?
My bad, I should have obsoleted that first patch.
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.
https://hg.mozilla.org/mozilla-central/rev/bc115ba15bb6
https://hg.mozilla.org/mozilla-central/rev/376272970f90
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
It looks like we'll need to pull out the r-'d patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Yes - Benoit/Jeff, can you make sure that happens, I'm heading out for > week.
Comment 15 is the backout of the r-'d patch.
The right code is in, the wrong code is out.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.