733562 - Offscreen FBO must not be created for Global Shared context

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

Attachment: Don't create FBO for global context

I'm noticed that after recent FBO offscreen fix we start calling ResizeOffscreenFBO for all offscreen contexts including shared one.

Probably some namings in path need to be corrected

Comment 1

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

Comment on attachment 603458 [details] [diff] [review]
Don't create FBO for global context

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

Almost there! let's do a quick second round please.

::: gfx/gl/GLContext.h
@@ +590,5 @@
>      }
>  
> +    enum GLOffscreenContextFlags {
> +        ContextFlagsNone,
> +        ContextFlagsGlobal

I would assign explicit hex values here, to be more clear: 0x0 and 0x1 explicitly, even if in this particular case that doesn't make a difference. Indeed, I think the intent is for this to be a bit-field.

Also, there is a discrepancy in the prefixes here: if GLOffscreen is not needed for the enum values, then it's probably not needed for the enum type itself.

Going one step further: 'Context' is probably redundant with the name of the GLContext class itself, but it's OK: existing enum types already have that problem, so I won't r- just for that.

::: gfx/gl/GLContextProviderCGL.mm
@@ +584,5 @@
>  
>  already_AddRefed<GLContext>
>  GLContextProviderCGL::CreateOffscreen(const gfxIntSize& aSize,
> +                                      const ContextFormat& aFormat,
> +                                      const GLOffscreenContextFlags& flags)

Since you don' use |flags| in this function, don't name it, just pass an unnamed argument. This will avoid 'unused argument' warnings.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +2604,5 @@
>  // often without the ability to texture from them directly.
>  already_AddRefed<GLContext>
>  GLContextProviderEGL::CreateOffscreen(const gfxIntSize& aSize,
> +                                      const ContextFormat& aFormat,
> +                                      const GLOffscreenContextFlags& flags)

Same as for GLContextProviderCGL.mm.

@@ +2617,5 @@
>  
>      if (!glContext)
>          return nsnull;
>  
> +    if (flags != GLContext::ContextFlagsGlobal &&

Let this work like a bitfield please: (flags & ContextFlagsGlobal).

@@ +2618,5 @@
>      if (!glContext)
>          return nsnull;
>  
> +    if (flags != GLContext::ContextFlagsGlobal &&
> +        !glContext->ResizeOffscreenFBO(aSize, true))

this is the crucial call, so I'm wary of 'hiding' it in the 2nd line of a multi-line if() condition. Let it be its own nested if() please.

@@ +2635,1 @@
>          return nsnull;

Same two comments.

@@ +2652,1 @@
>          // we weren't able to create the initial

Same.

::: gfx/gl/GLContextProviderGLX.cpp
@@ +1285,5 @@
>  
>  already_AddRefed<GLContext>
>  GLContextProviderGLX::CreateOffscreen(const gfxIntSize& aSize,
> +                                      const ContextFormat& aFormat,
> +                                      const GLOffscreenContextFlags& flags)

Same as for GLContextProviderCGL.mm.

::: gfx/gl/GLContextProviderOSMesa.cpp
@@ +258,5 @@
>  
>  already_AddRefed<GLContext>
>  GLContextProviderOSMesa::CreateOffscreen(const gfxIntSize& aSize,
> +                                         const ContextFormat& aFormat,
> +                                         const GLOffscreenContextFlags& flags)

Same as for GLContextProviderCGL.mm.

::: gfx/gl/GLContextProviderWGL.cpp
@@ +746,5 @@
>  
>  already_AddRefed<GLContext>
>  GLContextProviderWGL::CreateOffscreen(const gfxIntSize& aSize,
> +                                      const ContextFormat& aFormat,
> +                                      const GLOffscreenContextFlags& flags)

Same as for GLContextProviderCGL.mm.

Comment 2

[Oleg Romashin (:romaxa)]

Attachment: Don't create FBO for global context

Hopefully fixed all comments

Comment 3

[Oleg Romashin (:romaxa)]

Attachment: Don't create FBO for global context

Minor compile fix

Comment 4

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

Comment on attachment 606087 [details] [diff] [review]
Don't create FBO for global context

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

r=me but with the following change please:

::: gfx/gl/GLContextProviderCGL.mm
@@ +590,5 @@
>  
>  already_AddRefed<GLContext>
>  GLContextProviderCGL::CreateOffscreen(const gfxIntSize& aSize,
> +                                      const ContextFormat& aFormat,
> +                                      const ContextFlags& flags)

Passing a plain integer by constant reference is overkill. Please pass by value.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +2647,5 @@
>  // often without the ability to texture from them directly.
>  already_AddRefed<GLContext>
>  GLContextProviderEGL::CreateOffscreen(const gfxIntSize& aSize,
> +                                      const ContextFormat& aFormat,
> +                                      const ContextFlags& aFlags)

Same.

::: gfx/gl/GLContextProviderGLX.cpp
@@ +1291,5 @@
>  
>  already_AddRefed<GLContext>
>  GLContextProviderGLX::CreateOffscreen(const gfxIntSize& aSize,
> +                                      const ContextFormat& aFormat,
> +                                      const ContextFlags&)

Same.

::: gfx/gl/GLContextProviderImpl.h
@@ +91,5 @@
>       */
>      static already_AddRefed<GLContext>
>      CreateOffscreen(const gfxIntSize& aSize,
> +                    const ContextFormat& aFormat = ContextFormat::BasicRGBA32Format,
> +                    const ContextFlags& aFlags = GLContext::ContextFlagsNone);

Same.

::: gfx/gl/GLContextProviderNull.cpp
@@ +46,5 @@
>  
>  already_AddRefed<GLContext>
>  GLContextProviderNull::CreateOffscreen(const gfxIntSize&,
> +                                       const ContextFormat&,
> +                                       const ContextFlags&)

Same

::: gfx/gl/GLContextProviderOSMesa.cpp
@@ +263,1 @@
>  {

Same

::: gfx/gl/GLContextProviderWGL.cpp
@@ +750,5 @@
>  
>  already_AddRefed<GLContext>
>  GLContextProviderWGL::CreateOffscreen(const gfxIntSize& aSize,
> +                                      const ContextFormat& aFormat,
> +                                      const ContextFlags&)

Same

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Don't create FBO for global context.

Make reference as value

Comment 6

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/57d368003eb8

Comment 7

[Dão Gottwald [:dao]]

android and windows builds are burning. backed out.

Comment 8

[Oleg Romashin (:romaxa)]

Attachment: Don't create FBO for global context.

Oh, missing GLContext:: namespace for ContextFlags... forgot to fix that in attached patch :(

Comment 9

[Oleg Romashin (:romaxa)]

https://tbpl.mozilla.org/?tree=Try&rev=f471a4703b02 - good try build

Comment 10

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/d3b01ea93dab