Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2320763003:
[WebRTC] Add TwoCapturers test and TwoMagnifierCapturers test (Closed)
|
Created:
4 years, 3 months ago by Hzj_jie Modified:
4 years, 3 months ago Reviewers:
Sergey Ulanov CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, Jamie Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
Description[WebRTC] Add TwoCapturers test and TwoMagnifierCapturers test
This change is to add more test cases for ScreenCapturer implementation.
BUG=
Committed: https://crrev.com/8295c9326dbd01bc1ff21f42abb43eab137b1c7d
Cr-Commit-Position: refs/heads/master@{#14218}
Patch Set 1 #Patch Set 2 : Add suppressions.txt #
Total comments: 14
Patch Set 3 : Resolve review comments #
Total comments: 6
Patch Set 4 : Resolve review comments #Patch Set 5 : Disable TwoCapturers test #
Messages
Total messages: 25 (17 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
Patchset #2 (id:120001) has been deleted
https://codereview.chromium.org/2320763003/diff/140001/tools/valgrind-webrtc/... File tools/valgrind-webrtc/drmemory/suppressions.txt (right): https://codereview.chromium.org/2320763003/diff/140001/tools/valgrind-webrtc/... tools/valgrind-webrtc/drmemory/suppressions.txt:30: *!webrtc::ScreenCapturerWinGdi::ScreenCapturerWinMagnifier This doesn't look right. Did you want to change this to webrtc::ScreenCapturerWinMagnifier::ScreenCapturerWinMagnifier? https://codereview.chromium.org/2320763003/diff/140001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): https://codereview.chromium.org/2320763003/diff/140001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:123: for (int c = 0; c < 3; c++) { Where does 3 come from? Maybe add a comment here with a high-level description of what happens here. https://codereview.chromium.org/2320763003/diff/140001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:136: for (int j = 0; j < wait_capture_round; j++) { Maybe move this code to a separate function to make it easier to parse? There are 4 nested for loops here, which makes it hard to understand what's going on. https://codereview.chromium.org/2320763003/diff/140001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:141: // cannot capture a correct frame again if screen does not update. I don't understand this comment. All capturers should be capture a new frame even if nothing has changed. They will just keep updated_region() empty. Also TODO() normally should say what needs to be done. https://codereview.chromium.org/2320763003/diff/140001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:147: return; Is it appropriate to just return if one of the capturer fails? It means that the test will pass, and this doesn't look right to me. https://codereview.chromium.org/2320763003/diff/140001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:186: capturer_.reset(ScreenCapturer::Create(options)); It doesn't look right that a method called Set*() create a screen capturer. Rename it to CreateMagnifierCapturer()? https://codereview.chromium.org/2320763003/diff/140001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:356: std::unique_ptr<ScreenCapturer> capturer2(capturer_.release()); capturer2 = std::move(capturer_);
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:180001) has been deleted
Patchset #3 (id:200001) has been deleted
Patchset #3 (id:220001) has been deleted
Patchset #3 (id:240001) has been deleted
https://codereview.chromium.org/2320763003/diff/140001/tools/valgrind-webrtc/... File tools/valgrind-webrtc/drmemory/suppressions.txt (right): https://codereview.chromium.org/2320763003/diff/140001/tools/valgrind-webrtc/... tools/valgrind-webrtc/drmemory/suppressions.txt:30: *!webrtc::ScreenCapturerWinGdi::ScreenCapturerWinMagnifier On 2016/09/09 17:23:14, Sergey Ulanov wrote: > This doesn't look right. > Did you want to change this to > webrtc::ScreenCapturerWinMagnifier::ScreenCapturerWinMagnifier? Yes, it's a typo, and it caught by win_drmemory_full test. https://codereview.chromium.org/2320763003/diff/140001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): https://codereview.chromium.org/2320763003/diff/140001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:123: for (int c = 0; c < 3; c++) { On 2016/09/09 17:23:15, Sergey Ulanov wrote: > Where does 3 come from? Maybe add a comment here with a high-level description > of what happens here. Done. https://codereview.chromium.org/2320763003/diff/140001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:136: for (int j = 0; j < wait_capture_round; j++) { On 2016/09/09 17:23:15, Sergey Ulanov wrote: > Maybe move this code to a separate function to make it easier to parse? There > are 4 nested for loops here, which makes it hard to understand what's going on. Sure. https://codereview.chromium.org/2320763003/diff/140001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:141: // cannot capture a correct frame again if screen does not update. On 2016/09/09 17:23:14, Sergey Ulanov wrote: > I don't understand this comment. All capturers should be capture a new frame > even if nothing has changed. They will just keep updated_region() empty. > Also TODO() normally should say what needs to be done. Emmm, updated. https://codereview.chromium.org/2320763003/diff/140001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:147: return; On 2016/09/09 17:23:15, Sergey Ulanov wrote: > Is it appropriate to just return if one of the capturer fails? It means that the > test will pass, and this doesn't look right to me. Not really. CaptureFrame triggers an assertion failure. But I should add comment to explain, I remember Jamie asked same question before. https://codereview.chromium.org/2320763003/diff/140001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:186: capturer_.reset(ScreenCapturer::Create(options)); On 2016/09/09 17:23:15, Sergey Ulanov wrote: > It doesn't look right that a method called Set*() create a screen capturer. > Rename it to CreateMagnifierCapturer()? Done. https://codereview.chromium.org/2320763003/diff/140001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:356: std::unique_ptr<ScreenCapturer> capturer2(capturer_.release()); On 2016/09/09 17:23:15, Sergey Ulanov wrote: > capturer2 = std::move(capturer_); Done.
lgtm https://codereview.chromium.org/2320763003/diff/260001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): https://codereview.chromium.org/2320763003/diff/260001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:167: for (size_t j = 0; j < capturers.size(); j++) { You could use a range loop here. for (auto& capturer : capturers) https://codereview.chromium.org/2320763003/diff/260001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:183: succeeded_capturers++; Instead of setting capturers to nullptr and keeping this counter you could just remove them from capturers vector once they succeed. That would simplify this code. https://codereview.chromium.org/2320763003/diff/260001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:191: ASSERT_LT(i, wait_capture_round); Do you really need this assert? It just verifies that a loop index doesn't go out of range.
Patchset #4 (id:280001) has been deleted
Patchset #4 (id:300001) has been deleted
Sorry, due to a wrong assertion in the last iteration, the TwoCapturers test actually cannot pass on both linux and windows trybots. It's weird it can pass on my machine. I disable this test case for now, and submit this change first. Since it changes to a correct way to execute multiple capturers test, and investigate the failure soon. https://codereview.chromium.org/2320763003/diff/260001/webrtc/modules/desktop... File webrtc/modules/desktop_capture/screen_capturer_unittest.cc (right): https://codereview.chromium.org/2320763003/diff/260001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:167: for (size_t j = 0; j < capturers.size(); j++) { On 2016/09/14 00:08:44, Sergey Ulanov wrote: > You could use a range loop here. > for (auto& capturer : capturers) I need it to mark capturers[j] = nullptr or capturers.erase(...). https://codereview.chromium.org/2320763003/diff/260001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:183: succeeded_capturers++; On 2016/09/14 00:08:44, Sergey Ulanov wrote: > Instead of setting capturers to nullptr and keeping this counter you could just > remove them from capturers vector once they succeed. That would simplify this > code. It seems like this approach will make the logic more complex. When removing the first element, the new iterator should be placed before the first element, which is not allowed in vector. https://codereview.chromium.org/2320763003/diff/260001/webrtc/modules/desktop... webrtc/modules/desktop_capture/screen_capturer_unittest.cc:191: ASSERT_LT(i, wait_capture_round); On 2016/09/14 00:08:44, Sergey Ulanov wrote: > Do you really need this assert? It just verifies that a loop index doesn't go > out of range. Sorry, this is not correct, I expect all capturers should be able to work in 20 rounds.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2320763003/#ps340001 (title: "Disable TwoCapturers test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #5 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== [WebRTC] Add TwoCapturers test and TwoMagnifierCapturers test This change is to add more test cases for ScreenCapturer implementation. BUG= ========== to ========== [WebRTC] Add TwoCapturers test and TwoMagnifierCapturers test This change is to add more test cases for ScreenCapturer implementation. BUG= Committed: https://crrev.com/8295c9326dbd01bc1ff21f42abb43eab137b1c7d Cr-Commit-Position: refs/heads/master@{#14218} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8295c9326dbd01bc1ff21f42abb43eab137b1c7d Cr-Commit-Position: refs/heads/master@{#14218} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
