Implement no-gfx-driver-workarounds mode

RESOLVED FIXED in mozilla14

Status

()

Core
Graphics
--
enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

It would be nice to have a (non-default!!) mode where WebGL does not use any work-arounds for driver bugs. One could add a new pref, webgl.use-workarounds, defaulting to true, in

    modules/libpref/src/init/all.js

and then query it during WebGL context initialization, and only use our work-arounds if it's true, for example in WebGLContextGL.cpp we have

    // work around Mac OSX crash, see bug 631420
#ifdef XP_MACOSX
    if (mAttribBuffers[0].enabled &&
        !mCurrentProgram->IsAttribInUse(0))
        return VertexAttrib0Status::EmulatedUninitializedArray;
#endif

and we would only want to do that if this new "use_workarounds" variable is true.
Assignee: nobody → cdiehl
Severity: normal → enhancement
(Assignee)

Comment 1

5 years ago
Repurposing: this should be a general no-gfx-driver-workarounds mode, not specific to WebGL. Indeed, it happens that we move some WebGL workaround upstream to GLContext.
Assignee: cdiehl → nobody
Component: Canvas: WebGL → Graphics
QA Contact: canvas.webgl → thebes
Summary: Implement no-workarounds WebGL mode → Implement no-gfx-driver-workarounds mode
(Assignee)

Comment 2

5 years ago
Also, I am very, very angry about the fact that a certain proprietary OS vendor, who is our main cause of trouble in WebGL, is not caring at all about our bug reports, and I want to have the ability to shame them publicly if they don't start caring soon. So, this is becoming a priority.
(Assignee)

Comment 3

5 years ago
Created attachment 613066 [details] [diff] [review]
Dear XXX, here are all your driver bugs. Love, Mozilla
Attachment #613066 - Flags: review?(jgilbert)
(Assignee)

Comment 4

5 years ago
Hey Bas, the above patch introduces the gfx.work-around-driver-bugs pref but only really implements it in GLContext and WebGLContext. If there are any D3D driver workaround you're doing, you may want to make a follow-up patch here.
(Assignee)

Comment 5

5 years ago
Created attachment 613067 [details]
Dear
(Assignee)

Comment 6

5 years ago
Created attachment 613068 [details] [diff] [review]
Dear XXX, here are all your driver bugs. Love, Mozilla
Attachment #613066 - Attachment is obsolete: true
Attachment #613067 - Attachment is obsolete: true
Attachment #613066 - Flags: review?(jgilbert)
(Assignee)

Updated

5 years ago
Attachment #613068 - Flags: review?(jgilbert)
(Assignee)

Comment 7

5 years ago
Created attachment 613110 [details] [diff] [review]
In no-workarounds mode, also don't work around NPOT FBO bug
Attachment #613110 - Flags: review?(joe)
(Assignee)

Comment 8

5 years ago
Note that the "In no-workarounds mode, also don't work around NPOT FBO bug" patch depends on the "kill USE_GLES2" patch in bug 741730

Comment 9

5 years ago
Comment on attachment 613068 [details] [diff] [review]
Dear XXX, here are all your driver bugs. Love, Mozilla

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

::: gfx/gl/GLContext.cpp
@@ +298,5 @@
>              }
>          }
>      }
>  
> +    mWorkAroundDriverBugs = Preferences::GetBool("gfx.work-around-driver-bugs", true);

With OMTC, we're creating a GL context on the compositor thread. But preferences should only be accessed on the main thread. So I think we need a better way to do this (e.g. we could follow the approach that LayerManagerOGL::Initialize uses to read the FPS counter preference).
Give me one place that's called on the main thread before any GL work is done? LayerManagerOGL isn't such a place, it's not currently used e.g. on Windows and Linux.

Comment 11

