OMX: Don't leave FreeOutputBuffers if MakeCurrent fails.

OMX: Don't leave FreeOutputBuffers if MakeCurrent fails.

If it fails, we still want to free the OMX resources.

BUG=chrome-os-partner:12316
TEST=by hand: play a video, look for buffer leaks.

Change-Id: Ifcc4fe4248b72067e4592224b24fdb716ab4b230


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151376

[Ami GONE FROM CHROMIUM, 2012-08-11 04:23:37]

LGTM % nit & q

https://chromiumcodereview.appspot.com/10832263/diff/1/content/common/gpu/med...
File content/common/gpu/media/omx_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/10832263/diff/1/content/common/gpu/med...
content/common/gpu/media/omx_video_decode_accelerator.cc:725: if
(make_context_current_.Run())
multi-line if bodies require braces in chromium style.

https://chromiumcodereview.appspot.com/10832263/diff/1/content/common/gpu/med...
content/common/gpu/media/omx_video_decode_accelerator.cc:725: if
(make_context_current_.Run())
Do you have an idea of what can cause the makecurrent to fail but
OMX_FreeBuffer() to still be a useful thing to call?

[piman, 2012-08-11 04:50:12]

https://chromiumcodereview.appspot.com/10832263/diff/1/content/common/gpu/med...
File content/common/gpu/media/omx_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/10832263/diff/1/content/common/gpu/med...
content/common/gpu/media/omx_video_decode_accelerator.cc:725: if
(make_context_current_.Run())
It doesn't look like we need to make_context_current at all ( DestroyEglImage
doesn't call anything gl).

[Ami GONE FROM CHROMIUM, 2012-08-11 17:19:22]

On 2012/08/11 04:50:12, piman wrote:
>
https://chromiumcodereview.appspot.com/10832263/diff/1/content/common/gpu/med...
> File content/common/gpu/media/omx_video_decode_accelerator.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10832263/diff/1/content/common/gpu/med...
> content/common/gpu/media/omx_video_decode_accelerator.cc:725: if
> (make_context_current_.Run())
> It doesn't look like we need to make_context_current at all ( DestroyEglImage
> doesn't call anything gl).

How can you be sure?  When the EGLImage is created, the context is passed in. 
Is there some principle that implies we know it's not used in destruction?

[piman, 2012-08-11 20:19:32]

On Sat, Aug 11, 2012 at 10:19 AM, <fischman@chromium.org> wrote:

> On 2012/08/11 04:50:12, piman wrote:
>
> https://chromiumcodereview.**appspot.com/10832263/diff/1/**
>
content/common/gpu/media/omx_**video_decode_accelerator.cc<https://chromiumcodereview.appspot.com/10832263/diff/1/content/common/gpu/media/omx_video_decode_accelerator.cc>
>
>> File content/common/gpu/media/omx_**video_decode_accelerator.cc (right):
>>
>
>
> https://chromiumcodereview.**appspot.com/10832263/diff/1/**
>
content/common/gpu/media/omx_**video_decode_accelerator.cc#**newcode725<https://chromiumcodereview.appspot.com/10832263/diff/1/content/common/gpu/media/omx_video_decode_accelerator.cc#newcode725>
>
>> content/common/gpu/media/omx_**video_decode_accelerator.cc:**725: if
>> (make_context_current_.Run())
>> It doesn't look like we need to make_context_current at all (
>> DestroyEglImage
>> doesn't call anything gl).
>>
>
> How can you be sure?  When the EGLImage is created, the context is passed
> in.
>

Right, it's passed in, it doesn't rely on the context being current. EGL
APIs don't have any notion of a "current" context (it's a GL concept).


> Is there some principle that implies we know it's not used in destruction?
>

It may be used without being made current (EGL can maintain the association
inside of its internal structures, since it's passed at construction time).


>
>
https://chromiumcodereview.**appspot.com/10832263/<https://chromiumcodereview...
>

[marcheu, 2012-08-13 18:10:38]

On 2012/08/11 20:19:32, piman wrote:
> On Sat, Aug 11, 2012 at 10:19 AM, <mailto:fischman@chromium.org> wrote:
> 
> > On 2012/08/11 04:50:12, piman wrote:
> >
> > https://chromiumcodereview.**appspot.com/10832263/diff/1/**
> >
>
content/common/gpu/media/omx_**video_decode_accelerator.cc<https://chromiumcodereview.appspot.com/10832263/diff/1/content/common/gpu/media/omx_video_decode_accelerator.cc>
> >
> >> File content/common/gpu/media/omx_**video_decode_accelerator.cc (right):
> >>
> >
> >
> > https://chromiumcodereview.**appspot.com/10832263/diff/1/**
> >
>
content/common/gpu/media/omx_**video_decode_accelerator.cc#**newcode725<https://chromiumcodereview.appspot.com/10832263/diff/1/content/common/gpu/media/omx_video_decode_accelerator.cc#newcode725>
> >
> >> content/common/gpu/media/omx_**video_decode_accelerator.cc:**725: if
> >> (make_context_current_.Run())
> >> It doesn't look like we need to make_context_current at all (
> >> DestroyEglImage
> >> doesn't call anything gl).
> >>
> >
> > How can you be sure?  When the EGLImage is created, the context is passed
> > in.
> >
> 
> Right, it's passed in, it doesn't rely on the context being current. EGL
> APIs don't have any notion of a "current" context (it's a GL concept).
> 
> 
> > Is there some principle that implies we know it's not used in destruction?
> >
> 
> It may be used without being made current (EGL can maintain the association
> inside of its internal structures, since it's passed at construction time).
> 
> 
> >
> >
>
https://chromiumcodereview.**appspot.com/10832263/%3Chttps://chromiumcoderevi...>
> >

You're right, I adjusted the CL to stop calling MakeCurrent completely.

[Ami GONE FROM CHROMIUM, 2012-08-13 18:27:51]

lgtm

[piman, 2012-08-13 18:28:14]

lgtm

[Pawel Osciak, 2012-08-13 18:54:54]

lgtm

[commit-bot: I haz the power, 2012-08-13 21:23:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marcheu@chromium.org/10832263/6001

[commit-bot: I haz the power, 2012-08-13 22:49:01]

Change committed as 151376