Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2472923002:
Add a test case to make sure no output before getting any input (Closed)
DescriptionAdd a test case to make sure no output before getting any input
This is used to test crbug.com/641600.
VEA driver should not return an output buffer without any input.
BUG=648861
TEST=test on device.
Committed: https://crrev.com/a4b8c15b3b178281c8f4d92afc3b38ff607480f6
Cr-Commit-Position: refs/heads/master@{#430223}
Patch Set 1 #
Total comments: 19
Patch Set 2 : address comments #
Total comments: 8
Patch Set 3 : address comment #Patch Set 4 : fix compile error #Messages
Total messages: 20 (8 generated)
henryhsu@chromium.org changed reviewers: + posciak@chromium.org, wuchengli@chromium.org
PTAL
wuchengli@chromium.org changed reviewers: + owenlin@chromium.org
Owen. Please help review this.
https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:838: virtual void RequireBitstreamBuffers(unsigned int input_count, There is no need to list these pure virtual functions. https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:850: std::unique_ptr<VideoEncodeAccelerator> CreateFakeVEA(); Why these functions need to be a member of VEAClientBase? Can we make these function global stastic, or put them in a anonymous namespace so that bose VEAClient and VEAClientWithNoInput can use it. https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:859: VEAClientBase::VEAClientBase() { Please run gl cl format to check style issue. https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1707: VEANoInputClient(ClientStateNotification<ClientState>* note); explicit https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1760: gfx::Size visible_size(320, 200); How about making them static const variable. https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1761: for (size_t i = 0; i < arraysize(encoders); ++i) { for (const auto& encoder : encoders) { } https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1802: FROM_HERE, base::TimeDelta::FromMilliseconds(100), Why 100 ms? https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1990: note.reset(new ClientStateNotification<ClientState>()); why not merge the line with declaration. https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:2001: for (size_t state_no = 0; state_no < arraysize(state_transitions); can we use range based for loop here as well.
https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:838: virtual void RequireBitstreamBuffers(unsigned int input_count, On 2016/11/04 03:38:50, Owen Lin wrote: > There is no need to list these pure virtual functions. Done. https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:850: std::unique_ptr<VideoEncodeAccelerator> CreateFakeVEA(); On 2016/11/04 03:38:50, Owen Lin wrote: > Why these functions need to be a member of VEAClientBase? Can we make these > function global stastic, or put them in a anonymous namespace so that bose > VEAClient and VEAClientWithNoInput can use it. Hmm...If so, then we don't have VEAClientBase class. https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:859: VEAClientBase::VEAClientBase() { On 2016/11/04 03:38:51, Owen Lin wrote: > Please run gl cl format to check style issue. Done. https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1707: VEANoInputClient(ClientStateNotification<ClientState>* note); On 2016/11/04 03:38:50, Owen Lin wrote: > explicit Done. https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1760: gfx::Size visible_size(320, 200); On 2016/11/04 03:38:51, Owen Lin wrote: > How about making them static const variable. Done. https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1761: for (size_t i = 0; i < arraysize(encoders); ++i) { On 2016/11/04 03:38:51, Owen Lin wrote: > for (const auto& encoder : encoders) { > } Done. https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1802: FROM_HERE, base::TimeDelta::FromMilliseconds(100), On 2016/11/04 03:38:51, Owen Lin wrote: > Why 100 ms? frame rate is usually 30. 100 ms is about 3 frames duration. I think it is long enough. https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1990: note.reset(new ClientStateNotification<ClientState>()); On 2016/11/04 03:38:51, Owen Lin wrote: > why not merge the line with declaration. Done. https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:2001: for (size_t state_no = 0; state_no < arraysize(state_transitions); On 2016/11/04 03:38:51, Owen Lin wrote: > can we use range based for loop here as well. Done.
lgtm https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2472923002/diff/1/media/gpu/video_encode_acce... media/gpu/video_encode_accelerator_unittest.cc:1802: FROM_HERE, base::TimeDelta::FromMilliseconds(100), On 2016/11/04 04:15:17, henryhsu wrote: > On 2016/11/04 03:38:51, Owen Lin wrote: > > Why 100 ms? > > frame rate is usually 30. 100 ms is about 3 frames duration. > I think it is long enough. Acknowledged.
lgtm https://codereview.chromium.org/2472923002/diff/20001/media/gpu/video_encode_... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2472923002/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1673: // This client is only used to test crbug.com/641600 to make sure the encoder s/test crbug.com/641600 to// https://codereview.chromium.org/2472923002/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1726: const int kDefaultHeight = 200; s/200/240/ https://codereview.chromium.org/2472923002/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1774: } add a comment to explain. something like "// Make sure there's no output frame in 100ms." https://codereview.chromium.org/2472923002/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1799: state_ = new_state; Remove state_ because it's not used.
https://codereview.chromium.org/2472923002/diff/20001/media/gpu/video_encode_... File media/gpu/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/2472923002/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1673: // This client is only used to test crbug.com/641600 to make sure the encoder On 2016/11/07 04:25:17, wuchengli wrote: > s/test crbug.com/641600 to// Done. https://codereview.chromium.org/2472923002/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1726: const int kDefaultHeight = 200; On 2016/11/07 04:25:17, wuchengli wrote: > s/200/240/ Done. https://codereview.chromium.org/2472923002/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1774: } On 2016/11/07 04:25:17, wuchengli wrote: > add a comment to explain. something like "// Make sure there's no output frame > in 100ms." Done. https://codereview.chromium.org/2472923002/diff/20001/media/gpu/video_encode_... media/gpu/video_encode_accelerator_unittest.cc:1799: state_ = new_state; On 2016/11/07 04:25:17, wuchengli wrote: > Remove state_ because it's not used. Done.
The CQ bit was checked by henryhsu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@chromium.org, wuchengli@chromium.org Link to the patchset: https://codereview.chromium.org/2472923002/#ps40001 (title: "address comment")
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_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by henryhsu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@chromium.org, wuchengli@chromium.org Link to the patchset: https://codereview.chromium.org/2472923002/#ps60001 (title: "fix compile error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add a test case to make sure no output before getting any input This is used to test crbug.com/641600. VEA driver should not return an output buffer without any input. BUG=648861 TEST=test on device. ========== to ========== Add a test case to make sure no output before getting any input This is used to test crbug.com/641600. VEA driver should not return an output buffer without any input. BUG=648861 TEST=test on device. Committed: https://crrev.com/a4b8c15b3b178281c8f4d92afc3b38ff607480f6 Cr-Commit-Position: refs/heads/master@{#430223} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a4b8c15b3b178281c8f4d92afc3b38ff607480f6 Cr-Commit-Position: refs/heads/master@{#430223} |
