Limit number of GL contexts with SkiaGL

RESOLVED FIXED in mozilla25

Status

()

Core
Canvas: 2D
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

unspecified
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Currently, each <canvas> has its own associated GLContext. Because of this, it's very easy to run out of these or at least use a ton of memory. We should limit this to some sane value, and demote older canvases to software once the limit is reached. The test_canvas mochitest is a great example of how things can go bad here.
Created attachment 768437 [details] [diff] [review]
Limit number of GLContexts used with SkiaGL
Attachment #768437 - Flags: review?(bjacob)
Blocks: 887927
Comment on attachment 768437 [details] [diff] [review]
Limit number of GLContexts used with SkiaGL

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

r=me, but with a suggestion to make it nicer:

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +106,5 @@
>  #include "SurfaceTypes.h"
>  using mozilla::gl::GLContext;
>  using mozilla::gl::GLContextProvider;
> +
> +static nsTArray<mozilla::dom::CanvasRenderingContext2D*>* sDemotableContexts = nullptr;

I understand that you had to make this a pointer to avoid having a static constructor. And then you have to new' this nsTArray the first time it's used. But you have a better way to achieve this. The trick is that static local variables at function scope are only constructed when first called, so you as long as you have static local variables (as opposed to globals) you need not worry with that problem at all. So do this:

static nsTArray<mozilla::dom::CanvasRenderingContext2D*>& DemotableContexts() {
  static nsTArray<mozilla::dom::CanvasRenderingContext2D*> value;
  return value;
}

::: gfx/skia/src/gpu/gl/GrGLCaps.h
@@ +183,5 @@
>  
>      /// Is GL_BGRA supported
>      bool bgraFormatSupport() const { return fBGRAFormatSupport; }
>  
> +    /// Is GL_RGBA supported with glReadPixels()

Did you actually mean to add a comment to this skia file? Won't this just make rebasing harder?
Attachment #768437 - Flags: review?(bjacob) → review+
(In reply to Benoit Jacob [:bjacob] from comment #2)
> Comment on attachment 768437 [details] [diff] [review]
> Limit number of GLContexts used with SkiaGL
> 
> Review of attachment 768437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, but with a suggestion to make it nicer:
> 
> ::: content/canvas/src/CanvasRenderingContext2D.cpp
> @@ +106,5 @@
> >  #include "SurfaceTypes.h"
> >  using mozilla::gl::GLContext;
> >  using mozilla::gl::GLContextProvider;
> > +
> > +static nsTArray<mozilla::dom::CanvasRenderingContext2D*>* sDemotableContexts = nullptr;
> 
> I understand that you had to make this a pointer to avoid having a static
> constructor. And then you have to new' this nsTArray the first time it's
> used. But you have a better way to achieve this. The trick is that static
> local variables at function scope are only constructed when first called, so
> you as long as you have static local variables (as opposed to globals) you
> need not worry with that problem at all. So do this:
> 
> static nsTArray<mozilla::dom::CanvasRenderingContext2D*>&
> DemotableContexts() {
>   static nsTArray<mozilla::dom::CanvasRenderingContext2D*> value;
>   return value;
> }

That's a nice trick. Won't that trigger the leak checker on shutdown though?

> 
> ::: gfx/skia/src/gpu/gl/GrGLCaps.h
> @@ +183,5 @@
> >  
> >      /// Is GL_BGRA supported
> >      bool bgraFormatSupport() const { return fBGRAFormatSupport; }
> >  
> > +    /// Is GL_RGBA supported with glReadPixels()
> 
> Did you actually mean to add a comment to this skia file? Won't this just
> make rebasing harder?

Oops that's stuff from another patch. Fixed.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #3)
> (In reply to Benoit Jacob [:bjacob] from comment #2)
> > Comment on attachment 768437 [details] [diff] [review]
> > Limit number of GLContexts used with SkiaGL
> > 
> > Review of attachment 768437 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me, but with a suggestion to make it nicer:
> > 
> > ::: content/canvas/src/CanvasRenderingContext2D.cpp
> > @@ +106,5 @@
> > >  #include "SurfaceTypes.h"
> > >  using mozilla::gl::GLContext;
> > >  using mozilla::gl::GLContextProvider;
> > > +
> > > +static nsTArray<mozilla::dom::CanvasRenderingContext2D*>* sDemotableContexts = nullptr;
> > 
> > I understand that you had to make this a pointer to avoid having a static
> > constructor. And then you have to new' this nsTArray the first time it's
> > used. But you have a better way to achieve this. The trick is that static
> > local variables at function scope are only constructed when first called, so
> > you as long as you have static local variables (as opposed to globals) you
> > need not worry with that problem at all. So do this:
> > 
> > static nsTArray<mozilla::dom::CanvasRenderingContext2D*>&
> > DemotableContexts() {
> >   static nsTArray<mozilla::dom::CanvasRenderingContext2D*> value;
> >   return value;
> > }
> 
> That's a nice trick. Won't that trigger the leak checker on shutdown though?

This is just a nsTArray of POD's (pointers). Are our leak tools guarding these, or are they specifically about refcounted objects?
Note: if our leak tools are annoying about that, you could use a plain static array given that the size is known at compile time, or you could use a std::vector if runtime-sizing matters.
Created attachment 768711 [details] [diff] [review]
Limit number of GLContexts used with SkiaGL
Attachment #768711 - Flags: review?(bjacob)
Attachment #768437 - Attachment is obsolete: true
Comment on attachment 768711 [details] [diff] [review]
Limit number of GLContexts used with SkiaGL

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

If our leak tools give you trouble, just use something else than nsTArray as discussed above.
Attachment #768711 - Flags: review?(bjacob) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2315e78bd4e2
https://hg.mozilla.org/mozilla-central/rev/2315e78bd4e2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Updated

11 months ago
See Also: → bug 1335145
You need to log in before you can comment on or make changes to this bug.