rendering_helper - Wait for rendering before resetting the decoder.

rendering_helper - Wait for rendering before resetting the decoder.

BUG=391641
TEST= Run the test on lumpy

Committed: https://crrev.com/1cc19e420c43d6ec628a0b62bdcc86625d366a65
Cr-Commit-Position: refs/heads/master@{#291635}

[Owen Lin, 2014-08-13 10:05:02]

PTAL. Thanks.

[Pawel Osciak, 2014-08-14 05:08:33]

On 2014/08/13 10:05:02, Owen Lin wrote:
> PTAL. Thanks.

What do you think about this idea instead:

We already have a callback, ReusePicture(). And media::VideoFrame has a bool
end_of_stream_. We could set it for last frame and keep a count of
frames_at_render, and if it's not at 0 when we flush, then do this in
ReusePicture when the count drops to 0.
And the RenderingHelper would know to release last picture if it checks for
end_of_stream_ in VF after rendering?

[Pawel Osciak, 2014-08-14 05:11:08]

https://codereview.chromium.org/465293002/diff/1/content/common/gpu/media/ren...
File content/common/gpu/media/rendering_helper.cc (right):

https://codereview.chromium.org/465293002/diff/1/content/common/gpu/media/ren...
content/common/gpu/media/rendering_helper.cc:97: Clear();
Should we assert that pending frames are empty?

https://codereview.chromium.org/465293002/diff/1/content/common/gpu/media/ren...
content/common/gpu/media/rendering_helper.cc:540:
base::ResetAndReturn(&client->wait_rendering_cb).Run();
I think we have to pop the last frame here?

[Owen Lin, 2014-08-14 08:22:17]

On 2014/08/14 05:08:33, Pawel Osciak wrote:
> On 2014/08/13 10:05:02, Owen Lin wrote:
> > PTAL. Thanks.
> 
> What do you think about this idea instead:
> 
> We already have a callback, ReusePicture(). And media::VideoFrame has a bool
> end_of_stream_. We could set it for last frame and keep a count of
> frames_at_render, and if it's not at 0 when we flush, then do this in
> ReusePicture when the count drops to 0.
> And the RenderingHelper would know to release last picture if it checks for
> end_of_stream_ in VF after rendering?

A problem is it is hard to know which VideoFrame would be the last. There are
some cases we will play the video more than once. But I am not clear why it
related to ReusePicture. (no mater it is ReturnPicture or ReusePictureBuffer?)

[Pawel Osciak, 2014-08-14 08:54:28]

On 2014/08/14 08:22:17, Owen Lin wrote:
> On 2014/08/14 05:08:33, Pawel Osciak wrote:
> > On 2014/08/13 10:05:02, Owen Lin wrote:
> > > PTAL. Thanks.
> > 
> > What do you think about this idea instead:
> > 
> > We already have a callback, ReusePicture(). And media::VideoFrame has a bool
> > end_of_stream_. We could set it for last frame and keep a count of
> > frames_at_render, and if it's not at 0 when we flush, then do this in
> > ReusePicture when the count drops to 0.
> > And the RenderingHelper would know to release last picture if it checks for
> > end_of_stream_ in VF after rendering?
> 
> A problem is it is hard to know which VideoFrame would be the last. There are
> some cases we will play the video more than once. But I am not clear why it
> related to ReusePicture. (no mater it is ReturnPicture or ReusePictureBuffer?)

I guess I'd be ok with a Flush() call to the rendering helper too.
Oh, and I meant ReturnPicture in your other CL, sorry.

[Owen Lin, 2014-08-14 10:26:42]

On 2014/08/14 08:54:28, Pawel Osciak wrote:
> On 2014/08/14 08:22:17, Owen Lin wrote:
> > On 2014/08/14 05:08:33, Pawel Osciak wrote:
> > > On 2014/08/13 10:05:02, Owen Lin wrote:
> > > > PTAL. Thanks.
> > > 
> > > What do you think about this idea instead:
> > > 
> > > We already have a callback, ReusePicture(). And media::VideoFrame has a
bool
> > > end_of_stream_. We could set it for last frame and keep a count of
> > > frames_at_render, and if it's not at 0 when we flush, then do this in
> > > ReusePicture when the count drops to 0.
> > > And the RenderingHelper would know to release last picture if it checks
for
> > > end_of_stream_ in VF after rendering?
> > 
> > A problem is it is hard to know which VideoFrame would be the last. There
are
> > some cases we will play the video more than once. But I am not clear why it
> > related to ReusePicture. (no mater it is ReturnPicture or
ReusePictureBuffer?)
> 
> I guess I'd be ok with a Flush() call to the rendering helper too.
> Oh, and I meant ReturnPicture in your other CL, sorry.

Thanks. I see, I will 

1. Rename SetWaitRenderingCB() to Flush(). 
2. Add a member "in_flushing" to RenderedVideo. 

So that we can release the last picture. How do you think ?

[Owen Lin, 2014-08-14 10:55:35]

PTAL. Thanks.

https://codereview.chromium.org/465293002/diff/1/content/common/gpu/media/ren...
File content/common/gpu/media/rendering_helper.cc (right):

https://codereview.chromium.org/465293002/diff/1/content/common/gpu/media/ren...
content/common/gpu/media/rendering_helper.cc:97: Clear();
On 2014/08/14 05:11:07, Pawel Osciak wrote:
> Should we assert that pending frames are empty?
It's OK to have pending frames, Clear() will also clear it.

https://codereview.chromium.org/465293002/diff/1/content/common/gpu/media/ren...
content/common/gpu/media/rendering_helper.cc:540:
base::ResetAndReturn(&client->wait_rendering_cb).Run();
On 2014/08/14 05:11:07, Pawel Osciak wrote:
> I think we have to pop the last frame here?

Done.

[Pawel Osciak, 2014-08-15 06:06:43]

https://codereview.chromium.org/465293002/diff/40001/content/common/gpu/media...
File content/common/gpu/media/rendering_helper.h (right):

https://codereview.chromium.org/465293002/diff/40001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.h:103: void Flush(size_t window_id,
const base::Closure& flush_done_cb);
You misunderstood me. I don't think we need flush_done_cb. You can just do flush
done check in ReturnPicture in the client. You need to keep a counter of
frames_at_render_ in the client for this and if it reaches 0 when in flushing in
ReturnPicture, we are done.

