Add an additional VDA::Flush() mode to return all allocated buffers.

Add an additional VDA::Flush() mode to return all allocated buffers.

Currently, VDA::Flush() only returns to the Client PictureBuffers that
contain decoded frames, keeping any remaining PictureBuffers for later use.

Add a VDA::Config option to allow the Client to request a second VDA::Flush()
mode, in which the VDA returns all allocated PictureBuffers to the Client once
the Flush is done.

Also, add support for it in V4L2SVDA.

BUG=b/27780242
TEST=vdatest,crosvideo.appspot

[Pawel Osciak, 2016-03-22 08:02:57]

posciak@chromium.org changed reviewers:
+ kcwu@chromium.org, owenlin@chromium.org

[Pawel Osciak, 2016-03-22 08:02:57]

ptal

[kcwu, 2016-03-22 09:47:42]

lgtm; nits

https://codereview.chromium.org/1816203003/diff/1/content/common/gpu/media/fa...
File content/common/gpu/media/fake_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1816203003/diff/1/content/common/gpu/media/fa...
content/common/gpu/media/fake_video_decode_accelerator.cc:142: void
FakeVideoDecodeAccelerator::Flush(bool return_buffers) {
how about adding DCHECK(!return_buffers) for VDAs which don't support it?

https://codereview.chromium.org/1816203003/diff/1/content/common/gpu/media/vi...
File content/common/gpu/media/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/1816203003/diff/1/content/common/gpu/media/vi...
content/common/gpu/media/video_decode_accelerator_unittest.cc:790:
EXPECT_EQ(num_pictures_at_client_, 0);
EXPECT_EQ(0, num_pictures_at_client_);
expected first.

https://codereview.chromium.org/1816203003/diff/1/media/video/video_decode_ac...
File media/video/video_decode_accelerator.h (right):

https://codereview.chromium.org/1816203003/diff/1/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:210: // client. If |return_buffers| is
true, VDA must return all PictureBuffers it
s/return all PictureBuffers/return all PictureBuffers via PictureReady/

https://codereview.chromium.org/1816203003/diff/1/media/video/video_decode_ac...
media/video/video_decode_accelerator.h:215: virtual void Flush(bool
return_buffers) = 0;
How about move the description of |return_buffers| to after 'Can be used to
implement "end of stream" notification.' or into separate "Parameters:" section?
Otherwise I was confused that 'Can be used to implement "end of stream"
notification.' is related to |return_buffers|.

[Owen Lin, 2016-03-23 02:01:57]

lgtm. However, I thought you'll choose to put this option in config, where the
VDA has a chance to say it is not supported. It is not likely one would use
different options during decoding. What's the reason that changes your mind?

[Pawel Osciak, 2016-04-13 07:13:06]

Description was changed from

==========
Add a flag to VDA::Flush() to return all buffers when done.

Currently, VDA::Flush() only returns to the Client PictureBuffers that
contain decoded frames, keeping any remaining PictureBuffers for
later use.

Add a flag to VDA::Flush() to force the VDA to return all allocated
PictureBuffers to the Client once the Flush is done.

Also, add support for it in V4L2SVDA.

BUG=b/27780242
TEST=vdatest,crosvideo.appspot
==========

to

==========
Add an additional VDA::Flush() mode to return all allocated buffers.

Currently, VDA::Flush() only returns to the Client PictureBuffers that
contain decoded frames, keeping any remaining PictureBuffers for later use.

Add a VDA::Config option to allow the Client to request a second VDA::Flush()
mode, in which the VDA returns all allocated PictureBuffers to the Client once
the Flush is done.

Also, add support for it in V4L2SVDA.

BUG=b/27780242
TEST=vdatest,crosvideo.appspot
==========

[Pawel Osciak, 2016-04-13 07:13:26]

ptal

[Owen Lin, 2016-04-13 09:21:39]

https://chromiumcodereview.appspot.com/1816203003/diff/20001/media/video/vide...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1816203003/diff/20001/media/video/vide...
media/video/video_decode_accelerator.h:126: // PictureReady() before posting
NotifyFlushDone(), including buffers not
Actually, the whole VideoDecodeAccelerator runs in one thread. I mean the
methods of VDA as well as the callbacks in Client are called in this thread (and
we call it child thread). The fact that there is another decoder thread to help
sending/processing buffers is implementation details that each VDAImpl should
hide.

So the contract is simple, before Client::NotifyFlushDone() is called, all
buffers should be returned by NotifyPictureReady(). And after that, VDA should
start to use and hold any pictures sent by ReusePictureBuffer().

To make things clear I list two ideas in the shared document. Please take a look
and see if that is doable. But the point is to keep the above contract.

https://docs.google.com/a/google.com/document/d/1AuZXQDyDVcs1om5qzoQjBhtzKTEQ...

[Pawel Osciak, 2016-04-14 02:14:01]

https://chromiumcodereview.appspot.com/1816203003/diff/20001/media/video/vide...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1816203003/diff/20001/media/video/vide...
media/video/video_decode_accelerator.h:126: // PictureReady() before posting
NotifyFlushDone(), including buffers not
On 2016/04/13 09:21:39, Owen Lin wrote:
> Actually, the whole VideoDecodeAccelerator runs in one thread. I mean the
> methods of VDA as well as the callbacks in Client are called in this thread
(and
> we call it child thread). The fact that there is another decoder thread to
help
> sending/processing buffers is implementation details that each VDAImpl should
> hide.

That's true, however VDA is also an IPC interface and calls are IPC messages and
can be posted from and executed in separate worlds (e.g. different processes).
How it works on the GPU process side and its details is not applicable in all
cases. The only thing we can guarantee is the ordering of IPC messages, but not
when they arrive or when they execute and how.


> So the contract is simple, before Client::NotifyFlushDone() is called, all
> buffers should be returned by NotifyPictureReady(). And after that, VDA should
> start to use and hold any pictures sent by ReusePictureBuffer().

The Client and VDA are in different processes. The only way for the Client to
guarantee that VDA will keep the buffers returned to it after flush sequence, is
to wait for NotifyFlushDone() before actually posting any ReusePictureBuffers().
There is no way for Client or the VDA to know or control when a message arrives
on the other side, apart from not posting it, or posting it only after receiving
the message that has to precede it.

The problem is, there is no notification Client->VDA that it consumed the
NotifyFlushDone, so VDA cannot govern this. Instead, the Client must assume that
any ReusePictureBuffer() call it posts after calling Flush() and before it gets
NotifyFlushDone() may behave in either way. Only if Client posts RPB() after
getting NotifyFlushDone(), it can guarantee VDA will keep the buffer.

[Pawel Osciak, 2016-04-14 02:17:26]

posciak@chromium.org changed reviewers:
+ sandersd@chromium.org

[Pawel Osciak, 2016-04-14 02:17:27]

sandersd@chromium.org: Please review changes in media/.

Thanks!

[Owen Lin, 2016-04-14 02:57:17]

https://chromiumcodereview.appspot.com/1816203003/diff/20001/media/video/vide...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1816203003/diff/20001/media/video/vide...
media/video/video_decode_accelerator.h:126: // PictureReady() before posting
NotifyFlushDone(), including buffers not
On 2016/04/14 02:14:01, Pawel Osciak wrote:
> On 2016/04/13 09:21:39, Owen Lin wrote:
> > Actually, the whole VideoDecodeAccelerator runs in one thread. I mean the
> > methods of VDA as well as the callbacks in Client are called in this thread
> (and
> > we call it child thread). The fact that there is another decoder thread to
> help
> > sending/processing buffers is implementation details that each VDAImpl
should
> > hide.
> 
> That's true, however VDA is also an IPC interface and calls are IPC messages
and
> can be posted from and executed in separate worlds (e.g. different processes).
> How it works on the GPU process side and its details is not applicable in all
> cases. The only thing we can guarantee is the ordering of IPC messages, but
not
> when they arrive or when they execute and how.
> 
> 
> > So the contract is simple, before Client::NotifyFlushDone() is called, all
> > buffers should be returned by NotifyPictureReady(). And after that, VDA
should
> > start to use and hold any pictures sent by ReusePictureBuffer().
> 
> The Client and VDA are in different processes. The only way for the Client to
> guarantee that VDA will keep the buffers returned to it after flush sequence,
is
> to wait for NotifyFlushDone() before actually posting any
ReusePictureBuffers().
> There is no way for Client or the VDA to know or control when a message
arrives
> on the other side, apart from not posting it, or posting it only after
receiving
> the message that has to precede it.
> 
> The problem is, there is no notification Client->VDA that it consumed the
> NotifyFlushDone, so VDA cannot govern this. Instead, the Client must assume
that
> any ReusePictureBuffer() call it posts after calling Flush() and before it
gets
> NotifyFlushDone() may behave in either way. Only if Client posts RPB() after
> getting NotifyFlushDone(), it can guarantee VDA will keep the buffer.

I see. You're right, I only think about the the use case for ArcGVDA which also
runs in the GPU process. However this make things difficult to handle. Client
must keep returning buffers since it doesn't know how many buffers are required
to finish the flush. On the other hand, those returned buffers could be hold
because VDA may already post NotifyFlushDone(). That leads us back to the
original issue: ArcGVDA may not have a buffer to send the EOS flag.

How about adding a new function like ResumeFromFlush() in VDA, before it is
called, VDA just keeps returning picture buffers. Otherwise, I would suggest we
just keeping one more picture buffer in ArcGVDA and use it to send back the EOS
flag as a temporary solution.