Skip posting DecodeTask of resolution changing while resetting

Skip posting DecodeTask of resolution changing while resetting

The resolution changing event may happen while resetting (|kState_| is
kResetting) or flushing (kFlushing).

In this case we do not change state and post DecodeTask, otherwise
|kState_| will be no more kResetting/kFlushing and FinishReset()/FinishFlush() will not been
done. And the stream will still be resumed in FinishReset().

BUG=b/29930597
TEST=Loop run the following items to make sure resolution change smoothly while resetting and flushing.
1. xts google.exoplayer
2. cts AdaptivePlaybackTest#testH264_XXXXX
3. autotest video_VideoSeek.h264 and video_VideoSeek.h264.switchres

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

[johnylin1, 2016-07-15 06:48:51]

Description was changed from

==========
Skip posting DecodeTask of resolution changing while resetting

The resolution changing event may happen while resetting (|kState_| is
kResetting).

In this case we do not change state and post DecodeTask, otherwise
|kState_| will be no more kResetting and FinishReset() will not been
done. And the stream will still be resumed in FinishReset().

BUG=b/29930597
TEST=Loop run AdaptivePlaybackTest#testH264_adaptiveDrcEarlyEos with
logs and make sure everything's smooth even it encountered the case.
==========

to

==========
Skip posting DecodeTask of resolution changing while resetting

The resolution changing event may happen while resetting (|kState_| is
kResetting).

In this case we do not change state and post DecodeTask, otherwise
|kState_| will be no more kResetting and FinishReset() will not been
done. And the stream will still be resumed in FinishReset().

BUG=b/29930597
TEST=Loop run AdaptivePlaybackTest#testH264_adaptiveDrcEarlyEos with
logs and make sure everything's smooth even it encountered the case.
==========

[johnylin1, 2016-07-15 06:48:51]

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

[Owen Lin, 2016-07-19 06:21:48]

lgtm.

The bug is targeted to M53. We should merge it asap or remove the M53 hotlist
from the bug.

[Pawel Osciak, 2016-07-19 07:26:09]

https://chromiumcodereview.appspot.com/2151183002/diff/1/media/gpu/vaapi_vide...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2151183002/diff/1/media/gpu/vaapi_vide...
media/gpu/vaapi_video_decode_accelerator.cc:802: // change state and post
DeocdeTask(). The stream will be resumed in
s/DeocdeTask/DecodeTask/

