Last Comment Bug 739775 - Cleanup GLContext ResizeOffscreenFBO
: Cleanup GLContext ResizeOffscreenFBO
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Canvas: WebGL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jeff Gilbert [:jgilbert]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 729726
  Show dependency treegraph
 
Reported: 2012-03-27 13:45 PDT by Jeff Gilbert [:jgilbert]
Modified: 2012-04-25 20:37 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Cleanup ResizeOffscreenFBO (33.09 KB, patch)
2012-03-27 13:48 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: feedback+
Details | Diff | Splinter Review
Refactor ResizeOffscreenFBO (33.14 KB, patch)
2012-04-09 19:45 PDT, Jeff Gilbert [:jgilbert]
no flags Details | Diff | Splinter Review
Refactor ResizeOffscreenFBO (35.75 KB, patch)
2012-04-10 15:58 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
Details | Diff | Splinter Review
Better Refactor ResizeOffscreenFBO (35.87 KB, patch)
2012-04-16 17:09 PDT, Jeff Gilbert [:jgilbert]
jacob.benoit.1: review+
jgilbert: checkin+
Details | Diff | Splinter Review

Description Jeff Gilbert [:jgilbert] 2012-03-27 13:45:20 PDT
Cleaning up this mountain of a function is useful for a number of reasons. We should definitely split out choosing our formats from actually using them.

I am looking at a method of multi-buffering based on switching out the color attachment on our backing FBO, so an easy way to skip straight to buffer attachment/assembly would also be a plus.
Comment 1 Jeff Gilbert [:jgilbert] 2012-03-27 13:48:09 PDT
Created attachment 609872 [details] [diff] [review]
Cleanup ResizeOffscreenFBO

This patch splits everything out into sections. We also have a few other changes:
We no longer specify the RGBA4/RGBA8 for our color renderbuffer (just use RGBA)
We no longer ask for RGB565 anywhere.
GLES no longer tries to use 32-bit depth buffers when available.
Comment 2 Jeff Gilbert [:jgilbert] 2012-03-27 13:49:25 PDT
Derp, it also looks like I didn't propagate the naming change from DeleteOffscreenFBO to DeleteOffscreenFBOs. I'm only looking for feedback on the structure of the change, really, though.
Comment 3 Benoit Jacob [:bjacob] (mostly away) 2012-04-07 08:20:40 PDT
Comment on attachment 609872 [details] [diff] [review]
Cleanup ResizeOffscreenFBO

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

I like where this is going. Some nits about naming. Please add some comments to explain what each function is doing, it will definitely help future readers.

::: gfx/gl/GLContext.cpp
@@ +1222,5 @@
> +        }
> +    }
> +
> +    return formats;
> +}

I like that this seems to be splitting the existing kludge into functions that do one thing each.

@@ +1251,5 @@
> +    fBindTexture(LOCAL_GL_TEXTURE_2D, boundTexture);
> +}
> +
> +static inline void
> +RenderbufferStorageHelper(const GLContext* gl, GLsizei samples, GLenum internalFormat, const gfxIntSize& size)

Hm, please find a better name. This is like "hamburger helper" pasta.

::: gfx/gl/GLContext.h
@@ +1637,5 @@
>          NS_WARNING("ResizeOffscreenFBO failed to resize AA context even without AA!");
>          return false;
>      }
>  
> +    struct GLFormats {

Why the 's' here?
Comment 4 Jeff Gilbert [:jgilbert] 2012-04-09 17:18:44 PDT
(In reply to Benoit Jacob [:bjacob] from comment #3)
> Comment on attachment 609872 [details] [diff] [review]
> Cleanup ResizeOffscreenFBO
> 
> Review of attachment 609872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like where this is going. Some nits about naming. Please add some comments
> to explain what each function is doing, it will definitely help future
> readers.
> 
> ::: gfx/gl/GLContext.cpp
> @@ +1222,5 @@
> > +        }
> > +    }
> > +
> > +    return formats;
> > +}
> 
> I like that this seems to be splitting the existing kludge into functions
> that do one thing each.
> 
> @@ +1251,5 @@
> > +    fBindTexture(LOCAL_GL_TEXTURE_2D, boundTexture);
> > +}
> > +
> > +static inline void
> > +RenderbufferStorageHelper(const GLContext* gl, GLsizei samples, GLenum internalFormat, const gfxIntSize& size)
> 
> Hm, please find a better name. This is like "hamburger helper" pasta.

'RenderbufferStorageBySamples'?

> 
> ::: gfx/gl/GLContext.h
> @@ +1637,5 @@
> >          NS_WARNING("ResizeOffscreenFBO failed to resize AA context even without AA!");
> >          return false;
> >      }
> >  
> > +    struct GLFormats {
> 
> Why the 's' here?

The struct contains the the formats for color, stencil, and depth buffers, so I thought plural would be appropriate. The idea is that we're picking all of our GL formatS before-hand.
Comment 5 Jeff Gilbert [:jgilbert] 2012-04-09 19:45:13 PDT
Created attachment 613475 [details] [diff] [review]
Refactor ResizeOffscreenFBO

Let's actually review this for landing.
Comment 6 Jeff Gilbert [:jgilbert] 2012-04-10 15:58:20 PDT
Created attachment 613808 [details] [diff] [review]
Refactor ResizeOffscreenFBO

This one actually compiles and runs. ><
Comment 7 Jeff Gilbert [:jgilbert] 2012-04-11 21:58:13 PDT
Try is clean:
https://tbpl.mozilla.org/?tree=Try&rev=da9be9ca3f7f
Comment 8 Benoit Jacob [:bjacob] (mostly away) 2012-04-16 15:50:19 PDT
Comment on attachment 613808 [details] [diff] [review]
Refactor ResizeOffscreenFBO

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

::: gfx/gl/GLContext.cpp
@@ +1516,2 @@
>          printf_stderr("Framebuffer status: %X\n", status);
> +  #endif

Why can't this printf_stderr be a plain NS_WARNING without the #ifdef?

@@ +1572,3 @@
>  
>      // Replace with the new hotness
> +    std::swap(mOffscreenDrawFBO, drawFBO);

Hah, so that's what you use <algorithm> for.
Comment 9 Jeff Gilbert [:jgilbert] 2012-04-16 16:52:21 PDT
(In reply to Benoit Jacob [:bjacob] from comment #8)
> Comment on attachment 613808 [details] [diff] [review]
> Refactor ResizeOffscreenFBO
> 
> Review of attachment 613808 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLContext.cpp
> @@ +1516,2 @@
> >          printf_stderr("Framebuffer status: %X\n", status);
> > +  #endif
> 
> Why can't this printf_stderr be a plain NS_WARNING without the #ifdef?

NS_WARNING doesn't support printf-like formatting. If we use enough code to append together a string, it'll be big enough to warrant putting in a #if block for clarity's sake. We would also be relying on the unused path being optimized out.
Comment 10 Jeff Gilbert [:jgilbert] 2012-04-16 17:09:35 PDT
Created attachment 615544 [details] [diff] [review]
Better Refactor ResizeOffscreenFBO

Actually, it looks like I'm the worst, and that patch was slightly old.
New try:
https://tbpl.mozilla.org/?tree=Try&rev=0fa641b899c1

Changes:
Early out in GLContext.cpp 3-arg GLContext::ResizeOffscreenFBOs.
Simplified AA-failure fallback logic in GLContext.h 2-arg GLContext::ResizeOffscreenFBOs.
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-25 20:37:34 PDT
https://hg.mozilla.org/mozilla-central/rev/832debbbdedb

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