VDA: document the behavior of calling Reset before NotifyFlushDone.

VDA: document the behavior of calling Reset before NotifyFlushDone.

From video_decode_accelerator.h, it's not clear if it's valid to
call Reset before NotifyFlushDone. Currently ARC may call Reset
before NotifyFlushDone is back. We need to document this and add
a test for it.

BUG=671944
TEST=None

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

[wuchengli, 2016-12-07 07:12:22]

Description was changed from

==========
VDA: document the behavior of calling Reset before NotifyFlushDone.

BUG=671944
TEST=None
==========

to

==========
VDA: document the behavior of calling Reset before NotifyFlushDone.

From video_decode_accelerator.h, it's not clear if it's valid to
call Reset before NotifyFlushDone. Currently ARC may call Reset
before NotifyFlushDone is back. We need to document this and add
a test for it.

BUG=671944
TEST=None
==========

[wuchengli, 2016-12-07 07:12:40]

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

[wuchengli, 2016-12-07 07:13:18]

wuchengli@chromium.org changed reviewers:
+ xhwang@chromium.org

[wuchengli, 2016-12-07 07:15:22]

wuchengli@chromium.org changed reviewers:
+ dalecurtis@chromium.org, hubbe@chromium.org, jbauman@chromium.org,
liberato@chromium.org, sandersd@chromium.org, watk@chromium.org

[wuchengli, 2016-12-07 07:15:23]

PTAL.

[wuchengli, 2016-12-07 08:04:45]

PTAL.

[kcwu, 2016-12-07 08:20:25]

lgtm

[liberato (no reviews please), 2016-12-07 18:02:41]

AVDA, at least, doesn't seem to allow reset during flush, if i remember.  it can
fail to clear out the flushing state sometimes.  i also think that there are
some dchecks that get mad about trying to start another flush while one is
pending, in some of the Reset branches that do that.

gpu_video_decoder enforces a wait (busy-post, actually) in Reset if a flush is
in progress.  otherwise, i think that it could make avda confused.

does ARC go through GVD, or does it talk to the VDA directly?

[wuchengli, 2016-12-08 09:21:34]

On 2016/12/07 18:02:41, liberato wrote:
> AVDA, at least, doesn't seem to allow reset during flush, if i remember.  it
can
> fail to clear out the flushing state sometimes.  i also think that there are
> some dchecks that get mad about trying to start another flush while one is
> pending, in some of the Reset branches that do that.
> 
> gpu_video_decoder enforces a wait (busy-post, actually) in Reset if a flush is
> in progress.  otherwise, i think that it could make avda confused.
> 
> does ARC go through GVD, or does it talk to the VDA directly?
ARC talks to the VDA directly (using GpuVideoDecodeAcceleratorFactory).

[wuchengli, 2016-12-08 09:23:20]

Does anyone know the behavior of DxvaVDA and VtVDA if a client calls Reset
before NotifyFlushDone?

[jbauman, 2016-12-08 09:36:55]

On 2016/12/08 09:23:20, wuchengli wrote:
> Does anyone know the behavior of DxvaVDA and VtVDA if a client calls Reset
> before NotifyFlushDone?

I fixed this for dxvavda in r423058, as it seems like this happens in
video_decode_accelerator_unittest.

[sandersd (OOO until July 31), 2016-12-08 19:40:49]

On 2016/12/08 09:23:20, wuchengli wrote:
> Does anyone know the behavior of DxvaVDA and VtVDA if a client calls Reset
> before NotifyFlushDone?

VTVDA maintains a queue of pending operations and services them in order (the
|pending_flush_tasks_| queue). It will still call NotifyFlushDone() after a
Reset(), and in fact any sequence of decode/flush/reset calls will be processed
fully ordered.

Destroy() is handled by the same mechanism, but behaves slightly differently so
that no callbacks will be made.

This behavior is within the new API documentation, so VTVDA should be okay with
this change. The new documentation isn't clear what should happen if Reset() is
called twice, for example; you may want to clarify that.

The in-planning-phase VDAv2 design uses base::Callbacks passed into
Flush()/Reset() so that the responses can be linked to the requests easily, and
will allow arbitrary sequences without waiting for the completion callbacks.

AVDA can probably just be excepted from the rule, since the number of AVDA
clients is small and are all fully controlled by us (us being Chromium). We can
make sure that those clients to not require this behavior.