[Owen Lin, 2014-08-18 10:06:36]

In this PS, I moved rendering_helper->Flush() from NotifyFlushDone() to
NotifyResetDone(). It actually failed in the testcase "ReplayAfterEOS". In that
test, there would be still coming frames after NotifyFlushDone().

However, the moving makes DropPendingFrames() in a invalid state. It was
originally designed for the case of MidStreamReset. So that we don't need to
wait for those pending frames after reset. I guess it is OK to change the
behavior. If you don't agree, we can figure out some other way to do it, e.g.,
add a memeber "drop_pending_frames_at_reset". Please take another look and thank
you for the feedback.

https://codereview.chromium.org/465293002/diff/40001/content/common/gpu/media...
File content/common/gpu/media/rendering_helper.h (right):

https://codereview.chromium.org/465293002/diff/40001/content/common/gpu/media...
content/common/gpu/media/rendering_helper.h:103: void Flush(size_t window_id,
const base::Closure& flush_done_cb);
On 2014/08/15 06:06:43, Pawel Osciak wrote:
> You misunderstood me. I don't think we need flush_done_cb. You can just do
flush
> done check in ReturnPicture in the client. You need to keep a counter of
> frames_at_render_ in the client for this and if it reaches 0 when in flushing
in
> ReturnPicture, we are done.

Ah! I see. Thanks. We can save one CB.

[Pawel Osciak, 2014-08-20 10:34:12]

Oh, you rebased on top of the other CL... It won't work this way and it's also
difficult to review.
Please delete this rebased PS and upload only the diff without changes from the
other CL included. If you cannot do this, please commit the other CL first and
then rebase and upload again.
Thanks.

[Owen Lin, 2014-08-22 07:34:28]

I just rebased onto the TOT. PTAL. So many thanks.

[Pawel Osciak, 2014-08-22 08:49:43]

lgtm % nit

https://chromiumcodereview.appspot.com/465293002/diff/80001/content/common/gp...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://chromiumcodereview.appspot.com/465293002/diff/80001/content/common/gp...
content/common/gpu/media/video_decode_accelerator_unittest.cc:319: int
frames_at_render_;
Please document.

[Owen Lin, 2014-08-22 09:39:09]

The CQ bit was checked by owenlin@chromium.org

[commit-bot: I haz the power, 2014-08-22 09:39:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/owenlin@chromium.org/465293002/100001

[commit-bot: I haz the power, 2014-08-22 10:33:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[commit-bot: I haz the power, 2014-08-22 11:26:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-22 11:26:50]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Pawel Osciak, 2014-08-25 01:53:43]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2014-08-25 01:53:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/owenlin@chromium.org/465293002/100001

[commit-bot: I haz the power, 2014-08-25 02:46:51]

Committed patchset #6 (100001) as cc7dccb80ac4aba6762d960c718f56acc825b2f2

[commit-bot: I haz the power, 2014-09-10 02:32:56]

Patchset 6 (id:??) landed as
https://crrev.com/1cc19e420c43d6ec628a0b62bdcc86625d366a65
Cr-Commit-Position: refs/heads/master@{#291635}