Closed
Bug 733562
Opened 12 years ago
Closed 12 years ago
Offscreen FBO must not be created for Global Shared context
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: romaxa, Assigned: romaxa)
References
Details
Attachments
(1 file, 4 obsolete files)
9.91 KB,
patch
|
Details | Diff | Splinter Review |
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 1•12 years ago
|
||
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•12 years ago
|
||
Hopefully fixed all comments
Assignee: nobody → romaxa
Attachment #603458 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #606084 -
Flags: review?(bjacob)
Assignee | ||
Comment 3•12 years ago
|
||
Minor compile fix
Attachment #606084 -
Attachment is obsolete: true
Attachment #606087 -
Flags: review?(bjacob)
Attachment #606084 -
Flags: review?(bjacob)
Comment 4•12 years ago
|
||
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•12 years ago
|
||
Make reference as value
Attachment #606087 -
Attachment is obsolete: true
Attachment #607495 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 6•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/57d368003eb8
Keywords: checkin-needed
Target Milestone: --- → mozilla14
Comment 7•12 years ago
|
||
android and windows builds are burning. backed out.
Assignee | ||
Comment 8•12 years ago
|
||
Oh, missing GLContext:: namespace for ContextFlags... forgot to fix that in attached patch :(
Attachment #607495 -
Attachment is obsolete: true
Assignee | ||
Comment 9•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f471a4703b02 - good try build
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d3b01ea93dab
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•