5 years ago
(In reply to Benoit Jacob [:bjacob] from comment #10)
> Give me one place that's called on the main thread before any GL work is
> done? LayerManagerOGL isn't such a place, it's not currently used e.g. on
> Windows and Linux.

If we want a single place, nsBaseWidget's constructor should work.

Alternatively, we could do this check in the same places we read the layers acceleration prefs -- in GetLayerMangerPrefs in widget/windows/nsWindow.cpp, and in nsBaseWidget::GetShouldAccelerate.
Created attachment 613132 [details] [diff] [review]
Dear XXX, here are all your driver bugs. Love, Mozilla

The gfxPlatform singleton seemed like a nice home for this boolean, and gfxPlatform initialization like a nice place to read that pref, as other prefs are already being read from there.
Attachment #613068 - Attachment is obsolete: true
Attachment #613068 - Flags: review?(jgilbert)
Attachment #613132 - Flags: review?(jgilbert)
Attachment #613132 - Flags: review?(ajuma)

Comment 13

5 years ago
Comment on attachment 613132 [details] [diff] [review]
Dear XXX, here are all your driver bugs. Love, Mozilla

Looks good to me.
Attachment #613132 - Flags: review?(ajuma) → review+
Created attachment 613137 [details] [diff] [review]
Dear XXX, here are all your driver bugs. Love, Mozilla

oops, forgot to refresh that patch.
Attachment #613132 - Attachment is obsolete: true
Attachment #613132 - Flags: review?(jgilbert)
Attachment #613137 - Flags: review?(jgilbert)
Comment on attachment 613137 [details] [diff] [review]
Dear XXX, here are all your driver bugs. Love, Mozilla

carrying over r+ from ajuma
Attachment #613137 - Flags: review+
https://tbpl.mozilla.org/?tree=Try&rev=7927b837b0dd
Try is green.
Comment on attachment 613137 [details] [diff] [review]
Dear XXX, here are all your driver bugs. Love, Mozilla

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +2647,5 @@
>              // See comment in ValidateProgram below.
> +            if (gl->WorkAroundDriverBugs())
> +                i = 1;
> +            else
> +                gl->fGetProgramiv(progname, pname, &i);

{}

@@ +4517,5 @@
> +        if (gl->WorkAroundDriverBugs()) {
> +            const PRUint32 maxSourceLength = (PRUint32(1)<<18) - 1;
> +            if (sourceCString.Length() > maxSourceLength)
> +                return ErrorInvalidValue("compileShader: source has more than %d characters", 
> +                                         maxSourceLength);

Is it true that GL has no defined limits on shader length?

::: modules/libpref/src/init/all.js
@@ +244,5 @@
>  #ifdef ANDROID
>  pref("gfx.textures.poweroftwo.force-enabled", false);
>  #endif
>  
> +pref("gfx.work-around-driver-bugs", true);

This will need to be added to b2g and mobile as well, AIUI
Attachment #613137 - Flags: review+
Comment on attachment 613110 [details] [diff] [review]
In no-workarounds mode, also don't work around NPOT FBO bug

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

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +324,1 @@
>    mGLContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0);

Should this be inside the if (WorkAroundDriverBugs)?
Attachment #613110 - Flags: review?(joe) → review+
Comment on attachment 613137 [details] [diff] [review]
Dear XXX, here are all your driver bugs. Love, Mozilla

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

R+, since the nits are just a question and a suggestion.

::: gfx/gl/GLContext.h
@@ +1653,5 @@
>      // Useful for resizing offscreen buffers.
>  public:
>      void ClearSafely();
>  
> +    bool WorkAroundDriverBugs() const { return mWorkAroundDriverBugs; }

Since we need to define this for non-protected use, why not use this everywhere? If we ever want to add logic to this, it'll be a pain since most of our uses internal to GLContext use the member var, instead of the function.

@@ -1683,5 @@
>          PRInt32 maxAllowed = NS_MIN(mMaxRenderbufferSize, mMaxTextureSize);
>          return biggerDimension <= maxAllowed;
>      }
>  
> -protected:

Just checking that this was removed because it is redundant.
Attachment #613137 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/469e2e5b18e5
https://hg.mozilla.org/integration/mozilla-inbound/rev/971f7705b978
Assignee: nobody → bjacob
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/469e2e5b18e5
https://hg.mozilla.org/mozilla-central/rev/971f7705b978
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
/home/smaug/mozilla/hg/mozilla/gfx/layers/opengl/LayerManagerOGL.cpp:265:7: error: non-constant-expression cannot be narrowed from type 'int' to
      'GLenum' (aka 'unsigned int') in initializer list
https://hg.mozilla.org/mozilla-central/rev/8eb367e5b053
Sorry I've landed this without looking at the review comments :-(

(In reply to Joe Drew (:JOEDREW!) from comment #19)
> Comment on attachment 613110 [details] [diff] [review]
> In no-workarounds mode, also don't work around NPOT FBO bug
> 
> Review of attachment 613110 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/opengl/LayerManagerOGL.cpp
> @@ +324,1 @@
> >    mGLContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0);
> 
> Should this be inside the if (WorkAroundDriverBugs)?

Apparently not, since the old code ended with an unconditional:

 // back to default framebuffer, to avoid confusion		
 mGLContext->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, 0);

which my patch removed.

(In reply to Jeff Gilbert [:jgilbert] from comment #20)
> Comment on attachment 613137 [details] [diff] [review]
> Dear XXX, here are all your driver bugs. Love, Mozilla
> 
> Review of attachment 613137 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> R+, since the nits are just a question and a suggestion.
> 
> ::: gfx/gl/GLContext.h
> @@ +1653,5 @@
> >      // Useful for resizing offscreen buffers.
> >  public:
> >      void ClearSafely();
> >  
> > +    bool WorkAroundDriverBugs() const { return mWorkAroundDriverBugs; }
> 
> Since we need to define this for non-protected use, why not use this
> everywhere? If we ever want to add logic to this, it'll be a pain since most
> of our uses internal to GLContext use the member var, instead of the
> function.

You're right, I've hesitated a bit but this is better practice. Would make a good follow-up patch.

> 
> @@ -1683,5 @@
> >          PRInt32 maxAllowed = NS_MIN(mMaxRenderbufferSize, mMaxTextureSize);
> >          return biggerDimension <= maxAllowed;
> >      }
> >  
> > -protected:
> 
> Just checking that this was removed because it is redundant.

Yes, it was redundant.
You need to log in before you can comment on or make changes to this bug.