Always call OMX_FreeBuffer(), even in ERRORING.

Always call OMX_FreeBuffer(), even in ERRORING.

BUG=chrome-os-partner:12316
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151728

[marcheu, 2012-08-14 22:11:25]

https://chromiumcodereview.appspot.com/10854146/diff/2001/content/common/gpu/...
File content/common/gpu/media/omx_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/10854146/diff/2001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:730:
RETURN_ON_OMX_FAILURE(result, "OMX_FreeBuffer", PLATFORM_FAILURE,);
Can we keep iterating even if we fail a FreeBuffer call? That way we can still
try to free the other pictures, and destroy the eglImages.

[Pawel Osciak, 2012-08-14 22:17:14]

https://chromiumcodereview.appspot.com/10854146/diff/2001/content/common/gpu/...
File content/common/gpu/media/omx_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/10854146/diff/2001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:473:
ShutdownComponent();
Can we get here without freeing the buffers first? If so, I think we should free
them here.

https://chromiumcodereview.appspot.com/10854146/diff/2001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:615:
ShutdownComponent();
Same here, can we get here without freeing buffers?

[Ami GONE FROM CHROMIUM, 2012-08-15 04:08:29]

https://chromiumcodereview.appspot.com/10854146/diff/2001/content/common/gpu/...
File content/common/gpu/media/omx_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/10854146/diff/2001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:473:
ShutdownComponent();
On 2012/08/14 22:17:14, posciak wrote:
> Can we get here without freeing the buffers first? If so, I think we should
free
> them here.

No; Invalid & Loaded can only be reached through ERRORING or DESTROYING, or by
never have been initialized (see the state machine dispatcher below).

https://chromiumcodereview.appspot.com/10854146/diff/2001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:615:
ShutdownComponent();
On 2012/08/14 22:17:14, posciak wrote:
> Same here, can we get here without freeing buffers?

No, the only way to get to Loaded is through OnReachedIdleInDestroying(), which
calls FOB().

https://chromiumcodereview.appspot.com/10854146/diff/2001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:730:
RETURN_ON_OMX_FAILURE(result, "OMX_FreeBuffer", PLATFORM_FAILURE,);
On 2012/08/14 22:11:25, marcheu wrote:
> Can we keep iterating even if we fail a FreeBuffer call? That way we can still
> try to free the other pictures, and destroy the eglImages.

I really dislike this sort of programming.
I guess you're going for belt&suspenders here, but I don't like writing code
which "should" never get run.  Do you know of any concrete ways in which an
OMX_FreeBuffer call can fail and yet have further OMX_FB calls be useful, or the
DestroyEglImage below be useful?

At any rate, done.

[marcheu, 2012-08-15 05:43:07]

On 2012/08/15 04:08:29, Ami Fischman wrote:
>
https://chromiumcodereview.appspot.com/10854146/diff/2001/content/common/gpu/...
> File content/common/gpu/media/omx_video_decode_accelerator.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10854146/diff/2001/content/common/gpu/...
> content/common/gpu/media/omx_video_decode_accelerator.cc:473:
> ShutdownComponent();
> On 2012/08/14 22:17:14, posciak wrote:
> > Can we get here without freeing the buffers first? If so, I think we should
> free
> > them here.
> 
> No; Invalid & Loaded can only be reached through ERRORING or DESTROYING, or by
> never have been initialized (see the state machine dispatcher below).
> 
>
https://chromiumcodereview.appspot.com/10854146/diff/2001/content/common/gpu/...
> content/common/gpu/media/omx_video_decode_accelerator.cc:615:
> ShutdownComponent();
> On 2012/08/14 22:17:14, posciak wrote:
> > Same here, can we get here without freeing buffers?
> 
> No, the only way to get to Loaded is through OnReachedIdleInDestroying(),
which
> calls FOB().
> 
>
https://chromiumcodereview.appspot.com/10854146/diff/2001/content/common/gpu/...
> content/common/gpu/media/omx_video_decode_accelerator.cc:730:
> RETURN_ON_OMX_FAILURE(result, "OMX_FreeBuffer", PLATFORM_FAILURE,);
> On 2012/08/14 22:11:25, marcheu wrote:
> > Can we keep iterating even if we fail a FreeBuffer call? That way we can
still
> > try to free the other pictures, and destroy the eglImages.
> 
> I really dislike this sort of programming.
> I guess you're going for belt&suspenders here, but I don't like writing code

Yes belt & suspenders. After seeing some OMX implementations I'm now a much more
careful person :)

> which "should" never get run.  Do you know of any concrete ways in which an
> OMX_FreeBuffer call can fail and yet have further OMX_FB calls be useful, or
the
> DestroyEglImage below be useful?

Well, imagine an implementation with two refcnt on the eglimage (one from OMX
and one from a GL driver). OMX_FreeBuffer fails but eglImage release succeeds.
The buffer would stay around after that, but only until we close the OMX
context. Otherwise it'd stay around until the GL context disappears, which is
longer, and can potentially be the life of the GPU process.

[Ami GONE FROM CHROMIUM, 2012-08-15 05:48:40]

OK, so updated patch set lgty?

[marcheu, 2012-08-15 06:03:10]

On 2012/08/15 05:48:40, Ami Fischman wrote:
> OK, so updated patch set lgty?

yes ltgm

[marcheu, 2012-08-15 06:03:18]

On 2012/08/15 06:03:10, marcheu wrote:
> On 2012/08/15 05:48:40, Ami Fischman wrote:
> > OK, so updated patch set lgty?
> 
> yes ltgm

lgtm even

[commit-bot: I haz the power, 2012-08-15 14:12:00]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Ami GONE FROM CHROMIUM, 2012-08-15 14:30:35]

scherkus: i can haz super-star review?

[Pawel Osciak, 2012-08-15 14:34:54]

And worthless noncommiter lgtm as well.
Btw, another thing we have to always call not to leak is DestroyEglImage, but we
do it in FreeBuffers, so all is well.

[scherkus (not reviewing), 2012-08-15 17:19:07]

lgtm

[commit-bot: I haz the power, 2012-08-15 17:21:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/10854146/6002

[commit-bot: I haz the power, 2012-08-15 19:39:52]

Change committed as 151728