[wuchengli, 2016-12-12 05:01:01]

On 2016/12/08 19:40:49, sandersd (OOO until Dec 14) wrote:
> On 2016/12/08 09:23:20, wuchengli wrote:
> > Does anyone know the behavior of DxvaVDA and VtVDA if a client calls Reset
> > before NotifyFlushDone?
> 
> VTVDA maintains a queue of pending operations and services them in order (the
> |pending_flush_tasks_| queue). It will still call NotifyFlushDone() after a
> Reset(), and in fact any sequence of decode/flush/reset calls will be
processed
> fully ordered.
> 
> Destroy() is handled by the same mechanism, but behaves slightly differently
so
> that no callbacks will be made.
> 
> This behavior is within the new API documentation, so VTVDA should be okay
with
> this change. The new documentation isn't clear what should happen if Reset()
is
> called twice, for example; you may want to clarify that.
Calling Reset twice is a separate topic. I won't add it in this patch.
> 
> The in-planning-phase VDAv2 design uses base::Callbacks passed into
> Flush()/Reset() so that the responses can be linked to the requests easily,
and
> will allow arbitrary sequences without waiting for the completion callbacks.
> 
> AVDA can probably just be excepted from the rule, since the number of AVDA
> clients is small and are all fully controlled by us (us being Chromium). We
can
> make sure that those clients to not require this behavior.
Alright. All VDA except AVDA can handle Reset before NotifyFlushDone. Also
we can make sure clients of AVDA do not require this. It is OK to add this
documentation. Please LGT-M the patch. Thanks.

[Pawel Osciak, 2016-12-12 06:19:05]

Since the existing wording doesn't say we are not allowed to call Reset during
Flush, it's probably a good idea to specify this in detail, even if AVDA is not
exactly compliant, I agree it could be an exception given the above.

[Pawel Osciak, 2016-12-12 06:19:57]

On 2016/12/12 06:19:05, Pawel Osciak wrote:
> Since the existing wording doesn't say we are not allowed to call Reset during
> Flush, it's probably a good idea to specify this in detail, even if AVDA is
not
> exactly compliant, I agree it could be an exception given the above.

With that, lgtm

[wuchengli, 2016-12-14 09:28:36]

sandersd: are you back from vacation? Can you take a look? Thanks.

[sandersd (OOO until July 31), 2016-12-14 19:10:30]

lgtm

[wuchengli, 2016-12-15 05:01:32]

The CQ bit was checked by wuchengli@chromium.org

[commit-bot: I haz the power, 2016-12-15 05:02:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 07:22:58]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481778092438130,
"parent_rev": "0060bf7e5683085ea78928f31e69ea6fb53d1154", "commit_rev":
"b5013f2f578b6c3f764016cb717d8d58545dc546"}

[commit-bot: I haz the power, 2016-12-15 07:23:23]

Description was changed from

==========
VDA: document the behavior of calling Reset before NotifyFlushDone.

From video_decode_accelerator.h, it's not clear if it's valid to
call Reset before NotifyFlushDone. Currently ARC may call Reset
before NotifyFlushDone is back. We need to document this and add
a test for it.

BUG=671944
TEST=None
==========

to

==========
VDA: document the behavior of calling Reset before NotifyFlushDone.

From video_decode_accelerator.h, it's not clear if it's valid to
call Reset before NotifyFlushDone. Currently ARC may call Reset
before NotifyFlushDone is back. We need to document this and add
a test for it.

BUG=671944
TEST=None

Review-Url: https://codereview.chromium.org/2556873003
==========

[commit-bot: I haz the power, 2016-12-15 07:23:25]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-15 07:26:43]

Description was changed from

==========
VDA: document the behavior of calling Reset before NotifyFlushDone.

From video_decode_accelerator.h, it's not clear if it's valid to
call Reset before NotifyFlushDone. Currently ARC may call Reset
before NotifyFlushDone is back. We need to document this and add
a test for it.

BUG=671944
TEST=None

Review-Url: https://codereview.chromium.org/2556873003
==========

to

==========
VDA: document the behavior of calling Reset before NotifyFlushDone.

From video_decode_accelerator.h, it's not clear if it's valid to
call Reset before NotifyFlushDone. Currently ARC may call Reset
before NotifyFlushDone is back. We need to document this and add
a test for it.

BUG=671944
TEST=None

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

[commit-bot: I haz the power, 2016-12-15 07:26:44]

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