OMX: Avoid calling Fillbuffer while OMX is flushing.

OMX: Avoid calling Fillbuffer while OMX is flushing.

If Reusepicturebuffer is called when OMX port is being flushed
OMX_* call will error and force the decoder into error mode.

So delay the Fill buffer till state is not Resetting

BUG=chrome-os-partner:13711
TEST=by hand on snow

Change-Id: Ia581296a77d902b32cdd02f05da3a905af2acc42
Signed-off-by: Arun Mankuzhi <arun.m@samsung.com>;


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

[Ami GONE FROM CHROMIUM, 2012-09-12 01:09:36]

I don't see where the OMX spec prohibits calling OMX_FillThisBuffer during a
port-flush, which is why CanFillBuffer() returns true if c_s_c_==RESETTING (and
client_state_ is one of idle/executing/paused, per the table in 3.2.2 of the OMX
1.1.2 spec).

Can this be fixed in the SEC-OMX implementation instead?

In case it can't, I'm attaching some code-specific comments below.

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

https://chromiumcodereview.appspot.com/10916203/diff/1/content/common/gpu/med...
content/common/gpu/media/omx_video_decode_accelerator.cc:393:
RETURN_ON_FAILURE(CanFillBuffer(), "Can't fill buffer", ILLEGAL_STATE,);
This probably belongs *after* the new code, not before it.

https://chromiumcodereview.appspot.com/10916203/diff/1/content/common/gpu/med...
content/common/gpu/media/omx_video_decode_accelerator.cc:395: //During
port-flushing, do not call OMX FillThisBuffer
nits: add a space between the // and the 'D', and end the sentence with a
period.

[Ami GONE FROM CHROMIUM, 2012-09-12 01:10:42]

Oh, also, make sure to "Publish+Mail Comments" after uploading code-reviews;
otherwise no mail is sent even if you add a reviewer on rietveld (this is an
unfortunate difference between gerrit and rietveld).

[Arun M, 2012-09-17 12:06:55]

While flushing buffers, if a new Fill buffer is called this buffer has to be
also returned back to VDA and the VDA will then push it into the
queued_picture_buffer_ids_ queue.

[Ami GONE FROM CHROMIUM, 2012-09-17 17:27:10]

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

https://chromiumcodereview.appspot.com/10916203/diff/4001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:1132: return (csc !=
DESTROYING && csc != ERRORING) &&
As I said before:

I don't see where the OMX spec prohibits calling OMX_FillThisBuffer during a
port-flush, which is why CanFillBuffer() returns true if c_s_c_==RESETTING (and
client_state_ is one of idle/executing/paused, per the table in 3.2.2 of the OMX
1.1.2 spec).

Can this be fixed in the SEC-OMX implementation instead?

https://chromiumcodereview.appspot.com/10916203/diff/4001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:1133: (cs ==
OMX_StateIdle || cs == OMX_StateExecuting || cs == OMX_StatePause);
If this CL was to land, then I think this function should return false if
csc==RESETTING.

[Arun M, 2012-09-18 08:39:34]

IL spec 3.2.2 suggests client can call OMX_FillThisBuffer() during
Idle/Pause/Executing. The case we are discussing is during Flush() ports (Pause
+ flush and not pause alone) which mandates all buffers to be returned to
client.

[Ami GONE FROM CHROMIUM, 2012-09-18 16:50:43]

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

https://chromiumcodereview.appspot.com/10916203/diff/4001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:803: 
I think you need an
if (current_state_change_ == RESETTING)
  return;

here as well to guard against the same problem.

https://chromiumcodereview.appspot.com/10916203/diff/4001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:1128: bool
OmxVideoDecodeAccelerator::CanFillBuffer() {
In fact this is only used to assert that FillBuffer *can* be called, not to ask
whether it can be, so this should be renamed AssertCanFillBuffer() (and return
void).  I'll send a CL to do that unless you tell me you want to do it (in
another CL).

https://chromiumcodereview.appspot.com/10916203/diff/4001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:1132: return (csc !=
DESTROYING && csc != ERRORING) &&
please add 
&& csc !=RESETTING
to this parenthetical.

[Arun M, 2012-09-19 07:07:09]

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

