Offscreen FBO must not be created for Global Shared context

RESOLVED FIXED in mozilla14

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

Trunk
mozilla14
ARM
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 603458 [details] [diff] [review]
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
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-
(Assignee)

Comment 2

5 years ago
Created attachment 606084 [details] [diff] [review]
Don't create FBO for global context

Hopefully fixed all comments
Assignee: nobody → romaxa
Attachment #603458 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #606084 - Flags: review?(bjacob)
(Assignee)

Comment 3

5 years ago
Created attachment 606087 [details] [diff] [review]
Don't create FBO for global context

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+
(Assignee)

Comment 5

5 years ago
Created attachment 607495 [details] [diff] [review]
Don't create FBO for global context.

Make reference as value
Attachment #606087 - Attachment is obsolete: true
Attachment #607495 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/57d368003eb8
Keywords: checkin-needed
Target Milestone: --- → mozilla14
android and windows builds are burning. backed out.
(Assignee)

Comment 8

5 years ago
Created attachment 607645 [details] [diff] [review]
Don't create FBO for global context.

Oh, missing GLContext:: namespace for ContextFlags... forgot to fix that in attached patch :(
Attachment #607495 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=f471a4703b02 - good try build
https://hg.mozilla.org/mozilla-central/rev/d3b01ea93dab
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 777457
You need to log in before you can comment on or make changes to this bug.