Cleanup GLContext ResizeOffscreenFBO

RESOLVED FIXED in mozilla15

Status

()

Core
Canvas: WebGL
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
Attachment #609872 - Flags: feedback?(bjacob)
(Assignee)

Comment 2

6 years ago
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 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?
Attachment #609872 - Flags: feedback?(bjacob) → feedback+
(Assignee)

Comment 4

5 years ago
(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.
Assignee: nobody → jgilbert
Status: NEW → ASSIGNED
(Assignee)

Comment 5

5 years ago
Created attachment 613475 [details] [diff] [review]
Refactor ResizeOffscreenFBO

Let's actually review this for landing.
Attachment #609872 - Attachment is obsolete: true
Attachment #613475 - Flags: review?(bjacob)
(Assignee)

Comment 6

5 years ago
Created attachment 613808 [details] [diff] [review]
Refactor ResizeOffscreenFBO

This one actually compiles and runs. ><
Attachment #613475 - Attachment is obsolete: true
Attachment #613475 - Flags: review?(bjacob)
Attachment #613808 - Flags: review?(bjacob)
(Assignee)

Updated

5 years ago
Blocks: 729726
(Assignee)

Comment 7

5 years ago
Try is clean:
https://tbpl.mozilla.org/?tree=Try&rev=da9be9ca3f7f
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.
Attachment #613808 - Flags: review?(bjacob) → review+
(Assignee)

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
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.
Attachment #615544 - Flags: review?(bjacob)
Attachment #615544 - Flags: review?(bjacob) → review+
(Assignee)

Updated

5 years ago
Attachment #615544 - Flags: checkin?(jgilbert)
(Assignee)

Updated

5 years ago
Attachment #613808 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #615544 - Flags: checkin?(jgilbert) → checkin+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/832debbbdedb
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/832debbbdedb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.