https://chromiumcodereview.appspot.com/10916203/diff/4001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:803: 
Do we need this check? CanFillBuffer will return false while RESETTING.
On 2012/09/18 16:50:44, Ami Fischman wrote:
> I think you need an
> if (current_state_change_ == RESETTING)
>   return;
> 
> here as well to guard against the same problem.

https://chromiumcodereview.appspot.com/10916203/diff/4001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:1132: return (csc !=
DESTROYING && csc != ERRORING) &&
On 2012/09/18 16:50:44, Ami Fischman wrote:
> please add 
> && csc !=RESETTING
> to this parenthetical.

Done.

[Arun M, 2012-09-19 09:45:16]

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

https://chromiumcodereview.appspot.com/10916203/diff/4001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:1128: bool
OmxVideoDecodeAccelerator::CanFillBuffer() {
I will submit another CL for this.
On 2012/09/18 16:50:44, Ami Fischman wrote:
> In fact this is only used to assert that FillBuffer *can* be called, not to
ask
> whether it can be, so this should be renamed AssertCanFillBuffer() (and return
> void).  I'll send a CL to do that unless you tell me you want to do it (in
> another CL).

[Ami GONE FROM CHROMIUM, 2012-09-19 17:02:10]

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

https://chromiumcodereview.appspot.com/10916203/diff/4001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:803: 
On 2012/09/19 07:07:09, Arun M wrote:
> On 2012/09/18 16:50:44, Ami Fischman wrote:
> > I think you need an
> > if (current_state_change_ == RESETTING)
> >   return;
> > here as well to guard against the same problem.
> Do we need this check? CanFillBuffer will return false while RESETTING.

My point was that if this method is called during RESETTING, then the decoder
will StopOnError() below and won't be useful anymore.  
But merely returning early from here in case of RESETTING wouldn't be enough
anyway, because then nobody would be requesting FillThisBuffer on the elements
of pictures_.

Instead I think this wants to be
if (c_s_c_ == RESETTING) {
  for (OutputPictureById::iterator it = pictures_.begin();
       it != pictures_.end(); ++it) {
    queued_picture_buffer_ids_.push_back(it->first);
  }
  return;
}
so that when RESETTING is done these picture buffers will get "Reused".

[Arun M, 2012-09-20 11:40:12]

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

https://chromiumcodereview.appspot.com/10916203/diff/4001/content/common/gpu/...
content/common/gpu/media/omx_video_decode_accelerator.cc:803: 
On 2012/09/19 17:02:10, Ami Fischman wrote:
> On 2012/09/19 07:07:09, Arun M wrote:
> > On 2012/09/18 16:50:44, Ami Fischman wrote:
> > > I think you need an
> > > if (current_state_change_ == RESETTING)
> > >   return;
> > > here as well to guard against the same problem.
> > Do we need this check? CanFillBuffer will return false while RESETTING.
> 
> My point was that if this method is called during RESETTING, then the decoder
> will StopOnError() below and won't be useful anymore.  
> But merely returning early from here in case of RESETTING wouldn't be enough
> anyway, because then nobody would be requesting FillThisBuffer on the elements
> of pictures_.
> 
> Instead I think this wants to be
> if (c_s_c_ == RESETTING) {
>   for (OutputPictureById::iterator it = pictures_.begin();
>        it != pictures_.end(); ++it) {
>     queued_picture_buffer_ids_.push_back(it->first);
>   }
>   return;
> }
> so that when RESETTING is done these picture buffers will get "Reused".

Done.

[Ami GONE FROM CHROMIUM, 2012-09-20 17:19:40]

LGTM + cq'ing.

[commit-bot: I haz the power, 2012-09-20 17:20:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arun.m@samsung.com/10916203/16001

[commit-bot: I haz the power, 2012-09-21 04:55:06]

Retried try job too often for step(s) sync_integration_tests,
chrome_frame_net_tests

[commit-bot: I haz the power, 2012-09-21 05:02:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arun.m@samsung.com/10916203/16001

[commit-bot: I haz the power, 2012-09-23 02:29:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/arun.m@samsung.com/10916203/16001

[commit-bot: I haz the power, 2012-09-23 04:20:25]

Change committed as 158202