Last Comment Bug 733562 - Offscreen FBO must not be created for Global Shared context
: Offscreen FBO must not be created for Global Shared context
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Linux
: -- normal (vote)
: mozilla14
Assigned To: Oleg Romashin (:romaxa)
:
Mentors:
Depends on:
Blocks: 777457
  Show dependency treegraph
 
Reported: 2012-03-06 14:07 PST by Oleg Romashin (:romaxa)
Modified: 2012-07-27 14:10 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't create FBO for global context (9.80 KB, patch)
2012-03-06 14:07 PST, Oleg Romashin (:romaxa)
jacob.benoit.1: review-
Details | Diff | Review
Don't create FBO for global context (9.87 KB, patch)
2012-03-14 19:52 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review
Don't create FBO for global context (9.89 KB, patch)
2012-03-14 19:58 PDT, Oleg Romashin (:romaxa)
jacob.benoit.1: review+
Details | Diff | Review
Don't create FBO for global context. (9.88 KB, patch)
2012-03-20 02:53 PDT, Oleg Romashin (:romaxa)
romaxa: review+
Details | Diff | Review
Don't create FBO for global context. (9.91 KB, patch)
2012-03-20 11:47 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review

Description Oleg Romashin (:romaxa) 2012-03-06 14:07:48 PST
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
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2012-03-07 12:45:36 PST
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) 2012-03-14 19:52:32 PDT
Created attachment 606084 [details] [diff] [review]
Don't create FBO for global context

Hopefully fixed all comments
Comment 3 Oleg Romashin (:romaxa) 2012-03-14 19:58:03 PDT
Created attachment 606087 [details] [diff] [review]
Don't create FBO for global context

Minor compile fix
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2012-03-20 02:48:43 PDT
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) 2012-03-20 02:53:23 PDT
Created attachment 607495 [details] [diff] [review]
Don't create FBO for global context.

Make reference as value
Comment 7 Dão Gottwald [:dao] 2012-03-20 05:14:12 PDT
android and windows builds are burning. backed out.
Comment 8 Oleg Romashin (:romaxa) 2012-03-20 11:47:54 PDT
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 :(
Comment 9 Oleg Romashin (:romaxa) 2012-03-20 15:48:38 PDT
https://tbpl.mozilla.org/?tree=Try&rev=f471a4703b02 - good try build
Comment 10 Marco Bonardo [::mak] 2012-03-22 06:40:40 PDT
https://hg.mozilla.org/mozilla-central/rev/d3b01ea93dab

Note You need to log in before you can comment on or make changes to this bug.