Closed Bug 733562 Opened 8 years ago Closed 8 years ago

Offscreen FBO must not be created for Global Shared context

Categories

(Core :: Graphics, defect)

ARM
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: romaxa, Assigned: romaxa)

References

Details

Attachments

(1 file, 4 obsolete files)

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
Attachment #603458 - Flags: review?(bjacob)
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.
Attachment #603458 - Flags: review?(bjacob) → review-
Hopefully fixed all comments
Assignee: nobody → romaxa
Attachment #603458 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #606084 - Flags: review?(bjacob)
Minor compile fix
Attachment #606084 - Attachment is obsolete: true
Attachment #606087 - Flags: review?(bjacob)
Attachment #606084 - Flags: review?(bjacob)
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
Attachment #606087 - Flags: review?(bjacob) → review+
Make reference as value
Attachment #606087 - Attachment is obsolete: true
Attachment #607495 - Flags: review+
Keywords: checkin-needed
android and windows builds are burning. backed out.
Oh, missing GLContext:: namespace for ContextFlags... forgot to fix that in attached patch :(
Attachment #607495 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d3b01ea93dab
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.