Use explicit flush for ArcVideoAccelerator.

Use explicit flush for ArcVideoAccelerator.

Change to use a explicit flush() function instead of a buffer with
special EOS flag. The flush handling (EOS) is different between
Android and Chromium. We decide to make the ARC interface more
similar to the chromium VDA interface, which calls Flush() explicitly
to notify the End-Of-Stream event.

BUG=b/27780242
TEST=Play video in ApiDemo in Arc

Committed: https://crrev.com/cf3c4bf05a5b2f39a93455a8257f99248b6b097d
Cr-Commit-Position: refs/heads/master@{#394692}

[Owen Lin, 2016-05-03 03:14:58]

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

[Owen Lin, 2016-05-03 03:14:59]

PTAL

[kcwu, 2016-05-03 09:07:05]

lgtm

[Owen Lin, 2016-05-06 05:10:30]

owenlin@chromium.org changed reviewers:
+ dcheng@chromium.org

[Owen Lin, 2016-05-06 05:10:30]

Hi dcheng@, please help to review the mojom file. Thanks.

[dcheng, 2016-05-07 06:18:11]

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_gpu_video_de...
File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_gpu_video_de...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:218:
buffers_pending_import_[index].release(), true);
Out of curiosity, how does this actually get fd get closed eventually?
base::FileDescriptor's "ownership" is merely advisory.

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_video_accele...
File chrome/gpu/arc_video_accelerator.h (right):

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_video_accele...
chrome/gpu/arc_video_accelerator.h:32: uint32_t flags = 0;     // Not used.
Why do we need to keep this unused field?

https://codereview.chromium.org/1945543002/diff/1/components/arc/common/video...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/1945543002/diff/1/components/arc/common/video...
components/arc/common/video_accelerator.mojom:67: Flush@6();
Out of curiosity, how come we don't do something like Flush@6() => (), so the
caller can just wait for the return callback?

[Owen Lin, 2016-05-09 07:04:41]

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_gpu_video_de...
File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_gpu_video_de...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:218:
buffers_pending_import_[index].release(), true);
On 2016/05/07 06:18:11, dcheng wrote:
> Out of curiosity, how does this actually get fd get closed eventually?
> base::FileDescriptor's "ownership" is merely advisory.

posciak@ should know more about why choosing "GpuMemoryBufferHandle" to pass the
FD. In my understanding, VDA is not an interface for Posix system only. Using
GpuMemoryBufferHandle is a cross platform solution.

However, we only use it to pass the FD. In the implementation of
ImportBufferForPicture, the FD will be managed by ScopedFD again.

For example: 
https://code.google.com/p/chromium/codesearch#chromium/src/media/gpu/v4l2_sli...

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_video_accele...
File chrome/gpu/arc_video_accelerator.h (right):

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_video_accele...
chrome/gpu/arc_video_accelerator.h:32: uint32_t flags = 0;     // Not used.
On 2016/05/07 06:18:11, dcheng wrote:
> Why do we need to keep this unused field?

Just get a feeling it will be used soon. Let's remove it and add back when we
really need it.

https://codereview.chromium.org/1945543002/diff/1/components/arc/common/video...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/1945543002/diff/1/components/arc/common/video...
components/arc/common/video_accelerator.mojom:67: Flush@6();
On 2016/05/07 06:18:11, dcheng wrote:
> Out of curiosity, how come we don't do something like Flush@6() => (), so the
> caller can just wait for the return callback?

Just make it similar to the VDA interface:

https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_...

