Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2494893002:
Crash in DirectX capturer (Closed)
DescriptionCrash in DirectX capturer
A tiny but critical change to avoid a crash failure in DirectX capturer.
A good news is this failure is caught by ScreenCapturer integration tests.
BUG=314516
Committed: https://crrev.com/307409681680d9c783b4de1b48121a3056acfaf7
Cr-Commit-Position: refs/heads/master@{#15046}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Resolve review comments #
Total comments: 8
Patch Set 3 : Resolve review comments #
Total comments: 6
Patch Set 4 : Resolve review comments #Messages
Total messages: 23 (8 generated)
Description was changed from ========== Crash in DirectX capturer A tiny but critical change to avoid a crash failure in DirectX capturer. BUG=314516 ========== to ========== Crash in DirectX capturer A tiny but critical change to avoid a crash failure in DirectX capturer. BUG=314516 ==========
zijiehe@chromium.org changed reviewers: + jamiewalch@chromium.org, sergeyu@chromium.org
Description was changed from ========== Crash in DirectX capturer A tiny but critical change to avoid a crash failure in DirectX capturer. BUG=314516 ========== to ========== Crash in DirectX capturer A tiny but critical change to avoid a crash failure in DirectX capturer. A good news is this failure is caught by ScreenCapturer integration tests. BUG=314516 ==========
https://codereview.webrtc.org/2494893002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.webrtc.org/2494893002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:300: if (dest && dest != source) { Under what circumstances does it makes sense to allow nulls in contexts_? Could we instead make sure that doesn't happen?
https://codereview.webrtc.org/2494893002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.webrtc.org/2494893002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:300: if (dest && dest != source) { On 2016/11/11 00:00:41, Jamie wrote: > Under what circumstances does it makes sense to allow nulls in contexts_? Could > we instead make sure that doesn't happen? It's a performance trick, and totally expected. You may refer to line 290. More background, The Context is designed as a bridge between singleton (DXGI APIs) and multiton (ScreenCapturerWinDirectx). Each ScreenCapturerWinDirectx has its own state, since it's double-buffered, and we do not want to copy entire frame each time for performance concern. But DxgiDuplicatorController is singleton because of the restriction of DXGI APIs. So once a ScreenCapturerWinDirectx is created, it needs to register its Context object in DxgiDuplicatorController to get updated region information during each capturer call. And once it's destroyed, its context needs to be removed. So in line 290, there is a small trick to avoid removing element from a vector (O(1) vs O(n)). So we need to have this check here.
https://codereview.webrtc.org/2494893002/diff/1/webrtc/modules/desktop_captur... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.webrtc.org/2494893002/diff/1/webrtc/modules/desktop_captur... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:300: if (dest && dest != source) { On 2016/11/11 00:10:51, Hzj_jie wrote: > On 2016/11/11 00:00:41, Jamie wrote: > > Under what circumstances does it makes sense to allow nulls in contexts_? > Could > > we instead make sure that doesn't happen? > > It's a performance trick, and totally expected. You may refer to line 290. > > More background, > The Context is designed as a bridge between singleton (DXGI APIs) and multiton > (ScreenCapturerWinDirectx). Each ScreenCapturerWinDirectx has its own state, > since it's double-buffered, and we do not want to copy entire frame each time > for performance concern. But DxgiDuplicatorController is singleton because of > the restriction of DXGI APIs. > So once a ScreenCapturerWinDirectx is created, it needs to register its Context > object in DxgiDuplicatorController to get updated region information during each > capturer call. And once it's destroyed, its context needs to be removed. So in > line 290, there is a small trick to avoid removing element from a vector (O(1) > vs O(n)). So we need to have this check here. Thanks for the context. I think my question changes in that case to whether or not that performance optimization is worthwhile. How large is the vector expected to grow, and how often is Unregister called? If it's not on the critical path, I would prefer we just clean up the vector properly, otherwise we're leaving ourselves open to bugs like this in the future.
On 2016/11/11 00:15:21, Jamie wrote: > https://codereview.webrtc.org/2494893002/diff/1/webrtc/modules/desktop_captur... > File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): > > https://codereview.webrtc.org/2494893002/diff/1/webrtc/modules/desktop_captur... > webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:300: if (dest && > dest != source) { > On 2016/11/11 00:10:51, Hzj_jie wrote: > > On 2016/11/11 00:00:41, Jamie wrote: > > > Under what circumstances does it makes sense to allow nulls in contexts_? > > Could > > > we instead make sure that doesn't happen? > > > > It's a performance trick, and totally expected. You may refer to line 290. > > > > More background, > > The Context is designed as a bridge between singleton (DXGI APIs) and multiton > > (ScreenCapturerWinDirectx). Each ScreenCapturerWinDirectx has its own state, > > since it's double-buffered, and we do not want to copy entire frame each time > > for performance concern. But DxgiDuplicatorController is singleton because of > > the restriction of DXGI APIs. > > So once a ScreenCapturerWinDirectx is created, it needs to register its > Context > > object in DxgiDuplicatorController to get updated region information during > each > > capturer call. And once it's destroyed, its context needs to be removed. So in > > line 290, there is a small trick to avoid removing element from a vector (O(1) > > vs O(n)). So we need to have this check here. > > Thanks for the context. I think my question changes in that case to whether or > not that performance optimization is worthwhile. How large is the vector > expected to grow, and how often is Unregister called? If it's not on the > critical path, I would prefer we just clean up the vector properly, otherwise > we're leaving ourselves open to bugs like this in the future. No, it's not worthy. The vector will be changed each time a ScreenCapturerWinDirectx instance created or destroyed. I do not expect it's really frequent. I would prepare another change to remove this optimization. But I still prefer this change to be submitted, as it's still a simple and reliable way to resolve this issue immediately.
LGTM if the proper fix is significantly more complex than using erase() instead of assigning null. If it's that simple (or nearly that simple) then I don't really see the benefit of an intermediate fix.
On 2016/11/11 01:43:06, Jamie wrote: > LGTM if the proper fix is significantly more complex than using erase() instead > of assigning null. If it's that simple (or nearly that simple) then I don't > really see the benefit of an intermediate fix. Updated.
lgtm
https://codereview.webrtc.org/2494893002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.webrtc.org/2494893002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:278: if (contexts_[i] == nullptr) { I guess this loop can be removed now. https://codereview.webrtc.org/2494893002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:288: for (auto it = contexts_.begin(); it != contexts_.end(); it++) { This loop can be replaced with std::find: contexts_.erase(std::find( contexts_.begin(), contexts_.end(), context)); https://codereview.webrtc.org/2494893002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:290: contexts_.erase(it); What was the reason this code was implemented this way? I.e. why was it necessary to replace values with nullptr instead of erasing them?
Patchset #3 (id:40001) has been deleted
https://codereview.webrtc.org/2494893002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.webrtc.org/2494893002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:278: if (contexts_[i] == nullptr) { On 2016/11/11 18:27:18, Sergey Ulanov wrote: > I guess this loop can be removed now. Yes. Updated. https://codereview.webrtc.org/2494893002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:288: for (auto it = contexts_.begin(); it != contexts_.end(); it++) { On 2016/11/11 18:27:18, Sergey Ulanov wrote: > This loop can be replaced with std::find: > contexts_.erase(std::find( > contexts_.begin(), contexts_.end(), context)); Done. https://codereview.webrtc.org/2494893002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:290: contexts_.erase(it); On 2016/11/11 18:27:18, Sergey Ulanov wrote: > What was the reason this code was implemented this way? I.e. why was it > necessary to replace values with nullptr instead of erasing them? Because std::vector::erase is O(n) instead of O(1). I over-optimized this logic.
LGTM https://codereview.webrtc.org/2494893002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.webrtc.org/2494893002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:290: contexts_.erase(it); On 2016/11/11 21:51:31, Hzj_jie wrote: > On 2016/11/11 18:27:18, Sergey Ulanov wrote: > > What was the reason this code was implemented this way? I.e. why was it > > necessary to replace values with nullptr instead of erasing them? > > Because std::vector::erase is O(n) instead of O(1). I over-optimized this logic. FWIW it can be O(1) if you swap it with the last element, we don't care about ordering here, but IMO it's better to keep this code simpler. https://codereview.webrtc.org/2494893002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.webrtc.org/2494893002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:279: contexts_.push_back(context); Maybe DCHECK here that context is not already in contexts_? https://codereview.webrtc.org/2494893002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:284: // RTC_DCHECK_EQ will trigger build break. Refer to webrtc/base/checks.h:130. I don't think you really need this comment, it's a well known problem with DCHECK_EQ. https://codereview.webrtc.org/2494893002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:285: RTC_DCHECK(std::find(contexts_.begin(), contexts_.end(), context) != I'd prefer to save result of std::find() and use it here and in erase() call below. This would make it clearer how DCHECK is related to the line below it and the code would be simpler.
The failures on swarming do not relate to my change; the screen capturer tests are disabled. The logs are more like a fundamental failure, WindowsError: [Error 216] This version of %1 is not compatible with the version of Windows you're running. Check your computer's system information to see whether you need a x86 (32-bit) or x64 (64-bit) version of the program, and then contact the software publisher https://codereview.webrtc.org/2494893002/diff/20001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.webrtc.org/2494893002/diff/20001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:290: contexts_.erase(it); On 2016/11/11 22:57:35, Sergey Ulanov wrote: > On 2016/11/11 21:51:31, Hzj_jie wrote: > > On 2016/11/11 18:27:18, Sergey Ulanov wrote: > > > What was the reason this code was implemented this way? I.e. why was it > > > necessary to replace values with nullptr instead of erasing them? > > > > Because std::vector::erase is O(n) instead of O(1). I over-optimized this > logic. > > FWIW it can be O(1) if you swap it with the last element, we don't care about > ordering here, but IMO it's better to keep this code simpler. Oh, yes, since we have searched already. But the newer version should only search once. https://codereview.webrtc.org/2494893002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc (right): https://codereview.webrtc.org/2494893002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:279: contexts_.push_back(context); On 2016/11/11 22:57:35, Sergey Ulanov wrote: > Maybe DCHECK here that context is not already in contexts_? Done. https://codereview.webrtc.org/2494893002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:284: // RTC_DCHECK_EQ will trigger build break. Refer to webrtc/base/checks.h:130. On 2016/11/11 22:57:35, Sergey Ulanov wrote: > I don't think you really need this comment, it's a well known problem with > DCHECK_EQ. Really, I thought it was not so common. https://codereview.webrtc.org/2494893002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_output_duplicator.cc:285: RTC_DCHECK(std::find(contexts_.begin(), contexts_.end(), context) != On 2016/11/11 22:57:35, Sergey Ulanov wrote: > I'd prefer to save result of std::find() and use it here and in erase() call > below. This would make it clearer how DCHECK is related to the line below it and > the code would be simpler. Done.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamiewalch@chromium.org, sergeyu@chromium.org Link to the patchset: https://codereview.webrtc.org/2494893002/#ps80001 (title: "Resolve review comments")
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.
Description was changed from ========== Crash in DirectX capturer A tiny but critical change to avoid a crash failure in DirectX capturer. A good news is this failure is caught by ScreenCapturer integration tests. BUG=314516 ========== to ========== Crash in DirectX capturer A tiny but critical change to avoid a crash failure in DirectX capturer. A good news is this failure is caught by ScreenCapturer integration tests. BUG=314516 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Crash in DirectX capturer A tiny but critical change to avoid a crash failure in DirectX capturer. A good news is this failure is caught by ScreenCapturer integration tests. BUG=314516 ========== to ========== Crash in DirectX capturer A tiny but critical change to avoid a crash failure in DirectX capturer. A good news is this failure is caught by ScreenCapturer integration tests. BUG=314516 Committed: https://crrev.com/307409681680d9c783b4de1b48121a3056acfaf7 Cr-Commit-Position: refs/heads/master@{#15046} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/307409681680d9c783b4de1b48121a3056acfaf7 Cr-Commit-Position: refs/heads/master@{#15046} |
