929511 - use context sharing for WGL pbuffer surfaces

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

Attachment: turn on pbuffer sharing

I'm probably the only one who ever runs this code, but I need shared contexts here for SkiaGL and stuff.  This seems just an omission, so it's a fix.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 820372 [details] [diff] [review]
turn on pbuffer sharing

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

I don't fill qualified to review this.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 820372 [details] [diff] [review]
turn on pbuffer sharing

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

::: gfx/gl/GLContextProviderWGL.cpp
@@ +585,5 @@
>          context = wgl.fCreateContext(pbdc);
> +        if (context && shareContext &&
> +            !sWGLLib[aLibToUse].fShareLists(shareContext->Context(), context))
> +        {
> +            NS_WARNING("wglShareLists failed for pbuffer!");

'...for pbuffer-based context', right?

Comment 3

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

Attachment: better fix

This is a better fix; there was lots of broken stuff in here for window contexts as well.  As a bonus, this also enables CodeXL/gDEBugger support.  (Of course, CodeXL has a bug that makes it mostly useless at the moment, but the thought counts.)

(asking for review from one of you guys -- I don't think GLContextProviderWGL is ever used in normal usage, really)

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8344712 [details] [diff] [review]
better fix

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

Shouldn't we support a failure to create a shared context, and try creating a non-shared context?

::: gfx/gl/GLContextProviderWGL.cpp
@@ +122,2 @@
>      if (!mOGLLibrary) {
> +        mOGLLibrary = PR_LoadLibrary(&libGLFilename[0]);

Use c_str().

@@ +482,5 @@
>          if (context &&
>              shareContext &&
>              !sWGLLib[libToUse].fShareLists(shareContext->Context(), context))
>          {
> +            printf_stderr("WGL context creation failed for window: wglShareLists returned false!");

NS_ERROR?

@@ +511,5 @@
>  
>  static already_AddRefed<GLContextWGL>
>  CreatePBufferOffscreenContext(const gfxIntSize& aSize,
> +                              LibType aLibToUse,
> +                              GLContextWGL *aShareContext)

Does this return null if it can't create a shared context? (That's my vote)

@@ +573,4 @@
>              0
>          };
>  
> +        context = wgl.fCreateContextAttribs(pbdc, aShareContext->Context(), attribs);

Either allow for shareContext being null, or assert that it's non-null.

@@ +579,5 @@
> +        if (context && aShareContext) {
> +            if (!wgl.fShareLists(aShareContext->Context(), context)) {
> +                wgl.fDeleteContext(context);
> +                context = nullptr;
> +                printf_stderr("ERROR - creating pbuffer context failed because wglShareLists returned FALSE");

Shouldn't this be NS_ERROR?

@@ +673,4 @@
>          sWGLLib[libToUse].fChoosePixelFormat)
>      {
>          gfxIntSize dummySize = gfxIntSize(16, 16);
> +        glContext = CreatePBufferOffscreenContext(dummySize, libToUse, GetGlobalContextWGL());

We shouldn't entirely fail to create a context here just because context-sharing failed.

@@ +677,3 @@
>      }
>  
>      // If it failed, then create a window context and use a FBO.

Hah, this is *really* old. I should clean this up. We should probably only support one of these.

Comment 5

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

(In reply to Jeff Gilbert [:jgilbert] from comment #4)
> Comment on attachment 8344712 [details] [diff] [review]
> better fix
> 
> Review of attachment 8344712 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Shouldn't we support a failure to create a shared context, and try creating
> a non-shared context?

No, because if you need a shared context, you *really* need a shared context.  Falling back to non-shared doesn't make sense.  I talked with bjacob briefly about adding a ContextFlag to request one or the other.

> ::: gfx/gl/GLContextProviderWGL.cpp
> @@ +122,2 @@
> >      if (!mOGLLibrary) {
> > +        mOGLLibrary = PR_LoadLibrary(&libGLFilename[0]);
> 
> Use c_str().
> 
> @@ +482,5 @@
> >          if (context &&
> >              shareContext &&
> >              !sWGLLib[libToUse].fShareLists(shareContext->Context(), context))
> >          {
> > +            printf_stderr("WGL context creation failed for window: wglShareLists returned false!");
> 
> NS_ERROR?

Doesn't NS_ERROR abort?  I could be totally wrong here.  NS_WARN at least, I guess.

> >  static already_AddRefed<GLContextWGL>
> >  CreatePBufferOffscreenContext(const gfxIntSize& aSize,
> > +                              LibType aLibToUse,
> > +                              GLContextWGL *aShareContext)
> 
> Does this return null if it can't create a shared context? (That's my vote)

Only if one was specified and it can't share.

> 
> @@ +573,4 @@
> >              0
> >          };
> >  
> > +        context = wgl.fCreateContextAttribs(pbdc, aShareContext->Context(), attribs);
> 
> Either allow for shareContext being null, or assert that it's non-null.

Hm, I thought this branch was only taken if we had a share context; maybe not, I'll look at the code again tomorrow.  It should definitely allow for null.

> >      // If it failed, then create a window context and use a FBO.
> 
> Hah, this is *really* old. I should clean this up. We should probably only
> support one of these.

Ehhh. It doesn't really matter much, though yeah, if you can't support a pbuffer context it's unlikely your GL is useful enough for anything.  We can nuke this path.

Comment 6

[Kelsey Gilbert [:jgilbert]]

It's possible to still want a context even if we can't do resource sharing. Readback is slow, but it may still be better than nothing.

Comment 7

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

Sure; but the code can make that decision (either try to get a second context or punt).  Note that this work isn't for WebGL; it's for SkiaGL in content.  I'm just fixing up the windows stuff since that's where I was testing SkiaGL stuff :p

Comment 8

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

For what it's worth, in the case of "performance cliff" fallbacks such as from using a share group to using readback, I would rather have the fallback logic explicit in the application code, than implicit in the library code. So, I would vote in favor of just returning a nullptr if a share group was requested and can't be obtained.

Comment 9

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

I'm going to land this as-is and then rework it a bit afterwards; mainly to get it out of my patch queue (again, it won't really affect anyone since we never use WGL for anything by default).

Comment 10

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

 https://hg.mozilla.org/integration/mozilla-inbound/rev/8048271b130d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8048271b130d