[Pawel Osciak, 2016-05-09 07:27:39]

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_gpu_video_de...
File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_gpu_video_de...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:213: if
(buffers_pending_import_[index].is_valid()) {
Could you please add a comment explaining that if is_valid() == true, we
received this buffer from the Client via Bind*(), but this is the first time we
are passing it to the VDA, so need to import first?

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_gpu_video_de...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:214:
std::vector<gfx::GpuMemoryBufferHandle> buffers;
Would std::vector<gfx::GpuMemoryBufferHandle> buffers(1); work?

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_gpu_video_de...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:218:
buffers_pending_import_[index].release(), true);
On 2016/05/09 07:04:41, Owen Lin wrote:
> On 2016/05/07 06:18:11, dcheng wrote:
> > Out of curiosity, how does this actually get fd get closed eventually?
> > base::FileDescriptor's "ownership" is merely advisory.
> 
> posciak@ should know more about why choosing "GpuMemoryBufferHandle" to pass
the
> FD. In my understanding, VDA is not an interface for Posix system only. Using
> GpuMemoryBufferHandle is a cross platform solution.
> 
> However, we only use it to pass the FD. In the implementation of
> ImportBufferForPicture, the FD will be managed by ScopedFD again.
> 
> For example: 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/gpu/v4l2_sli...

The contract is that the implementation of ImportBufferForPicture() will take
ownership and close the fd (please see
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_...).

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_video_accele...
File chrome/gpu/arc_video_accelerator.h (right):

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_video_accele...
chrome/gpu/arc_video_accelerator.h:145: // to client by OnBufferDone(),
Client::OnFlushDone() will be called.
I think this is after all frames pending decode and/or display have been
returned, but not specifically all output buffers (we won't get empty buffers
back)?

[dcheng, 2016-05-09 21:22:00]

https://codereview.chromium.org/1945543002/diff/1/components/arc/common/video...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/1945543002/diff/1/components/arc/common/video...
components/arc/common/video_accelerator.mojom:67: Flush@6();
On 2016/05/09 at 07:04:41, Owen Lin wrote:
> On 2016/05/07 06:18:11, dcheng wrote:
> > Out of curiosity, how come we don't do something like Flush@6() => (), so
the
> > caller can just wait for the return callback?
> 
> Just make it similar to the VDA interface:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_...

Wouldn't the code be simpler though if we just used the appropriate mojo
primitives? Is there some advantage to having two separate interfaces for one
logical call that I'm missing?

[Owen Lin, 2016-05-10 08:26:35]

PTAL. Thanks.

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_gpu_video_de...
File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_gpu_video_de...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:213: if
(buffers_pending_import_[index].is_valid()) {
On 2016/05/09 07:27:38, Pawel Osciak wrote:
> Could you please add a comment explaining that if is_valid() == true, we
> received this buffer from the Client via Bind*(), but this is the first time
we
> are passing it to the VDA, so need to import first?

Done.

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_gpu_video_de...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:214:
std::vector<gfx::GpuMemoryBufferHandle> buffers;
On 2016/05/09 07:27:38, Pawel Osciak wrote:
> Would std::vector<gfx::GpuMemoryBufferHandle> buffers(1); work?

I will use the brace initialization syntax (suggested by dcheng) instead.

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_video_accele...
File chrome/gpu/arc_video_accelerator.h (right):

https://codereview.chromium.org/1945543002/diff/1/chrome/gpu/arc_video_accele...
chrome/gpu/arc_video_accelerator.h:145: // to client by OnBufferDone(),
Client::OnFlushDone() will be called.
On 2016/05/09 07:27:39, Pawel Osciak wrote:
> I think this is after all frames pending decode and/or display have been
> returned, but not specifically all output buffers (we won't get empty buffers
> back)?

Comments modified accordingly.

https://codereview.chromium.org/1945543002/diff/1/components/arc/common/video...
File components/arc/common/video_accelerator.mojom (right):

https://codereview.chromium.org/1945543002/diff/1/components/arc/common/video...
components/arc/common/video_accelerator.mojom:67: Flush@6();
On 2016/05/09 21:22:00, dcheng wrote:
> On 2016/05/09 at 07:04:41, Owen Lin wrote:
> > On 2016/05/07 06:18:11, dcheng wrote:
> > > Out of curiosity, how come we don't do something like Flush@6() => (), so
> the
> > > caller can just wait for the return callback?
> > 
> > Just make it similar to the VDA interface:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/video/video_...
> 
> Wouldn't the code be simpler though if we just used the appropriate mojo
> primitives? Is there some advantage to having two separate interfaces for one
> logical call that I'm missing?

I think you're right that we can use mojo primitives for Flush() -->
onFlushDone() or Reset()--> onResetDone().

But I don't see much benefits/difference. We still need the interface for error
handling and other callbacks. Adding these completion callbacks to the interface
seems a better idea. Otherwise, in Chromium side, we need to keep those
callbacks and invoke them when we are notified by
VDA::Client::Notify(Flush|Reset)Done().

In addition, that also makes people who are familiar with VDA easy to
match/understand the code.

The question makes me to think why VDA was not designed as using
Flush(base::Closure callback). But I don't see strong reason/benefit to change
it.

[Pawel Osciak, 2016-05-10 09:20:43]

lgtm

[dcheng, 2016-05-10 17:06:58]

LGTM.

I think the interface looks funny, but it is what it is.

[dcheng, 2016-05-10 23:00:11]

https://codereview.chromium.org/1945543002/diff/60001/chrome/gpu/arc_gpu_vide...
File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1945543002/diff/60001/chrome/gpu/arc_gpu_vide...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:222: index,
std::vector<gfx::GpuMemoryBufferHandle>{handle});
Also, just write {handle} here (no std::vector<gfx::GMBH>)

