797664 - GLContext raw_f functions should not contain function-altering logic

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Kelsey Gilbert [:jgilbert]]

Evidently, we are currently using FixYValue to (potentially) change the y value passed in to a number of raw_f GL functions. These are needed in a limited set of cases, and should really be handled at a higher level than this.

These should probably be split off into separate functions that indicate they are mFlipped-aware.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Totally agree; I noticed this when I was looking at merging in your changes locally.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: pull logic out of raw_f* funcs and make them private. Assert that we're not both offscreen and flipped.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: pull logic out of raw_f* funcs and make them private. Assert that we're not both offscreen and flipped.

Sorry for the noise. Pulled in some that I saw we were missing.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 686836 [details] [diff] [review]
patch 1: pull logic out of raw_f* funcs and make them private. Assert that we're not both offscreen and flipped.

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

Looks fine

::: gfx/gl/GLContext.h
@@ +2163,5 @@
>      }
>  
>      void fGetTexImage(GLenum target, GLint level, GLenum format, GLenum type, GLvoid *img) {
>          if (!mSymbols.fGetTexImage) {
> +            return;

Huh -- shouldn't this and GetTexLevelParameteriv be asserts or something?  Not really an issue for this patch, just blindly ignoring them seems odd.  (Or assert !mIsGLES2, like for DepthRange)

Comment 5

[Kelsey Gilbert [:jgilbert]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #4)
> Comment on attachment 686836 [details] [diff] [review]
> patch 1: pull logic out of raw_f* funcs and make them private. Assert that
> we're not both offscreen and flipped.
> 
> Review of attachment 686836 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine
> 
> ::: gfx/gl/GLContext.h
> @@ +2163,5 @@
> >      }
> >  
> >      void fGetTexImage(GLenum target, GLint level, GLenum format, GLenum type, GLvoid *img) {
> >          if (!mSymbols.fGetTexImage) {
> > +            return;
> 
> Huh -- shouldn't this and GetTexLevelParameteriv be asserts or something? 
> Not really an issue for this patch, just blindly ignoring them seems odd. 
> (Or assert !mIsGLES2, like for DepthRange)

At some point I would like to always assert that we're not calling a null function. For now, let's just assert for these edge-case functions, where it's only available in one of Desktop GL or GLES.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Add asserts so that it's more obvious when we call a function which doesn't work in GLContext.

Asserts!

Comment 7

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

Comment on attachment 691717 [details] [diff] [review]
patch 2: Add asserts so that it's more obvious when we call a function which doesn't work in GLContext.

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

::: gfx/gl/GLContext.h
@@ +2142,2 @@
>          BEFORE_GL_CALL;
> +        MOZ_ASSERT(mSymbols.fGetTexImage);

This assertion should crash also release builds.

Comment 8

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

Comment on attachment 691717 [details] [diff] [review]
patch 2: Add asserts so that it's more obvious when we call a function which doesn't work in GLContext.

I guess that this is OK after all. In case of a null pointer, we would crash cleanly enough that it's not worth the hassle to ensure that consistently runtime-abort in all such cases.

Comment 9

[Kelsey Gilbert [:jgilbert]]

If we know why we're crashing, there's no reason to require devs to debug and have to figure it out again later.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Add runtime asserts so that it's more obvious when we call a function which doesn't work in GLContext.

Comment 11

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Don't warn if optional GLContext symbols are missing except with MOZ_GL_DEBUG.

Comment 12

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

Comment on attachment 693134 [details] [diff] [review]
patch 2: Add runtime asserts so that it's more obvious when we call a function which doesn't work in GLContext.

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

::: gfx/gl/GLContext.h
@@ +1726,5 @@
>  #define AFTER_GL_CALL do { } while (0)
>  
>  #endif
>  
> +#define ASSERT_SYMBOL_PRESENT(func) \

I don't like that this forces the caller to pass the right |func| argument. The reason why assert macros are macros is so they can determine this automatically using __FUNC__ or compiler-speficic refinements such as __PRETTY_FUNCTION__; we most certainly already have a portable macro doing the right thing; try looking for MOZ_FUNCTION_NAME (maybe even in this very file).

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 693135 [details] [diff] [review]
patch 3: Don't warn if optional GLContext symbols are missing except with MOZ_GL_DEBUG.

Yay!

Comment 14

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #12)
> Comment on attachment 693134 [details] [diff] [review]
> patch 2: Add runtime asserts so that it's more obvious when we call a
> function which doesn't work in GLContext.
> 
> Review of attachment 693134 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.h
> @@ +1726,5 @@
> >  #define AFTER_GL_CALL do { } while (0)
> >  
> >  #endif
> >  
> > +#define ASSERT_SYMBOL_PRESENT(func) \
> 
> I don't like that this forces the caller to pass the right |func| argument.
> The reason why assert macros are macros is so they can determine this
> automatically using __FUNC__ or compiler-speficic refinements such as
> __PRETTY_FUNCTION__; we most certainly already have a portable macro doing
> the right thing; try looking for MOZ_FUNCTION_NAME (maybe even in this very
> file).

Oh, I see what you mean. We could even have it check that __FUNC__ or whatever matches the function name we pass in. (DEBUG builds only, of course)

Comment 15

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Add runtime asserts so that it's more obvious when we call a function which doesn't work in GLContext.

Checking against MOZ_FUNCTION_NAME to be sure we're referring to the right function.

Comment 16

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

Comment on attachment 693204 [details] [diff] [review]
patch 2: Add runtime asserts so that it's more obvious when we call a function which doesn't work in GLContext.

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

Oh, right. Sorry. I had missed that you needed to get |func| passed to you so you could actually do the symbol check. Sorry, and yes this solution looks good.

Comment 17

[Kelsey Gilbert [:jgilbert]]

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d72507cbd59
https://hg.mozilla.org/integration/mozilla-inbound/rev/83ac1379e10e
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef80b6f2622

Try:
https://tbpl.mozilla.org/?tree=Try&rev=f19f0c774528

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1d72507cbd59
https://hg.mozilla.org/mozilla-central/rev/83ac1379e10e
https://hg.mozilla.org/mozilla-central/rev/3ef80b6f2622