https://chromiumcodereview.appspot.com/2151183002/diff/1/media/gpu/vaapi_vide...
media/gpu/vaapi_video_decode_accelerator.cc:804: if (state_ != kResetting) {
Could we be in kFlushing? Should we also consider that case here?

[johnylin1, 2016-07-19 08:04:41]

https://chromiumcodereview.appspot.com/2151183002/diff/1/media/gpu/vaapi_vide...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2151183002/diff/1/media/gpu/vaapi_vide...
media/gpu/vaapi_video_decode_accelerator.cc:804: if (state_ != kResetting) {
On 2016/07/19 07:26:08, Pawel Osciak wrote:
> Could we be in kFlushing? Should we also consider that case here?

Yes, I found out that the same issue may be encountered if resolution changing
event comes right between Flush() and FinishFlush(), then client will not be
notified FlushDone.

[johnylin1, 2016-07-20 02:50:43]

https://chromiumcodereview.appspot.com/2151183002/diff/20001/media/gpu/vaapi_...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2151183002/diff/20001/media/gpu/vaapi_...
media/gpu/vaapi_video_decode_accelerator.cc:816: if (state_ != kResetting &&
state_ != kFlushing) {
I ran overnight for AdaptivePlaybackTest#testH264_adaptiveDrcEarlyEos to make
sure that everything goes right if |state_| == kFlushing. Tests are all passed
(71 times).

Is there other test we need to verify for the reliability of this change?

[Owen Lin, 2016-07-20 03:19:21]

On 2016/07/20 02:50:43, johnylin1 wrote:
>
https://chromiumcodereview.appspot.com/2151183002/diff/20001/media/gpu/vaapi_...
> File media/gpu/vaapi_video_decode_accelerator.cc (right):
> 
>
https://chromiumcodereview.appspot.com/2151183002/diff/20001/media/gpu/vaapi_...
> media/gpu/vaapi_video_decode_accelerator.cc:816: if (state_ != kResetting &&
> state_ != kFlushing) {
> I ran overnight for AdaptivePlaybackTest#testH264_adaptiveDrcEarlyEos to make
> sure that everything goes right if |state_| == kFlushing. Tests are all passed
> (71 times).
> 
> Is there other test we need to verify for the reliability of this change?

Maybe other AdaptivePlaybackTest#testH264_XXXXX test cases
 as well as the xts test package: "google.exoplayer".

[Pawel Osciak, 2016-07-20 04:28:35]

https://chromiumcodereview.appspot.com/2151183002/diff/20001/media/gpu/vaapi_...
File media/gpu/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2151183002/diff/20001/media/gpu/vaapi_...
media/gpu/vaapi_video_decode_accelerator.cc:816: if (state_ != kResetting &&
state_ != kFlushing) {
On 2016/07/20 02:50:43, johnylin1 wrote:
> I ran overnight for AdaptivePlaybackTest#testH264_adaptiveDrcEarlyEos to make
> sure that everything goes right if |state_| == kFlushing. Tests are all passed
> (71 times).
> 
> Is there other test we need to verify for the reliability of this change?

Please also test with video_VideoSeek.

[johnylin1, 2016-07-21 03:12:07]

On 2016/07/20 04:28:35, Pawel Osciak wrote:
>
https://chromiumcodereview.appspot.com/2151183002/diff/20001/media/gpu/vaapi_...
> File media/gpu/vaapi_video_decode_accelerator.cc (right):
> 
>
https://chromiumcodereview.appspot.com/2151183002/diff/20001/media/gpu/vaapi_...
> media/gpu/vaapi_video_decode_accelerator.cc:816: if (state_ != kResetting &&
> state_ != kFlushing) {
> On 2016/07/20 02:50:43, johnylin1 wrote:
> > I ran overnight for AdaptivePlaybackTest#testH264_adaptiveDrcEarlyEos to
make
> > sure that everything goes right if |state_| == kFlushing. Tests are all
passed
> > (71 times).
> > 
> > Is there other test we need to verify for the reliability of this change?
> 
> Please also test with video_VideoSeek.

Thanks, I have loop-run the following:
1. xts google.exoplayer
2. cts AdaptivePlaybackTest#testH264_XXXXX
3. video_VideoSeek.h264 and video_VideoSeek.h264.switchres

I found a flaky crash issue of one test in google.exoplayer (b/30240830), but
it's not related to this change.
Except that this change seems reliable.

[johnylin1, 2016-07-21 04:08:58]

Description was changed from

==========
Skip posting DecodeTask of resolution changing while resetting

The resolution changing event may happen while resetting (|kState_| is
kResetting).

In this case we do not change state and post DecodeTask, otherwise
|kState_| will be no more kResetting and FinishReset() will not been
done. And the stream will still be resumed in FinishReset().

BUG=b/29930597
TEST=Loop run AdaptivePlaybackTest#testH264_adaptiveDrcEarlyEos with
logs and make sure everything's smooth even it encountered the case.
==========

to

==========
Skip posting DecodeTask of resolution changing while resetting

The resolution changing event may happen while resetting (|kState_| is
kResetting) or flushing (kFlushing).

In this case we do not change state and post DecodeTask, otherwise
|kState_| will be no more kResetting/kFlushing and FinishReset()/FinishFlush()
will not been
done. And the stream will still be resumed in FinishReset().

BUG=b/29930597
TEST=Loop run the following items to make sure resolution change smoothly while
resetting and flushing.
1. xts google.exoplayer
2. cts AdaptivePlaybackTest#testH264_XXXXX
3. autotest video_VideoSeek.h264 and video_VideoSeek.h264.switchres
==========

[Pawel Osciak, 2016-07-21 04:37:23]

lgtm

[Pawel Osciak, 2016-07-21 04:37:30]

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

[commit-bot: I haz the power, 2016-07-21 04:37:40]

Dry run: 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-07-21 05:49:37]

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

[commit-bot: I haz the power, 2016-07-21 05:49:42]

Dry run: This issue passed the CQ dry run.

[johnylin1, 2016-07-21 06:20:48]

The CQ bit was checked by johnylin@chromium.org

[johnylin1, 2016-07-21 06:20:49]

The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2151183002/#ps20001
(title: "Skip posting DecodeTask of resolution changing while resetting")

[commit-bot: I haz the power, 2016-07-21 06:21:16]

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-07-21 06:24:28]

Description was changed from

==========
Skip posting DecodeTask of resolution changing while resetting

The resolution changing event may happen while resetting (|kState_| is
kResetting) or flushing (kFlushing).

In this case we do not change state and post DecodeTask, otherwise
|kState_| will be no more kResetting/kFlushing and FinishReset()/FinishFlush()
will not been
done. And the stream will still be resumed in FinishReset().

BUG=b/29930597
TEST=Loop run the following items to make sure resolution change smoothly while
resetting and flushing.
1. xts google.exoplayer
2. cts AdaptivePlaybackTest#testH264_XXXXX
3. autotest video_VideoSeek.h264 and video_VideoSeek.h264.switchres
==========

to

==========
Skip posting DecodeTask of resolution changing while resetting

The resolution changing event may happen while resetting (|kState_| is
kResetting) or flushing (kFlushing).

In this case we do not change state and post DecodeTask, otherwise
|kState_| will be no more kResetting/kFlushing and FinishReset()/FinishFlush()
will not been
done. And the stream will still be resumed in FinishReset().

BUG=b/29930597
TEST=Loop run the following items to make sure resolution change smoothly while
resetting and flushing.
1. xts google.exoplayer
2. cts AdaptivePlaybackTest#testH264_XXXXX
3. autotest video_VideoSeek.h264 and video_VideoSeek.h264.switchres
==========

[commit-bot: I haz the power, 2016-07-21 06:24:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-21 06:25:48]

Description was changed from

==========
Skip posting DecodeTask of resolution changing while resetting

The resolution changing event may happen while resetting (|kState_| is
kResetting) or flushing (kFlushing).

In this case we do not change state and post DecodeTask, otherwise
|kState_| will be no more kResetting/kFlushing and FinishReset()/FinishFlush()
will not been
done. And the stream will still be resumed in FinishReset().

BUG=b/29930597
TEST=Loop run the following items to make sure resolution change smoothly while
resetting and flushing.
1. xts google.exoplayer
2. cts AdaptivePlaybackTest#testH264_XXXXX
3. autotest video_VideoSeek.h264 and video_VideoSeek.h264.switchres
==========

to

==========
Skip posting DecodeTask of resolution changing while resetting

The resolution changing event may happen while resetting (|kState_| is
kResetting) or flushing (kFlushing).

In this case we do not change state and post DecodeTask, otherwise
|kState_| will be no more kResetting/kFlushing and FinishReset()/FinishFlush()
will not been
done. And the stream will still be resumed in FinishReset().

BUG=b/29930597
TEST=Loop run the following items to make sure resolution change smoothly while
resetting and flushing.
1. xts google.exoplayer
2. cts AdaptivePlaybackTest#testH264_XXXXX
3. autotest video_VideoSeek.h264 and video_VideoSeek.h264.switchres

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

[commit-bot: I haz the power, 2016-07-21 06:25:49]

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