[Owen Lin, 2016-05-11 01:49:57]

https://codereview.chromium.org/1945543002/diff/60001/chrome/gpu/arc_gpu_vide...
File chrome/gpu/arc_gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1945543002/diff/60001/chrome/gpu/arc_gpu_vide...
chrome/gpu/arc_gpu_video_decode_accelerator.cc:222: index,
std::vector<gfx::GpuMemoryBufferHandle>{handle});
On 2016/05/10 23:00:11, dcheng wrote:
> Also, just write {handle} here (no std::vector<gfx::GMBH>)

Done. Thanks.

[Owen Lin, 2016-05-11 01:50:42]

The CQ bit was checked by owenlin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-11 01:51:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1945543002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1945543002/80001

[commit-bot: I haz the power, 2016-05-11 02:35:59]

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

[commit-bot: I haz the power, 2016-05-11 02:36:00]

Dry run: This issue passed the CQ dry run.

[Owen Lin, 2016-05-19 05:23:24]

The CQ bit was checked by owenlin@chromium.org

[Owen Lin, 2016-05-19 05:23:24]

The patchset sent to the CQ was uploaded after l-g-t-m from kcwu@chromium.org,
dcheng@chromium.org, posciak@chromium.org
Link to the patchset: https://codereview.chromium.org/1945543002/#ps120001
(title: "rebase and update version")

[commit-bot: I haz the power, 2016-05-19 05:23:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1945543002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1945543002/120001

[commit-bot: I haz the power, 2016-05-19 06:11:30]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-19 06:12:54]

Description was changed from

==========
Use explicit flush for ArcVideoAccelerator.

Change to use a explicit flush() function instead of a buffer with
special EOS flag. The flush handling (EOS) is different between
Android and Chromium. We decide to make the ARC interface more
similar to the chromium VDA interface, which calls Flush() explicitly
to notify the End-Of-Stream event.

BUG=b/27780242
TEST=Play video in ApiDemo in Arc
==========

to

==========
Use explicit flush for ArcVideoAccelerator.

Change to use a explicit flush() function instead of a buffer with
special EOS flag. The flush handling (EOS) is different between
Android and Chromium. We decide to make the ARC interface more
similar to the chromium VDA interface, which calls Flush() explicitly
to notify the End-Of-Stream event.

BUG=b/27780242
TEST=Play video in ApiDemo in Arc

Committed: https://crrev.com/cf3c4bf05a5b2f39a93455a8257f99248b6b097d
Cr-Commit-Position: refs/heads/master@{#394692}
==========

[commit-bot: I haz the power, 2016-05-19 06:12:55]

Patchset 7 (id:??) landed as
https://crrev.com/cf3c4bf05a5b2f39a93455a8257f99248b6b097d
Cr-Commit-Position: refs/heads/master@{#394692}