Mac h264: Plug GL program leak

Mac h264: Plug GL program leak

The state for YUV to RGB conversion for GLImageIOSurface is stored
in the GLImageIOSurface object. This is objectionable because
- This means every GLImage object will re-create all shaders
- For hardware decode, we create a new GLImage for every decoded frame,
  which means allocating a shader for every frame.
- It is not clear which CGLContextObj the GL objects are associated
  with.
- For hardware decode, we do not specify that a GL context is current
  when we free GLImages, so we end up leaking one GL program every
  frame.
- This memory leak ends up causing GL context destroy to take more than
  ten seconds, causing a GPU hang when the GL context is destroyed.

Move the program object into an RGBConverter class. Keep a global
registry of RGBConverter classes for each CGLContextObj, and share
RGBConverter objects across GLImages. Make the RGBConverter always
destroy its objects when it is destroyed. It can do this because it
retains the CGLContextObj that was used to allocate them.

This could be better in that a pathological user of GLImages could
still cause the RGBConverter to be created and destroyed every frame.

BUG=598388, 599314
Committed: https://crrev.com/c9295d4075c1e4a32977c446ab9734a88abf8391
Cr-Commit-Position: refs/heads/master@{#386757}

[ccameron, 2016-04-09 00:37:12]

Description was changed from

==========
Mac h264: Plug GL program leak

BUG=598388,599314
==========

to

==========
Mac h264: Plug GL program leak

The state for YUV to RGB conversion for GLImageIOSurface is stored
in the GLImageIOSurface object. This is objectionable because
- This means every GLImage object will re-create all shaders
- For hardware decode, we create a new GLImage for every decoded frame,
  which means allocating a shader for every frame.
- It is not clear which CGLContextObj the GL objects are associated
  with.
- For hardware decode, we do not specify that a GL context is current
  when we free GLImages, so we end up leaking one GL program every
  frame.
- This memory leak ends up causing GL context destroy to take more than
  ten seconds, causing a GPU hang when the GL context is destroyed.

Move the program object into an RGBConverter class. Keep a global
registry of RGBConverter classes for each CGLContextObj, and share
RGBConverter objects across GLImages. Make the RGBConverter always
destroy its objects when it is destroyed. It can do this because it
retains the CGLContextObj that was used to allocate them.

This could be better in that a pathological user of GLImages could
still cause the RGBConverter to be created and destroyed every frame.

BUG=598388,599314
==========

[ccameron, 2016-04-09 00:37:12]

ccameron@chromium.org changed reviewers:
+ dcastagna@chromium.org, reveman@chromium.org

[ccameron, 2016-04-09 00:41:07]

ptal.

We shot down the idea of an RGBAConverter in a previous patch. Given that
accelerated decode creates a new GLImage every frame and then leaks its GL
resources, I would like to push a bit more forcefully on the issue this time
around.

[Daniele Castagna, 2016-04-09 01:02:23]

On 2016/04/09 at 00:41:07, ccameron wrote:
> ptal.
> 
> We shot down the idea of an RGBAConverter in a previous patch. Given that
accelerated decode creates a new GLImage every frame and then leaks its GL
resources, I would like to push a bit more forcefully on the issue this time
around.

I personally have no problems with the idea of an RGBAConverter.
I'm confused about the map from context -> rgb converter, when does it happen
that we have more than one context? After a context lost?

[ccameron, 2016-04-09 01:26:59]

On 2016/04/09 01:02:23, Daniele Castagna wrote:
> On 2016/04/09 at 00:41:07, ccameron wrote:
> > ptal.
> > 
> > We shot down the idea of an RGBAConverter in a previous patch. Given that
> accelerated decode creates a new GLImage every frame and then leaks its GL
> resources, I would like to push a bit more forcefully on the issue this time
> around.
> 
> I personally have no problems with the idea of an RGBAConverter.
> I'm confused about the map from context -> rgb converter, when does it happen
> that we have more than one context? After a context lost?

There is nothing in the interface that guarantees that the GL program was
created on the context from which CopyTexImage is called. I created the map to
ensure that we always use a GL program from the current context's CGLContextObj.

In practice, because we use virtualized contexts, we're unlikely to see multiple
contexts. If we have a catastrophic error in one of the contexts, then the
CGLContextObj is likely to change.

[Daniele Castagna, 2016-04-11 19:55:30]

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.mm
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:221: CGLContextObj current_context =
CGLGetCurrentContext();
Should we DCHECK that current_context is not null?

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:222: auto found =
g_rgb_converters.Get().find(current_context);
Is it possible that we get GLImageIOSurface::BindTexImage from different threads
and we end up here from multiple threads?

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:282: GLint old_active_texture = -1;
It'd be nice to solve the Scoped* issue before landing this patch, but since it
won't happen, should we mention why we're not using ScopedActiveTexture and
ScopedTextureBinder here, you can also mention the bug you opened about it.

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:296: glGenFramebuffersEXT(1, &framebuffer);
Why did you decide not to put framebuffer in the RGBConverter?

[ccameron, 2016-04-11 20:41:21]

Thanks! Updated.

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.mm
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:221: CGLContextObj current_context =
CGLGetCurrentContext();
On 2016/04/11 19:55:30, Daniele Castagna wrote:
> Should we DCHECK that current_context is not null?

Good idea -- added.

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:222: auto found =
g_rgb_converters.Get().find(current_context);
On 2016/04/11 19:55:29, Daniele Castagna wrote:
> Is it possible that we get GLImageIOSurface::BindTexImage from different
threads
> and we end up here from multiple threads?

Yes -- the GLImageIOSurface::thread_checker_ should ensure that all calls to the
GLImageIOSurface are on the same thread.

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:282: GLint old_active_texture = -1;
On 2016/04/11 19:55:29, Daniele Castagna wrote:
> It'd be nice to solve the Scoped* issue before landing this patch, but since
it
> won't happen, should we mention why we're not using ScopedActiveTexture and
> ScopedTextureBinder here, you can also mention the bug you opened about it.

Added a comment for this.

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:296: glGenFramebuffersEXT(1, &framebuffer);
On 2016/04/11 19:55:29, Daniele Castagna wrote:
> Why did you decide not to put framebuffer in the RGBConverter?

No reason -- updated the patch to leave it in the RGBConverter.

[Daniele Castagna, 2016-04-11 20:56:16]

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.mm
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:222: auto found =
g_rgb_converters.Get().find(current_context);
On 2016/04/11 at 20:41:21, ccameron wrote:
> On 2016/04/11 19:55:29, Daniele Castagna wrote:
> > Is it possible that we get GLImageIOSurface::BindTexImage from different
threads
> > and we end up here from multiple threads?
> 
> Yes -- the GLImageIOSurface::thread_checker_ should ensure that all calls to
the GLImageIOSurface are on the same thread.

That checks that all the calls to an instance of GLImageIOSurface are on the
same thread, but now we have a singleton that could potentially be accessed from
different image instances.
Should we make that thread-safe?

We can just put the converter in the GLContext in that way we don't have to
worry about this.

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:394: void GLImageIOSurface::Destroy(bool
have_context) {
Why was have_context always false in the HW decoder case?

[ccameron, 2016-04-12 00:34:35]

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.mm
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:222: auto found =
g_rgb_converters.Get().find(current_context);
On 2016/04/11 20:56:16, Daniele Castagna wrote:
> On 2016/04/11 at 20:41:21, ccameron wrote:
> > On 2016/04/11 19:55:29, Daniele Castagna wrote:
> > > Is it possible that we get GLImageIOSurface::BindTexImage from different
> threads
> > > and we end up here from multiple threads?
> > 
> > Yes -- the GLImageIOSurface::thread_checker_ should ensure that all calls to
> the GLImageIOSurface are on the same thread.
> 
> That checks that all the calls to an instance of GLImageIOSurface are on the
> same thread, but now we have a singleton that could potentially be accessed
from
> different image instances.
> Should we make that thread-safe?
> 
> We can just put the converter in the GLContext in that way we don't have to
> worry about this.

My understanding is that there is only one valid thread to access all GLImages
on.

That said, I'm open to attaching this to the GLContext -- would that be
preferable?

https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
ui/gl/gl_image_io_surface.mm:394: void GLImageIOSurface::Destroy(bool
have_context) {
On 2016/04/11 20:56:16, Daniele Castagna wrote:
> Why was have_context always false in the HW decoder case?

The HW decoder will free the GLImage at unpredictable times (like, in a callback
from when it finished decoding a frame). As a result, it doesn't know which
context will be current.

The exact mechanism is that it ends up calling the destructor on the GLImage,
which causes causes Destroy(false) to be called.

[Daniele Castagna, 2016-04-12 01:16:04]

On 2016/04/12 at 00:34:35, ccameron wrote:
> https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.mm
> File ui/gl/gl_image_io_surface.mm (right):
> 
>
https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
> ui/gl/gl_image_io_surface.mm:222: auto found =
g_rgb_converters.Get().find(current_context);
> On 2016/04/11 20:56:16, Daniele Castagna wrote:
> > On 2016/04/11 at 20:41:21, ccameron wrote:
> > > On 2016/04/11 19:55:29, Daniele Castagna wrote:
> > > > Is it possible that we get GLImageIOSurface::BindTexImage from different
> > threads
> > > > and we end up here from multiple threads?
> > > 
> > > Yes -- the GLImageIOSurface::thread_checker_ should ensure that all calls
to
> > the GLImageIOSurface are on the same thread.
> > 
> > That checks that all the calls to an instance of GLImageIOSurface are on the
> > same thread, but now we have a singleton that could potentially be accessed
from
> > different image instances.
> > Should we make that thread-safe?
> > 
> > We can just put the converter in the GLContext in that way we don't have to
> > worry about this.
> 
> My understanding is that there is only one valid thread to access all GLImages
on.
> 

Same understanding, I'm not sure why that thread_checker_ is there then.

> That said, I'm open to attaching this to the GLContext -- would that be
preferable?
> 

Whatever you prefer. I'm OK with this solution if it's always accessed from the
same thread.

>
https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
> ui/gl/gl_image_io_surface.mm:394: void GLImageIOSurface::Destroy(bool
have_context) {
> On 2016/04/11 20:56:16, Daniele Castagna wrote:
> > Why was have_context always false in the HW decoder case?
> 
> The HW decoder will free the GLImage at unpredictable times (like, in a
callback from when it finished decoding a frame). As a result, it doesn't know
which context will be current.
> 
> The exact mechanism is that it ends up calling the destructor on the GLImage,
which causes causes Destroy(false) to be called.

Got it, you're describing what goes on in vt_video_decode_accelerator_mac.cc
ReusePictureBuffer, right?
If that is the case, can't we just use make_context_current_cb_ and destroy the
image with have_context true?

[ccameron, 2016-04-12 03:21:09]

On 2016/04/12 01:16:04, Daniele Castagna wrote:
> On 2016/04/12 at 00:34:35, ccameron wrote:
> >
https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.mm
> > File ui/gl/gl_image_io_surface.mm (right):
> > 
> >
>
https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
> > ui/gl/gl_image_io_surface.mm:222: auto found =
> g_rgb_converters.Get().find(current_context);
> > On 2016/04/11 20:56:16, Daniele Castagna wrote:
> > > On 2016/04/11 at 20:41:21, ccameron wrote:
> > > > On 2016/04/11 19:55:29, Daniele Castagna wrote:
> > > > > Is it possible that we get GLImageIOSurface::BindTexImage from
different
> > > threads
> > > > > and we end up here from multiple threads?
> > > > 
> > > > Yes -- the GLImageIOSurface::thread_checker_ should ensure that all
calls
> to
> > > the GLImageIOSurface are on the same thread.
> > > 
> > > That checks that all the calls to an instance of GLImageIOSurface are on
the
> > > same thread, but now we have a singleton that could potentially be
accessed
> from
> > > different image instances.
> > > Should we make that thread-safe?
> > > 
> > > We can just put the converter in the GLContext in that way we don't have
to
> > > worry about this.
> > 
> > My understanding is that there is only one valid thread to access all
GLImages
> on.
> > 
> 
> Same understanding, I'm not sure why that thread_checker_ is there then.
> 
> > That said, I'm open to attaching this to the GLContext -- would that be
> preferable?
> > 
> 
> Whatever you prefer. I'm OK with this solution if it's always accessed from
the
> same thread.

For now I'd rather keep the change limited to GLImageIOSurface, and then have a
follow-on patch to move things to GLContextCGL. I do think that is ultimately
where it belongs.

> >
>
https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
> > ui/gl/gl_image_io_surface.mm:394: void GLImageIOSurface::Destroy(bool
> have_context) {
> > On 2016/04/11 20:56:16, Daniele Castagna wrote:
> > > Why was have_context always false in the HW decoder case?
> > 
> > The HW decoder will free the GLImage at unpredictable times (like, in a
> callback from when it finished decoding a frame). As a result, it doesn't know
> which context will be current.
> > 
> > The exact mechanism is that it ends up calling the destructor on the
GLImage,
> which causes causes Destroy(false) to be called.
> 
> Got it, you're describing what goes on in vt_video_decode_accelerator_mac.cc
> ReusePictureBuffer, right?
> If that is the case, can't we just use make_context_current_cb_ and destroy
the
> image with have_context true?

That would fix the crashing bug, but we would still re-compile the shaders every
single frame, which is far too much.

[Daniele Castagna, 2016-04-12 04:56:26]

On 2016/04/12 at 03:21:09, ccameron wrote:
> On 2016/04/12 01:16:04, Daniele Castagna wrote:
> > On 2016/04/12 at 00:34:35, ccameron wrote:
> > >
https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.mm
> > > File ui/gl/gl_image_io_surface.mm (right):
> > > 
> > >
> >
https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
> > > ui/gl/gl_image_io_surface.mm:222: auto found =
> > g_rgb_converters.Get().find(current_context);
> > > On 2016/04/11 20:56:16, Daniele Castagna wrote:
> > > > On 2016/04/11 at 20:41:21, ccameron wrote:
> > > > > On 2016/04/11 19:55:29, Daniele Castagna wrote:
> > > > > > Is it possible that we get GLImageIOSurface::BindTexImage from
different
> > > > threads
> > > > > > and we end up here from multiple threads?
> > > > > 
> > > > > Yes -- the GLImageIOSurface::thread_checker_ should ensure that all
calls
> > to
> > > > the GLImageIOSurface are on the same thread.
> > > > 
> > > > That checks that all the calls to an instance of GLImageIOSurface are on
the
> > > > same thread, but now we have a singleton that could potentially be
accessed
> > from
> > > > different image instances.
> > > > Should we make that thread-safe?
> > > > 
> > > > We can just put the converter in the GLContext in that way we don't have
to
> > > > worry about this.
> > > 
> > > My understanding is that there is only one valid thread to access all
GLImages
> > on.
> > > 
> > 
> > Same understanding, I'm not sure why that thread_checker_ is there then.
> > 
> > > That said, I'm open to attaching this to the GLContext -- would that be
> > preferable?
> > > 
> > 
> > Whatever you prefer. I'm OK with this solution if it's always accessed from
the
> > same thread.
> 
> For now I'd rather keep the change limited to GLImageIOSurface, and then have
a follow-on patch to move things to GLContextCGL. I do think that is ultimately
where it belongs.

SGTM.

> 
> > >
> >
https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
> > > ui/gl/gl_image_io_surface.mm:394: void GLImageIOSurface::Destroy(bool
> > have_context) {
> > > On 2016/04/11 20:56:16, Daniele Castagna wrote:
> > > > Why was have_context always false in the HW decoder case?
> > > 
> > > The HW decoder will free the GLImage at unpredictable times (like, in a
> > callback from when it finished decoding a frame). As a result, it doesn't
know
> > which context will be current.
> > > 
> > > The exact mechanism is that it ends up calling the destructor on the
GLImage,
> > which causes causes Destroy(false) to be called.
> > 
> > Got it, you're describing what goes on in vt_video_decode_accelerator_mac.cc
> > ReusePictureBuffer, right?
> > If that is the case, can't we just use make_context_current_cb_ and destroy
the
> > image with have_context true?
> 
> That would fix the crashing bug, but we would still re-compile the shaders
every single frame, which is far too much.

I was not suggesting to remove the RGBConverter.

Just trying to understand why we can't have the decoder to respect GLImage
interface and call Destroy with a current context when available.
IIUC what have_context really means is if we successfully made a context current
before calling GLImage::Destroy, if not, GLImage can avoid cleaning up the
resources since the context is gone. In the decoder case we don't even try and
we just pass false, it seems like that should be fixed too.

Probably better to address that in another CL though.

LGTM.

[ccameron, 2016-04-12 06:28:22]

On 2016/04/12 04:56:26, Daniele Castagna wrote:
> On 2016/04/12 at 03:21:09, ccameron wrote:
> > On 2016/04/12 01:16:04, Daniele Castagna wrote:
> > > On 2016/04/12 at 00:34:35, ccameron wrote:
> > > >
> https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.mm
> > > > File ui/gl/gl_image_io_surface.mm (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
> > > > ui/gl/gl_image_io_surface.mm:222: auto found =
> > > g_rgb_converters.Get().find(current_context);
> > > > On 2016/04/11 20:56:16, Daniele Castagna wrote:
> > > > > On 2016/04/11 at 20:41:21, ccameron wrote:
> > > > > > On 2016/04/11 19:55:29, Daniele Castagna wrote:
> > > > > > > Is it possible that we get GLImageIOSurface::BindTexImage from
> different
> > > > > threads
> > > > > > > and we end up here from multiple threads?
> > > > > > 
> > > > > > Yes -- the GLImageIOSurface::thread_checker_ should ensure that all
> calls
> > > to
> > > > > the GLImageIOSurface are on the same thread.
> > > > > 
> > > > > That checks that all the calls to an instance of GLImageIOSurface are
on
> the
> > > > > same thread, but now we have a singleton that could potentially be
> accessed
> > > from
> > > > > different image instances.
> > > > > Should we make that thread-safe?
> > > > > 
> > > > > We can just put the converter in the GLContext in that way we don't
have
> to
> > > > > worry about this.
> > > > 
> > > > My understanding is that there is only one valid thread to access all
> GLImages
> > > on.
> > > > 
> > > 
> > > Same understanding, I'm not sure why that thread_checker_ is there then.
> > > 
> > > > That said, I'm open to attaching this to the GLContext -- would that be
> > > preferable?
> > > > 
> > > 
> > > Whatever you prefer. I'm OK with this solution if it's always accessed
from
> the
> > > same thread.
> > 
> > For now I'd rather keep the change limited to GLImageIOSurface, and then
have
> a follow-on patch to move things to GLContextCGL. I do think that is
ultimately
> where it belongs.
> 
> SGTM.
> 
> > 
> > > >
> > >
>
https://codereview.chromium.org/1870323002/diff/1/ui/gl/gl_image_io_surface.m...
> > > > ui/gl/gl_image_io_surface.mm:394: void GLImageIOSurface::Destroy(bool
> > > have_context) {
> > > > On 2016/04/11 20:56:16, Daniele Castagna wrote:
> > > > > Why was have_context always false in the HW decoder case?
> > > > 
> > > > The HW decoder will free the GLImage at unpredictable times (like, in a
> > > callback from when it finished decoding a frame). As a result, it doesn't
> know
> > > which context will be current.
> > > > 
> > > > The exact mechanism is that it ends up calling the destructor on the
> GLImage,
> > > which causes causes Destroy(false) to be called.
> > > 
> > > Got it, you're describing what goes on in
vt_video_decode_accelerator_mac.cc
> > > ReusePictureBuffer, right?
> > > If that is the case, can't we just use make_context_current_cb_ and
destroy
> the
> > > image with have_context true?
> > 
> > That would fix the crashing bug, but we would still re-compile the shaders
> every single frame, which is far too much.
> 
> I was not suggesting to remove the RGBConverter.
> 
> Just trying to understand why we can't have the decoder to respect GLImage
> interface and call Destroy with a current context when available.
> IIUC what have_context really means is if we successfully made a context
current
> before calling GLImage::Destroy, if not, GLImage can avoid cleaning up the
> resources since the context is gone. In the decoder case we don't even try and
> we just pass false, it seems like that should be fixed too.
> 
> Probably better to address that in another CL though.

Yes, good point -- we should make the context current for the Destroy call. I'll
update the VideoToolbox code in a follow-on.

> LGTM.

Thanks!

[ccameron, 2016-04-12 15:42:48]

reveman@, did you want to take a look at this as well?

[reveman, 2016-04-12 15:51:03]

https://codereview.chromium.org/1870323002/diff/20001/ui/gl/gl_image_io_surfa...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1870323002/diff/20001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.mm:225: if (found != g_rgb_converters.Get().end())
This isn't thread safe. Can we add a thread checker to make sure this is not
used incorrectly?

[ccameron, 2016-04-12 16:16:59]

Thanks -- updated.

https://codereview.chromium.org/1870323002/diff/20001/ui/gl/gl_image_io_surfa...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1870323002/diff/20001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.mm:225: if (found != g_rgb_converters.Get().end())
On 2016/04/12 15:51:03, reveman wrote:
> This isn't thread safe. Can we add a thread checker to make sure this is not
> used incorrectly?

Good point -- done.

[ccameron, 2016-04-12 18:39:33]

I'm getting pressure from PMs to get this landed for a build at 5pm today --
does this lg?

[reveman, 2016-04-12 18:44:44]

lgtm

[ccameron, 2016-04-12 18:52:27]

Thanks!!

[ccameron, 2016-04-12 18:52:34]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2016-04-12 18:52:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
dcastagna@chromium.org
Link to the patchset: https://codereview.chromium.org/1870323002/#ps40001
(title: "Add thread checker")

[commit-bot: I haz the power, 2016-04-12 18:53:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1870323002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1870323002/40001

[commit-bot: I haz the power, 2016-04-12 19:29:37]

Description was changed from

==========
Mac h264: Plug GL program leak

The state for YUV to RGB conversion for GLImageIOSurface is stored
in the GLImageIOSurface object. This is objectionable because
- This means every GLImage object will re-create all shaders
- For hardware decode, we create a new GLImage for every decoded frame,
  which means allocating a shader for every frame.
- It is not clear which CGLContextObj the GL objects are associated
  with.
- For hardware decode, we do not specify that a GL context is current
  when we free GLImages, so we end up leaking one GL program every
  frame.
- This memory leak ends up causing GL context destroy to take more than
  ten seconds, causing a GPU hang when the GL context is destroyed.

Move the program object into an RGBConverter class. Keep a global
registry of RGBConverter classes for each CGLContextObj, and share
RGBConverter objects across GLImages. Make the RGBConverter always
destroy its objects when it is destroyed. It can do this because it
retains the CGLContextObj that was used to allocate them.

This could be better in that a pathological user of GLImages could
still cause the RGBConverter to be created and destroyed every frame.

BUG=598388,599314
==========

to

==========
Mac h264: Plug GL program leak

The state for YUV to RGB conversion for GLImageIOSurface is stored
in the GLImageIOSurface object. This is objectionable because
- This means every GLImage object will re-create all shaders
- For hardware decode, we create a new GLImage for every decoded frame,
  which means allocating a shader for every frame.
- It is not clear which CGLContextObj the GL objects are associated
  with.
- For hardware decode, we do not specify that a GL context is current
  when we free GLImages, so we end up leaking one GL program every
  frame.
- This memory leak ends up causing GL context destroy to take more than
  ten seconds, causing a GPU hang when the GL context is destroyed.

Move the program object into an RGBConverter class. Keep a global
registry of RGBConverter classes for each CGLContextObj, and share
RGBConverter objects across GLImages. Make the RGBConverter always
destroy its objects when it is destroyed. It can do this because it
retains the CGLContextObj that was used to allocate them.

This could be better in that a pathological user of GLImages could
still cause the RGBConverter to be created and destroyed every frame.

BUG=598388,599314
==========

[commit-bot: I haz the power, 2016-04-12 19:29:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-12 19:30:49]

Description was changed from

==========
Mac h264: Plug GL program leak

The state for YUV to RGB conversion for GLImageIOSurface is stored
in the GLImageIOSurface object. This is objectionable because
- This means every GLImage object will re-create all shaders
- For hardware decode, we create a new GLImage for every decoded frame,
  which means allocating a shader for every frame.
- It is not clear which CGLContextObj the GL objects are associated
  with.
- For hardware decode, we do not specify that a GL context is current
  when we free GLImages, so we end up leaking one GL program every
  frame.
- This memory leak ends up causing GL context destroy to take more than
  ten seconds, causing a GPU hang when the GL context is destroyed.

Move the program object into an RGBConverter class. Keep a global
registry of RGBConverter classes for each CGLContextObj, and share
RGBConverter objects across GLImages. Make the RGBConverter always
destroy its objects when it is destroyed. It can do this because it
retains the CGLContextObj that was used to allocate them.

This could be better in that a pathological user of GLImages could
still cause the RGBConverter to be created and destroyed every frame.

BUG=598388,599314
==========

to

==========
Mac h264: Plug GL program leak

The state for YUV to RGB conversion for GLImageIOSurface is stored
in the GLImageIOSurface object. This is objectionable because
- This means every GLImage object will re-create all shaders
- For hardware decode, we create a new GLImage for every decoded frame,
  which means allocating a shader for every frame.
- It is not clear which CGLContextObj the GL objects are associated
  with.
- For hardware decode, we do not specify that a GL context is current
  when we free GLImages, so we end up leaking one GL program every
  frame.
- This memory leak ends up causing GL context destroy to take more than
  ten seconds, causing a GPU hang when the GL context is destroyed.

Move the program object into an RGBConverter class. Keep a global
registry of RGBConverter classes for each CGLContextObj, and share
RGBConverter objects across GLImages. Make the RGBConverter always
destroy its objects when it is destroyed. It can do this because it
retains the CGLContextObj that was used to allocate them.

This could be better in that a pathological user of GLImages could
still cause the RGBConverter to be created and destroyed every frame.

BUG=598388,599314

Committed: https://crrev.com/c9295d4075c1e4a32977c446ab9734a88abf8391
Cr-Commit-Position: refs/heads/master@{#386757}
==========

[commit-bot: I haz the power, 2016-04-12 19:30:50]

Patchset 3 (id:??) landed as
https://crrev.com/c9295d4075c1e4a32977c446ab9734a88abf8391
Cr-Commit-Position: refs/heads/master@{#386757}