GLContext raw_f functions should not contain function-altering logic

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

Assignee

Description

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

Updated

7 years ago
Depends on: 788408
Totally agree; I noticed this when I was looking at merging in your changes locally.
Assignee

Comment 3

7 years ago
Sorry for the noise. Pulled in some that I saw we were missing.
Attachment #686813 - Attachment is obsolete: true
Attachment #686813 - Flags: review?(vladimir)
Attachment #686836 - Flags: review?(vladimir)
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)
Attachment #686836 - Flags: review?(vladimir) → review+
Assignee

Comment 5

7 years ago
(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 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.
Attachment #691717 - Flags: review?(bjacob) → review-
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.
Attachment #691717 - Flags: review- → review+
Assignee

Comment 9

7 years ago
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 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).
Attachment #693134 - Flags: review?(bjacob) → review-
Comment on attachment 693135 [details] [diff] [review]
patch 3: Don't warn if optional GLContext symbols are missing except with MOZ_GL_DEBUG.

Yay!
Attachment #693135 - Flags: review?(vladimir) → review+
Assignee

Comment 14

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

Comment 15

7 years ago
Checking against MOZ_FUNCTION_NAME to be sure we're referring to the right function.
Attachment #693134 - Attachment is obsolete: true
Attachment #693204 - Flags: review?(bjacob)
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.
Attachment #693204 - Flags: review?(bjacob) → review+
Assignee

Updated

7 years ago
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/1d72507cbd59
https://hg.mozilla.org/mozilla-central/rev/83ac1379e10e
https://hg.mozilla.org/mozilla-central/rev/3ef80b6f2622
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20

Updated

7 years ago
Depends on: 825995
Depends on: 826570
Assignee

Updated

6 years ago
Duplicate of this bug: 776787
You need to log in before you can comment on or make changes to this bug.