Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2703123002:
Skips the first frame in DxgiDuplicatorController (Closed)
|
Created:
3 years, 10 months ago by Hzj_jie Modified:
3 years, 9 months ago Reviewers:
Sergey Ulanov CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, Jamie Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSkips the first frame in DxgiDuplicatorController
A bug has been reported to complaint the ScreenCapturerWinDirectx cannot capture
the first frame, which is used in "Report an issue" page.
A simple solution here is to skip the first frame.
This change also removes the friend relationship between
DxgiDuplicatorController / DxgiAdapterDuplicator / DxgiOutputDuplicator, which
is not really necessary.
BUG=682112
Review-Url: https://codereview.webrtc.org/2703123002
Cr-Commit-Position: refs/heads/master@{#16815}
Committed: https://chromium.googlesource.com/external/webrtc/+/8697f9afbaa42c55f07331457f4427471f11a861
Patch Set 1 #
Total comments: 24
Patch Set 2 : Resolve review comments #Patch Set 3 : Reduce frames_to_skip to 1 #Patch Set 4 : Resolve review comments #
Messages
Total messages: 49 (35 generated)
The CQ bit was checked by zijiehe@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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zijiehe@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.webrtc.org/...
The CQ bit was checked by zijiehe@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.webrtc.org/...
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Dxgi screen capturer skips first 10 frames BUG=682112 ========== to ========== Skips the first frame in DxgiDuplicatorController A bug has been reported to complaint the ScreenCapturerWinDirectx cannot capture the first frame, which is used in "Report an issue" page. A simple solution here is to skip the first frame. This change also removes the friend relationship between DxgiDuplicatorController / DxgiAdapterDuplicator / DxgiOutputDuplicator, which is not really necessary. BUG=682112 ==========
Description was changed from ========== Skips the first frame in DxgiDuplicatorController A bug has been reported to complaint the ScreenCapturerWinDirectx cannot capture the first frame, which is used in "Report an issue" page. A simple solution here is to skip the first frame. This change also removes the friend relationship between DxgiDuplicatorController / DxgiAdapterDuplicator / DxgiOutputDuplicator, which is not really necessary. BUG=682112 ========== to ========== Skips the first several frames in DxgiDuplicatorController A bug has been reported to complaint the ScreenCapturerWinDirectx cannot capture the first frame, which is used in "Report an issue" page. A simple solution here is to skip the first several frames. This change also removes the friend relationship between DxgiDuplicatorController / DxgiAdapterDuplicator / DxgiOutputDuplicator, which is not really necessary. BUG=682112 ==========
The CQ bit was checked by zijiehe@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.webrtc.org/...
Patchset #1 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
zijiehe@chromium.org changed reviewers: + sergeyu@chromium.org
This looks like a workaround that doesn't address the root cause. Do you know why we get black/stale frames? Also I'm not sure this will help in all cases. From what I saw the problem is not only that the capturer returns black first frame, but also that it also returns stale frame for the first Capture() call. I.e. if one capturer is captures a single frame at T0 (which is black) and then another capturer is created at T1 then it would first return an outdated frame from T0. I don't see how this change can help in that scenario.
On 2017/02/21 21:10:29, Sergey Ulanov wrote: > This looks like a workaround that doesn't address the root cause. Do you know > why we get black/stale frames? > > Also I'm not sure this will help in all cases. From what I saw the problem is > not only that the capturer returns black first frame, but also that it also > returns stale frame for the first Capture() call. I.e. if one capturer is > captures a single frame at T0 (which is black) and then another capturer is > created at T1 then it would first return an outdated frame from T0. I don't see > how this change can help in that scenario. Regarding to the bug, I found three issues, 1. The first frame is black. This looks like an expected behavior of the DXGI APIs. The IDXGIOutputDuplication::AcquireNextFrame() always capture the update of the screen. i.e. In T0, there is no update comparing to the previous state. So this change is targeting to resolve the issue by skipping first several (currently this value is 2) frames until a fully captured frame. 2. The stale frame, i.e. DxgiDuplicatorController returns last frame instead of current frame. I have not reproduced this issue on my machine, but logically this can be happened when there is no frame updated between two Duplicate() calls. In this scenario, DxgiOutputDuplicator uses last frame to fill current frame. There may be some other hidden issues. I am still trying to reproduce. 3. The frames are always black. This happens on my virtual machine after some mysterious Windows 10 or VirtualBox update. The very bad thing is, in this scenario, DXGI APIs return correct values without any error notifications. https://codereview.chromium.org/2709523003/ is targeting this issue.
On 2017/02/21 22:03:40, Hzj_jie wrote: > On 2017/02/21 21:10:29, Sergey Ulanov wrote: > > This looks like a workaround that doesn't address the root cause. Do you know > > why we get black/stale frames? > > > > Also I'm not sure this will help in all cases. From what I saw the problem is > > not only that the capturer returns black first frame, but also that it also > > returns stale frame for the first Capture() call. I.e. if one capturer is > > captures a single frame at T0 (which is black) and then another capturer is > > created at T1 then it would first return an outdated frame from T0. I don't > see > > how this change can help in that scenario. > > Regarding to the bug, I found three issues, > 1. The first frame is black. This looks like an expected behavior of the DXGI > APIs. The IDXGIOutputDuplication::AcquireNextFrame() always capture the update > of the screen. i.e. In T0, there is no update comparing to the previous state. > So this change is targeting to resolve the issue by skipping first several > (currently this value is 2) frames until a fully captured frame. > 2. The stale frame, i.e. DxgiDuplicatorController returns last frame instead of > current frame. I have not reproduced this issue on my machine, but logically > this can be happened when there is no frame updated between two Duplicate() > calls. In this scenario, DxgiOutputDuplicator uses last frame to fill current > frame. There may be some other hidden issues. I am still trying to reproduce. > 3. The frames are always black. This happens on my virtual machine after some > mysterious Windows 10 or VirtualBox update. The very bad thing is, in this > scenario, DXGI APIs return correct values without any error notifications. > https://codereview.chromium.org/2709523003/ is targeting this issue. This change equals to return a ternary from DxgiOutputDuplicator::Duplicate() function. i.e. success / failure / try again later. Technically said, we should return "TryAgainLater" state if AcquireNextFrame() returns DXGI_ERROR_WAIT_TIMEOUT and !last_frame_. But it's not only more complex to change to the ternary state, i.e. what should return if one duplicator returns TryAgainLater, but another returns false. It's also not worthy. The "TryAgainLater" state happens only at the very first several frames, which can be covered by this change in a more simple approach.
https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc (right): https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:151: int64_t DxgiAdapterDuplicator::num_frames_captured() const { It should be called GetNumFramesCaptured() https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:153: for (size_t i = 0; i < duplicators_.size(); i++) { for (auto& duplicator : duplicators_) https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:154: if (duplicators_[i].num_frames_captured() < min) { min = std::min(min, duplicator.num_frames_captured()); https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:28: void SleepMoreThanMs(int milliseconds) { Do you really need this function? webrtc::SleepMs() calls Sleep() and I don't think Sleep() can return early on windows. https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:324: int64_t DxgiDuplicatorController::num_frames_captured() const { GetNumFramesCaptured() https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:351: const int64_t skip_frames = 2; Where does 2 come from? why 1 is not enough? https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:351: const int64_t skip_frames = 2; maybe call it frames_to_skip? https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:359: target->size().height() >= desktop_size().height()) { Why is this necessary? Shouldn't the capturer make sure that the target frame fits the screen? https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:382: if (rtc::TimeMillis() - start_ms > (ms_per_frame * skip_frames * 4)) { Where does 4 come from? I suggest adding a separate const for total timeout for the first frame. MSDN examples pass 500 to AcquireNextFrame(), so I suggest using it here as well.
The CQ bit was checked by zijiehe@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.webrtc.org/...
https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc (right): https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:151: int64_t DxgiAdapterDuplicator::num_frames_captured() const { On 2017/02/23 19:07:30, Sergey Ulanov wrote: > It should be called GetNumFramesCaptured() Done. https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:153: for (size_t i = 0; i < duplicators_.size(); i++) { On 2017/02/23 19:07:30, Sergey Ulanov wrote: > for (auto& duplicator : duplicators_) Done. https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_adapter_duplicator.cc:154: if (duplicators_[i].num_frames_captured() < min) { On 2017/02/23 19:07:30, Sergey Ulanov wrote: > min = std::min(min, duplicator.num_frames_captured()); Done. https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:28: void SleepMoreThanMs(int milliseconds) { On 2017/02/23 19:07:30, Sergey Ulanov wrote: > Do you really need this function? webrtc::SleepMs() calls Sleep() and I don't > think Sleep() can return early on windows. According to MSDN, the Sleep() function may still return before the time we provided. Especially when the time is less than one time slice of Windows, which is typically 16 milliseconds on Windows 10. In our scenario, DX capturer uses ~2 milliseconds for one frame, so we usually ask the thread to sleep 15 milliseconds. But I think this function should belong to webrtc/base. See https://cs.chromium.org/chromium/src/base/threading/platform_thread.h. https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:324: int64_t DxgiDuplicatorController::num_frames_captured() const { On 2017/02/23 19:07:30, Sergey Ulanov wrote: > GetNumFramesCaptured() Done. https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:351: const int64_t skip_frames = 2; On 2017/02/23 19:07:30, Sergey Ulanov wrote: > maybe call it frames_to_skip? Done. https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:351: const int64_t skip_frames = 2; On 2017/02/23 19:07:30, Sergey Ulanov wrote: > Where does 2 come from? why 1 is not enough? By using 2, we can ensure we always wait for 17 milliseconds. (Around line 370). https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:359: target->size().height() >= desktop_size().height()) { On 2017/02/23 19:07:30, Sergey Ulanov wrote: > Why is this necessary? Shouldn't the capturer make sure that the target frame > fits the screen? A ScreenCapturerWinDirectx may capture only one of the monitors, so the |target| size may be smaller than entire screen resolution, which causes some of the DxgiOutputDuplicator to be ignored. https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:382: if (rtc::TimeMillis() - start_ms > (ms_per_frame * skip_frames * 4)) { On 2017/02/23 19:07:30, Sergey Ulanov wrote: > Where does 4 come from? I suggest adding a separate const for total timeout for > the first frame. MSDN examples pass 500 to AcquireNextFrame(), so I suggest > using it here as well. It's a very random number. :) Updated.
https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:28: void SleepMoreThanMs(int milliseconds) { On 2017/02/23 20:52:11, Hzj_jie wrote: > On 2017/02/23 19:07:30, Sergey Ulanov wrote: > > Do you really need this function? webrtc::SleepMs() calls Sleep() and I don't > > think Sleep() can return early on windows. > > According to MSDN, the Sleep() function may still return before the time we > provided. Where does it say that? I was reading these page: https://msdn.microsoft.com/en-us/library/windows/desktop/ms686298(v=vs.85).aspx I don't see it saying anywhere it may return early. > Especially when the time is less than one time slice of Windows, which > is typically 16 milliseconds on Windows 10. In our scenario, DX capturer uses ~2 > milliseconds for one frame, so we usually ask the thread to sleep 15 > milliseconds. > > But I think this function should belong to webrtc/base. See > https://cs.chromium.org/chromium/src/base/threading/platform_thread.h. https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:351: const int64_t skip_frames = 2; On 2017/02/23 20:52:11, Hzj_jie wrote: > On 2017/02/23 19:07:30, Sergey Ulanov wrote: > > Where does 2 come from? why 1 is not enough? > > By using 2, we can ensure we always wait for 17 milliseconds. (Around line 370). But why do we need to ensure that at all? I think we just need to make sure at least one frame has been captured, that's all.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:28: void SleepMoreThanMs(int milliseconds) { On 2017/02/23 21:06:43, Sergey Ulanov wrote: > On 2017/02/23 20:52:11, Hzj_jie wrote: > > On 2017/02/23 19:07:30, Sergey Ulanov wrote: > > > Do you really need this function? webrtc::SleepMs() calls Sleep() and I > don't > > > think Sleep() can return early on windows. > > > > According to MSDN, the Sleep() function may still return before the time we > > provided. > > Where does it say that? I was reading these page: > https://msdn.microsoft.com/en-us/library/windows/desktop/ms686298(v=vs.85).aspx > I don't see it saying anywhere it may return early. > > > Especially when the time is less than one time slice of Windows, which > > is typically 16 milliseconds on Windows 10. In our scenario, DX capturer uses > ~2 > > milliseconds for one frame, so we usually ask the thread to sleep 15 > > milliseconds. > > > > But I think this function should belong to webrtc/base. See > > https://cs.chromium.org/chromium/src/base/threading/platform_thread.h. > It states, If dwMilliseconds is less than the resolution of the system clock, the thread may sleep for less than the specified length of time. If dwMilliseconds is greater than one tick but less than two, the wait can be anywhere between one and two ticks, and so on. Here the resolution of the system means the time slice length of the OS. It can be impacted by the timeBegionPeriod() function. https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:351: const int64_t skip_frames = 2; On 2017/02/23 21:06:43, Sergey Ulanov wrote: > On 2017/02/23 20:52:11, Hzj_jie wrote: > > On 2017/02/23 19:07:30, Sergey Ulanov wrote: > > > Where does 2 come from? why 1 is not enough? > > > > By using 2, we can ensure we always wait for 17 milliseconds. (Around line > 370). > > But why do we need to ensure that at all? I think we just need to make sure at > least one frame has been captured, that's all. Here the real issue is, we need to ensure two succeeded AcquireNextFrame() function calls in two monitor fresh cycles. If we return before the frame has been fully updated (here we sleep 17 milliseconds to ensure it), we may still get a black frame. DxgiDuplicatorController immediately starts to capture the frame after this function. Indeed I have tried several values of frames_to_skip, and 2 is the minimum but workable value.
The CQ bit was checked by zijiehe@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.webrtc.org/...
On 2017/02/23 21:48:58, Hzj_jie wrote: > https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... > File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): > > https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... > webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:28: void > SleepMoreThanMs(int milliseconds) { > On 2017/02/23 21:06:43, Sergey Ulanov wrote: > > On 2017/02/23 20:52:11, Hzj_jie wrote: > > > On 2017/02/23 19:07:30, Sergey Ulanov wrote: > > > > Do you really need this function? webrtc::SleepMs() calls Sleep() and I > > don't > > > > think Sleep() can return early on windows. > > > > > > According to MSDN, the Sleep() function may still return before the time we > > > provided. > > > > Where does it say that? I was reading these page: > > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms686298(v=vs.85).aspx > > I don't see it saying anywhere it may return early. > > > > > Especially when the time is less than one time slice of Windows, which > > > is typically 16 milliseconds on Windows 10. In our scenario, DX capturer > uses > > ~2 > > > milliseconds for one frame, so we usually ask the thread to sleep 15 > > > milliseconds. > > > > > > But I think this function should belong to webrtc/base. See > > > https://cs.chromium.org/chromium/src/base/threading/platform_thread.h. > > > > It states, > If dwMilliseconds is less than the resolution of the system clock, the thread > may sleep for less than the specified length of time. If dwMilliseconds is > greater than one tick but less than two, the wait can be anywhere between one > and two ticks, and so on. > Here the resolution of the system means the time slice length of the OS. It can > be impacted by the timeBegionPeriod() function. > > https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... > webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:351: const > int64_t skip_frames = 2; > On 2017/02/23 21:06:43, Sergey Ulanov wrote: > > On 2017/02/23 20:52:11, Hzj_jie wrote: > > > On 2017/02/23 19:07:30, Sergey Ulanov wrote: > > > > Where does 2 come from? why 1 is not enough? > > > > > > By using 2, we can ensure we always wait for 17 milliseconds. (Around line > > 370). > > > > But why do we need to ensure that at all? I think we just need to make sure at > > least one frame has been captured, that's all. > > Here the real issue is, we need to ensure two succeeded AcquireNextFrame() > function calls in two monitor fresh cycles. > If we return before the frame has been fully updated (here we sleep 17 > milliseconds to ensure it), we may still get a black frame. > DxgiDuplicatorController immediately starts to capture the frame after this > function. > Indeed I have tried several values of frames_to_skip, and 2 is the minimum but > workable value. Sorry for the wrong information, it looks like the key is SleepMoreThanMs(17) here instead of frames_to_skip = 2. I can reproduce the black frame by removing the sleep statement, but cannot when frames_to_skip is 1. Personally I prefer to keep the GetNumFramesCaptured() API to avoid by any accident we need to set the frames_to_skip to more than 1.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I still don't think we need SleepMoreThanMs(). Otherwise LGTM https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:28: void SleepMoreThanMs(int milliseconds) { On 2017/02/23 21:48:58, Hzj_jie wrote: > On 2017/02/23 21:06:43, Sergey Ulanov wrote: > > On 2017/02/23 20:52:11, Hzj_jie wrote: > > > On 2017/02/23 19:07:30, Sergey Ulanov wrote: > > > > Do you really need this function? webrtc::SleepMs() calls Sleep() and I > > don't > > > > think Sleep() can return early on windows. > > > > > > According to MSDN, the Sleep() function may still return before the time we > > > provided. > > > > Where does it say that? I was reading these page: > > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms686298(v=vs.85).aspx > > I don't see it saying anywhere it may return early. > > > > > Especially when the time is less than one time slice of Windows, which > > > is typically 16 milliseconds on Windows 10. In our scenario, DX capturer > uses > > ~2 > > > milliseconds for one frame, so we usually ask the thread to sleep 15 > > > milliseconds. > > > > > > But I think this function should belong to webrtc/base. See > > > https://cs.chromium.org/chromium/src/base/threading/platform_thread.h. > > > > It states, > If dwMilliseconds is less than the resolution of the system clock, the thread > may sleep for less than the specified length of time. If dwMilliseconds is > greater than one tick but less than two, the wait can be anywhere between one > and two ticks, and so on. > Here the resolution of the system means the time slice length of the OS. It can > be impacted by the timeBegionPeriod() function. Ah, I see. Sorry I missed it. Still, I don't think we really need this function. If Sleep() returns too soon then EnsureFrameCaptured() will just call this function again.
The CQ bit was checked by zijiehe@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.webrtc.org/...
On 2017/02/24 00:40:40, Sergey Ulanov wrote: > I still don't think we need SleepMoreThanMs(). Otherwise LGTM > > https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... > File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): > > https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... > webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:28: void > SleepMoreThanMs(int milliseconds) { > On 2017/02/23 21:48:58, Hzj_jie wrote: > > On 2017/02/23 21:06:43, Sergey Ulanov wrote: > > > On 2017/02/23 20:52:11, Hzj_jie wrote: > > > > On 2017/02/23 19:07:30, Sergey Ulanov wrote: > > > > > Do you really need this function? webrtc::SleepMs() calls Sleep() and I > > > don't > > > > > think Sleep() can return early on windows. > > > > > > > > According to MSDN, the Sleep() function may still return before the time > we > > > > provided. > > > > > > Where does it say that? I was reading these page: > > > > > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms686298(v=vs.85).aspx > > > I don't see it saying anywhere it may return early. > > > > > > > Especially when the time is less than one time slice of Windows, which > > > > is typically 16 milliseconds on Windows 10. In our scenario, DX capturer > > uses > > > ~2 > > > > milliseconds for one frame, so we usually ask the thread to sleep 15 > > > > milliseconds. > > > > > > > > But I think this function should belong to webrtc/base. See > > > > https://cs.chromium.org/chromium/src/base/threading/platform_thread.h. > > > > > > > It states, > > If dwMilliseconds is less than the resolution of the system clock, the thread > > may sleep for less than the specified length of time. If dwMilliseconds is > > greater than one tick but less than two, the wait can be anywhere between one > > and two ticks, and so on. > > Here the resolution of the system means the time slice length of the OS. It > can > > be impacted by the timeBegionPeriod() function. > > Ah, I see. Sorry I missed it. Still, I don't think we really need this function. > If Sleep() returns too soon then EnsureFrameCaptured() will just call this > function again. If the AcquireNextFrame() works as we expected, yes, we do not need this function.
Description was changed from ========== Skips the first several frames in DxgiDuplicatorController A bug has been reported to complaint the ScreenCapturerWinDirectx cannot capture the first frame, which is used in "Report an issue" page. A simple solution here is to skip the first several frames. This change also removes the friend relationship between DxgiDuplicatorController / DxgiAdapterDuplicator / DxgiOutputDuplicator, which is not really necessary. BUG=682112 ========== to ========== Skips the first frame in DxgiDuplicatorController A bug has been reported to complaint the ScreenCapturerWinDirectx cannot capture the first frame, which is used in "Report an issue" page. A simple solution here is to skip the first frame. This change also removes the friend relationship between DxgiDuplicatorController / DxgiAdapterDuplicator / DxgiOutputDuplicator, which is not really necessary. BUG=682112 ==========
https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... File webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc (right): https://codereview.webrtc.org/2703123002/diff/60001/webrtc/modules/desktop_ca... webrtc/modules/desktop_capture/win/dxgi_duplicator_controller.cc:28: void SleepMoreThanMs(int milliseconds) { On 2017/02/24 00:40:40, Sergey Ulanov wrote: > On 2017/02/23 21:48:58, Hzj_jie wrote: > > On 2017/02/23 21:06:43, Sergey Ulanov wrote: > > > On 2017/02/23 20:52:11, Hzj_jie wrote: > > > > On 2017/02/23 19:07:30, Sergey Ulanov wrote: > > > > > Do you really need this function? webrtc::SleepMs() calls Sleep() and I > > > don't > > > > > think Sleep() can return early on windows. > > > > > > > > According to MSDN, the Sleep() function may still return before the time > we > > > > provided. > > > > > > Where does it say that? I was reading these page: > > > > > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms686298(v=vs.85).aspx > > > I don't see it saying anywhere it may return early. > > > > > > > Especially when the time is less than one time slice of Windows, which > > > > is typically 16 milliseconds on Windows 10. In our scenario, DX capturer > > uses > > > ~2 > > > > milliseconds for one frame, so we usually ask the thread to sleep 15 > > > > milliseconds. > > > > > > > > But I think this function should belong to webrtc/base. See > > > > https://cs.chromium.org/chromium/src/base/threading/platform_thread.h. > > > > > > > It states, > > If dwMilliseconds is less than the resolution of the system clock, the thread > > may sleep for less than the specified length of time. If dwMilliseconds is > > greater than one tick but less than two, the wait can be anywhere between one > > and two ticks, and so on. > > Here the resolution of the system means the time slice length of the OS. It > can > > be impacted by the timeBegionPeriod() function. > > Ah, I see. Sorry I missed it. Still, I don't think we really need this function. > If Sleep() returns too soon then EnsureFrameCaptured() will just call this > function again. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.webrtc.org/2703123002/#ps120001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487914853410970,
"parent_rev": "72c307a6de4d91d98328551a1cbf857bddfc20ac", "commit_rev":
"8697f9afbaa42c55f07331457f4427471f11a861"}
Message was sent while issue was closed.
Description was changed from ========== Skips the first frame in DxgiDuplicatorController A bug has been reported to complaint the ScreenCapturerWinDirectx cannot capture the first frame, which is used in "Report an issue" page. A simple solution here is to skip the first frame. This change also removes the friend relationship between DxgiDuplicatorController / DxgiAdapterDuplicator / DxgiOutputDuplicator, which is not really necessary. BUG=682112 ========== to ========== Skips the first frame in DxgiDuplicatorController A bug has been reported to complaint the ScreenCapturerWinDirectx cannot capture the first frame, which is used in "Report an issue" page. A simple solution here is to skip the first frame. This change also removes the friend relationship between DxgiDuplicatorController / DxgiAdapterDuplicator / DxgiOutputDuplicator, which is not really necessary. BUG=682112 Review-Url: https://codereview.webrtc.org/2703123002 Cr-Commit-Position: refs/heads/master@{#16815} Committed: https://chromium.googlesource.com/external/webrtc/+/8697f9afbaa42c55f07331457... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/8697f9afbaa42c55f07331457... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
