Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2713863002:
VDA unittest: test for calling Reset before NotifyFlushDone (Closed)
DescriptionVDA unittest: test for calling Reset before NotifyFlushDone
For VDA it is valid to call Reset() before receiving NotifyResetDone() and VDA
should cancel the flush. Add a test to check this scenario.
BUG=674828
TEST= run ResetBeforeNotifyFlushDone test on debug build of
Samus: passed.
Elm:
(1) Test is passed on ToT build.
(2) Revert https://codereview.chromium.org/2506363004, test hits
dcheck in V4L2VideoDecodeAccelerator::StartDevicePoll()
https://cs.chromium.org/chromium/src/media/gpu/v4l2_video_decode_accelerator.cc?dr&l=1840
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2713863002
Cr-Commit-Position: refs/heads/master@{#456990}
Committed: https://chromium.googlesource.com/chromium/src/+/ccc25c33b711f0d546fe4edf6b879489ab09cde4
Patch Set 1 #Patch Set 2 : VDA unittest: test for calling Reset before NotifyFlushDone #
Total comments: 4
Patch Set 3 : VDA unittest: test for calling Reset before NotifyFlushDone #
Total comments: 15
Patch Set 4 : VDA unittest: test for calling Reset before NotifyFlushDone #Patch Set 5 : VDA unittest: test for calling Reset before NotifyFlushDone #
Total comments: 4
Patch Set 6 : VDA unittest: test for calling Reset before NotifyFlushDone #Patch Set 7 : Debug patch for trybot win_chromium_rel_ng #Patch Set 8 : VDA unittest: test for calling Reset before NotifyFlushDone #Patch Set 9 : Debug patch for trybot win_chromium_rel_ng #Patch Set 10 : Debug patch #Patch Set 11 : VDA unittest: test for calling Reset before NotifyFlushDone #
Total comments: 2
Patch Set 12 : VDA unittest: test for calling Reset before NotifyFlushDone #Messages
Total messages: 46 (25 generated)
Description was changed from ========== VDA unittest: test for calling Reset before NotifyFlushDone For VDA it is valid to call Reset() before receiving NotifyResetDone() and VDA should cancel the flush. Add a test to check this scenario. BUG=674828 TEST= run ResetBeforeNotifyFlushDone test on debug build of Elm. (1) Test is passed on ToT build. (2) Revert https://codereview.chromium.org/2506363004, test hits dcheck in V4L2VideoDecodeAccelerator::StartDevicePoll() https://cs.chromium.org/chromium/src/media/gpu/v4l2_video_decode_accelerator.... ========== to ========== VDA unittest: test for calling Reset before NotifyFlushDone For VDA it is valid to call Reset() before receiving NotifyResetDone() and VDA should cancel the flush. Add a test to check this scenario. BUG=674828 TEST= run ResetBeforeNotifyFlushDone test on debug build of Elm. (1) Test is passed on ToT build. (2) Revert https://codereview.chromium.org/2506363004, test hits dcheck in V4L2VideoDecodeAccelerator::StartDevicePoll() https://cs.chromium.org/chromium/src/media/gpu/v4l2_video_decode_accelerator.... CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
johnylin@chromium.org changed reviewers: + posciak@chromium.org, wuchengli@chromium.org
https://codereview.chromium.org/2713863002/diff/20001/media/gpu/video_decode_... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2713863002/diff/20001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:830: SetState(CS_FLUSHED); Would it make sense to unify this code with other similar Reset() cases in a helper method? https://codereview.chromium.org/2713863002/diff/20001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:1483: else if (video_file->reset_after_frame_num != Would it perhaps be possible to improve both checks to check whether we decoded at least the number of frames returned/decoded before calling the reset plus num_frames (as we replay the whole stream from the beginning to the end after reset)?
https://codereview.chromium.org/2713863002/diff/20001/media/gpu/video_decode_... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2713863002/diff/20001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:830: SetState(CS_FLUSHED); On 2017/02/24 07:49:42, Pawel Osciak wrote: > Would it make sense to unify this code with other similar Reset() cases in a > helper method? Done. https://codereview.chromium.org/2713863002/diff/20001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:1483: else if (video_file->reset_after_frame_num != On 2017/02/24 07:49:41, Pawel Osciak wrote: > Would it perhaps be possible to improve both checks to check whether we decoded > at least the number of frames returned/decoded before calling the reset plus > num_frames (as we replay the whole stream from the beginning to the end after > reset)? Done.
https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:484: // Reset the associated decoder helpr. helper https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:819: void GLRenderingVDAClient::ResetDecoder() { The name is not very good. There are three other decoder_->Reset(); not calling this function. Is it possible to call ResetDecoder() in every decoder_->Reset();? If not, num_decoded_frames_before_reset_ is only set correctly sometimes. I also feel num_decoded_frames_before_reset_ is not the main thing we want to test here. Removing it can simplify the code a bit. https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:824: if (state_ == CS_FLUSHED) I don't understand. Why do we need this line? https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:861: // NotifyFlushDone(). But if client gets, it should be always before s/gets/gets here/ https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:863: LOG_ASSERT(state_ == CS_RESETTING); When state_ != CS_RESETTING, it can be a bug of driver. We shouldn't crash the test. Just ASSERT_EQ it and remove the return. https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:1091: decoder_->Reset(); I'm curious. Why the state doesn't need to be set to CS_RESETTING here?
https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:819: void GLRenderingVDAClient::ResetDecoder() { On 2017/03/02 09:44:05, wuchengli wrote: > The name is not very good. There are three other decoder_->Reset(); not calling > this function. Is it possible to call ResetDecoder() in every > decoder_->Reset();? If not, num_decoded_frames_before_reset_ is only set > correctly sometimes. I also feel num_decoded_frames_before_reset_ is not the > main thing we want to test here. Removing it can simplify the code a bit. As discussed, let's use ResetDecoderAfterFlush. https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:822: num_decoded_frames_before_reset_ = num_decoded_frames_; It's more consistent to set num_decoded_frames_before_reset_ every time we call decoder_->Reset(). But I feel it's lots of code. Pawel. How about removing this? https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:824: if (state_ == CS_FLUSHED) On 2017/03/02 09:44:04, wuchengli wrote: > I don't understand. Why do we need this line? As discussed, this can be removed.
https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:484: // Reset the associated decoder helpr. On 2017/03/02 09:44:04, wuchengli wrote: > helper Done. https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:819: void GLRenderingVDAClient::ResetDecoder() { On 2017/03/02 10:07:12, wuchengli wrote: > On 2017/03/02 09:44:05, wuchengli wrote: > > The name is not very good. There are three other decoder_->Reset(); not > calling > > this function. Is it possible to call ResetDecoder() in every > > decoder_->Reset();? If not, num_decoded_frames_before_reset_ is only set > > correctly sometimes. I also feel num_decoded_frames_before_reset_ is not the > > main thing we want to test here. Removing it can simplify the code a bit. > As discussed, let's use ResetDecoderAfterFlush. Done. https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:822: num_decoded_frames_before_reset_ = num_decoded_frames_; On 2017/03/02 10:07:12, wuchengli wrote: > It's more consistent to set num_decoded_frames_before_reset_ every time we call > decoder_->Reset(). But I feel it's lots of code. Pawel. How about removing this? After discussed, we gonna add this check after refactoring VDA unittest. Remove it in this CL. https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:824: if (state_ == CS_FLUSHED) On 2017/03/02 10:07:11, wuchengli wrote: > On 2017/03/02 09:44:04, wuchengli wrote: > > I don't understand. Why do we need this line? > As discussed, this can be removed. Done. https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:861: // NotifyFlushDone(). But if client gets, it should be always before On 2017/03/02 09:44:05, wuchengli wrote: > s/gets/gets here/ Done. https://codereview.chromium.org/2713863002/diff/40001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:863: LOG_ASSERT(state_ == CS_RESETTING); On 2017/03/02 09:44:04, wuchengli wrote: > When state_ != CS_RESETTING, it can be a bug of driver. We shouldn't crash the > test. Just ASSERT_EQ it and remove the return. Done.
https://codereview.chromium.org/2713863002/diff/80001/media/gpu/video_decode_... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2713863002/diff/80001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:481: // Reset the associated decoder after flushing helper. s/helper// https://codereview.chromium.org/2713863002/diff/80001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:816: DCHECK_GE(remaining_play_throughs_, 0); Do we need if (decoder_deleted()) return; here? The original code has it. I think we don't it because line 849 only has it?
https://codereview.chromium.org/2713863002/diff/80001/media/gpu/video_decode_... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2713863002/diff/80001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:481: // Reset the associated decoder after flushing helper. On 2017/03/07 14:24:02, wuchengli wrote: > s/helper// Done. https://codereview.chromium.org/2713863002/diff/80001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:816: DCHECK_GE(remaining_play_throughs_, 0); On 2017/03/07 14:24:02, wuchengli wrote: > Do we need if (decoder_deleted()) return; here? The original code has it. I > think we don't it because line 849 only has it? Yes, in original code since line849 has it so actually it is redundant. I don't think we need to check again inside ResetDecoderAfterFlush()
lgtm
Please test on at least one intel cros device. Thanks.
Description was changed from ========== VDA unittest: test for calling Reset before NotifyFlushDone For VDA it is valid to call Reset() before receiving NotifyResetDone() and VDA should cancel the flush. Add a test to check this scenario. BUG=674828 TEST= run ResetBeforeNotifyFlushDone test on debug build of Elm. (1) Test is passed on ToT build. (2) Revert https://codereview.chromium.org/2506363004, test hits dcheck in V4L2VideoDecodeAccelerator::StartDevicePoll() https://cs.chromium.org/chromium/src/media/gpu/v4l2_video_decode_accelerator.... CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== VDA unittest: test for calling Reset before NotifyFlushDone For VDA it is valid to call Reset() before receiving NotifyResetDone() and VDA should cancel the flush. Add a test to check this scenario. BUG=674828 TEST= run ResetBeforeNotifyFlushDone test on debug build of Samus: passed. Elm: (1) Test is passed on ToT build. (2) Revert https://codereview.chromium.org/2506363004, test hits dcheck in V4L2VideoDecodeAccelerator::StartDevicePoll() https://cs.chromium.org/chromium/src/media/gpu/v4l2_video_decode_accelerator.... CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
On 2017/03/08 10:05:53, wuchengli wrote: > Please test on at least one intel cros device. Thanks. tested on Samus, thanks.
The CQ bit was checked by johnylin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_optional_gpu_tests_rel on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by johnylin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by johnylin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Have you found out why win bot failed?
Have you found out why win bot failed?
On 2017/03/13 14:08:53, wuchengli wrote: > Have you found out why win bot failed? Yes, the original code is failed for dxva_vda (windows). In DXVA_VDA::Reset(), it will directly call NotifyFlushDone() if state is kFlushing, so patch 6 will fail. https://cs.chromium.org/chromium/src/media/gpu/dxva_video_decode_accelerator_... I try to modify to patch 8 which just adjust the order of calling Decoder->Reset() and SetState(kResetting), but it will cause another failure (TearDownTiming) on dxva_vda. Need to try another solution.
The CQ bit was checked by johnylin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by johnylin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Next time please give a description for each PS. It will be easier for the reviewer to find a specific patch.
lgtm https://codereview.chromium.org/2713863002/diff/200001/media/gpu/video_decode... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2713863002/diff/200001/media/gpu/video_decode... media/gpu/video_decode_accelerator_unittest.cc:816: DCHECK_GE(remaining_play_throughs_, 0); Document SetState(CS_RESETTING); should be called before decoder_->Reset(); because VDA can call NotifyFlushDone from Reset(). Add a TODO to call SetState before all Flush() and Reset().
https://codereview.chromium.org/2713863002/diff/200001/media/gpu/video_decode... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2713863002/diff/200001/media/gpu/video_decode... media/gpu/video_decode_accelerator_unittest.cc:816: DCHECK_GE(remaining_play_throughs_, 0); On 2017/03/15 02:26:29, wuchengli wrote: > Document SetState(CS_RESETTING); should be called before decoder_->Reset(); > because VDA can call NotifyFlushDone from Reset(). Add a TODO to call SetState > before all Flush() and Reset(). Done.
The CQ bit was checked by johnylin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wuchengli@chromium.org Link to the patchset: https://codereview.chromium.org/2713863002/#ps220001 (title: "VDA unittest: test for calling Reset before NotifyFlushDone")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1489547978577940,
"parent_rev": "555dee3871c4fe9bdcbba6c1f226461772229b78", "commit_rev":
"ccc25c33b711f0d546fe4edf6b879489ab09cde4"}
Message was sent while issue was closed.
Description was changed from ========== VDA unittest: test for calling Reset before NotifyFlushDone For VDA it is valid to call Reset() before receiving NotifyResetDone() and VDA should cancel the flush. Add a test to check this scenario. BUG=674828 TEST= run ResetBeforeNotifyFlushDone test on debug build of Samus: passed. Elm: (1) Test is passed on ToT build. (2) Revert https://codereview.chromium.org/2506363004, test hits dcheck in V4L2VideoDecodeAccelerator::StartDevicePoll() https://cs.chromium.org/chromium/src/media/gpu/v4l2_video_decode_accelerator.... CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== VDA unittest: test for calling Reset before NotifyFlushDone For VDA it is valid to call Reset() before receiving NotifyResetDone() and VDA should cancel the flush. Add a test to check this scenario. BUG=674828 TEST= run ResetBeforeNotifyFlushDone test on debug build of Samus: passed. Elm: (1) Test is passed on ToT build. (2) Revert https://codereview.chromium.org/2506363004, test hits dcheck in V4L2VideoDecodeAccelerator::StartDevicePoll() https://cs.chromium.org/chromium/src/media/gpu/v4l2_video_decode_accelerator.... CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2713863002 Cr-Commit-Position: refs/heads/master@{#456990} Committed: https://chromium.googlesource.com/chromium/src/+/ccc25c33b711f0d546fe4edf6b87... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/ccc25c33b711f0d546fe4edf6b87... |
