[DesktopCapture] Detect screen resolution changes in DirectX capturer

[DesktopCapture] Detect screen resolution changes in DirectX capturer

This change adds a ResolutionChangeDetector to help dxgi components, say
DxgiDuplicatorController and DxgiTexture to detect resolution changes.

BUG=684162
Review-Url: https://codereview.webrtc.org/2682913002
Cr-Commit-Position: refs/heads/master@{#16654}
Committed: https://chromium.googlesource.com/external/webrtc/+/5fea5fb1836076d5df22a1f869ac8484d3b7dad0

[Hzj_jie, 2017-02-08 02:53:01]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 02:53:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-08 02:53:23]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-02-08 02:54:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 02:54:41]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/17226)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17052)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22488)

[Hzj_jie, 2017-02-08 03:26:07]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 03:26:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-08 03:26:22]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-02-08 03:27:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 03:27:44]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22489)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21239)

[Hzj_jie, 2017-02-08 03:49:35]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 03:49:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-08 03:49:45]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-02-08 03:51:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 03:51:09]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17054)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22490)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21240)

[Hzj_jie, 2017-02-08 04:22:26]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 04:22:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-08 04:22:49]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-02-08 04:23:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 04:24:00]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/17229)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17055)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/21241)

[Hzj_jie, 2017-02-08 04:28:42]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 04:28:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-08 04:28:53]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-02-08 04:30:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 04:30:20]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/17056)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22493)

[Hzj_jie, 2017-02-08 04:39:50]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 04:39:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-08 04:40:01]

Patchset #1 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-02-08 04:41:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 04:41:42]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22494)

[Hzj_jie, 2017-02-08 17:24:37]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 17:24:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-08 17:24:59]

Patchset #1 (id:120001) has been deleted

[commit-bot: I haz the power, 2017-02-08 18:38:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 18:38:56]

Dry run: This issue passed the CQ dry run.

[Hzj_jie, 2017-02-08 20:55:53]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 20:56:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-08 20:56:52]

Patchset #1 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-02-08 21:10:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 21:10:15]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)

[Hzj_jie, 2017-02-08 21:56:30]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 21:56:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-08 21:56:46]

Patchset #1 (id:160001) has been deleted

[Hzj_jie, 2017-02-08 22:19:52]

zijiehe@chromium.org changed reviewers:
+ jamiewalch@chromium.org, sergeyu@chromium.org

[commit-bot: I haz the power, 2017-02-08 23:06:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 23:06:52]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-02-09 00:16:57]

With this change one Capture() request will always fail when screen resolution
changes. Is it not possible to avoid it?

[Hzj_jie, 2017-02-09 00:18:47]

On 2017/02/09 00:16:57, Sergey Ulanov wrote:
> With this change one Capture() request will always fail when screen resolution
> changes. Is it not possible to avoid it?

I do not think so, the first IsChanged() function call will always return false,
i.e. the Capture() function call will succeed. Have I made a mistake here?

[Sergey Ulanov, 2017-02-09 00:59:30]

On 2017/02/09 00:18:47, Hzj_jie wrote:
> On 2017/02/09 00:16:57, Sergey Ulanov wrote:
> > With this change one Capture() request will always fail when screen
resolution
> > changes. Is it not possible to avoid it?
> 
> I do not think so, the first IsChanged() function call will always return
false,
> i.e. the Capture() function call will succeed. Have I made a mistake here?

There is no problem with the very first Capture() call. If I understand
correctly, after every screen resolution change the next Capture() request will
always fail with this change (because DxgiTexture::CopyFrom() fails), triggering
screen capturer to be reinitialized. Is there a way to avoid this failed
Capture() call?

[Hzj_jie, 2017-02-09 01:43:54]

On 2017/02/09 00:59:30, Sergey Ulanov wrote:
> On 2017/02/09 00:18:47, Hzj_jie wrote:
> > On 2017/02/09 00:16:57, Sergey Ulanov wrote:
> > > With this change one Capture() request will always fail when screen
> resolution
> > > changes. Is it not possible to avoid it?
> > 
> > I do not think so, the first IsChanged() function call will always return
> false,
> > i.e. the Capture() function call will succeed. Have I made a mistake here?
> 
> There is no problem with the very first Capture() call. If I understand
> correctly, after every screen resolution change the next Capture() request
will
> always fail with this change (because DxgiTexture::CopyFrom() fails),
triggering
> screen capturer to be reinitialized. Is there a way to avoid this failed
> Capture() call?

Got you. Sorry for the misunderstanding.
A possible solution is to add a resolution_change_detector_ also in
ScreenCapturerWinDirectx, and exports DxgiDuplicatorController::Deinitialize()
function. If the screen resolution has been changed, we can reinitialize dxgi
components in-place, instead of waiting for a CaptureFrame() failure.

We always need to reinitialize dxgi components once screen resolution changed.
The DesktopFrame in ScreenCapturerWinDirectx::frames_ is created based on
DxgiDuplicatorController::desktop_size(). And DX capturer is monitor-based, i.e.
each DxgiOutputDuplicator needs to maintain its desktop_rect_ in system level,
which can only be retrieved in DxgiAdapterDuplicator::DoInitialize().

[Hzj_jie, 2017-02-09 02:00:30]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 02:00:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-09 02:01:49]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 02:01:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-09 02:02:16]

Patchset #2 (id:200001) has been deleted

[Hzj_jie, 2017-02-09 02:37:30]

Updated.

[commit-bot: I haz the power, 2017-02-09 03:18:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 03:18:05]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-02-10 19:58:25]

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
File webrtc/modules/desktop_capture/win/dxgi_texture.h (right):

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/dxgi_texture.h:65: virtual bool DoRelease() =
0;
These two methods are intended to be overwritten by children. It's better to
make the protected to make this clear.

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right):

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:71:
GetScreenRect(kFullDesktopScreenId, L"").size())) {
indentation - this line should be indented 4 spaces more.

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:71:
GetScreenRect(kFullDesktopScreenId, L"").size())) {
Why is it kFullDesktopScreenId instead of current_screen_id_?
There are cases when resolution of the monitor we are capturing is changed,
while total resolution of desktop stays the same.

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:71:
GetScreenRect(kFullDesktopScreenId, L"").size())) {
std::wstring() for the second parameter

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:139: bool
ScreenCapturerWinDirectx::SelectSource(SourceId id) {
Should we also initialized duplicators and reset resolution_change_detector_
here?

[Hzj_jie, 2017-02-11 00:33:50]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-11 00:33:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-11 00:40:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-11 00:40:26]

Dry run: Try jobs failed on following builders:
  win_clang_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/10812)
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/22528)

[Hzj_jie, 2017-02-11 00:45:08]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-11 00:45:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-11 00:45:33]

Patchset #3 (id:240001) has been deleted

[commit-bot: I haz the power, 2017-02-11 01:00:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-11 01:00:26]

Dry run: Try jobs failed on following builders:
  win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)

[Hzj_jie, 2017-02-11 01:29:40]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[Hzj_jie, 2017-02-11 01:29:55]

Patchset #3 (id:260001) has been deleted

[commit-bot: I haz the power, 2017-02-11 01:29:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-11 01:51:22]

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
File webrtc/modules/desktop_capture/win/dxgi_texture.h (right):

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/dxgi_texture.h:65: virtual bool DoRelease() =
0;
On 2017/02/10 19:58:25, Sergey Ulanov wrote:
> These two methods are intended to be overwritten by children. It's better to
> make the protected to make this clear.

In C++, private functions are also overrideable, though not accessible. Do we
always prefer to use protected here? These two functions should not be called
directly by derived classes, though technically derived classes can still access
these functionalities.

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right):

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:71:
GetScreenRect(kFullDesktopScreenId, L"").size())) {
On 2017/02/10 19:58:25, Sergey Ulanov wrote:
> std::wstring() for the second parameter

Done.

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:71:
GetScreenRect(kFullDesktopScreenId, L"").size())) {
On 2017/02/10 19:58:25, Sergey Ulanov wrote:
> indentation - this line should be indented 4 spaces more.

Done.

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:71:
GetScreenRect(kFullDesktopScreenId, L"").size())) {
On 2017/02/10 19:58:25, Sergey Ulanov wrote:
> Why is it kFullDesktopScreenId instead of current_screen_id_?
> There are cases when resolution of the monitor we are capturing is changed,
> while total resolution of desktop stays the same.

Three reasons,
1. it's not guaranteed GetScreenRect() returns the same size as DX APIs.
2. current_screen_id_ comes from DX APIs, which does not have a same meaning as
the parameter of GetScreenRect().
3. It's not likely the total resolution won't change if resolutions of one or
more monitors have changed. But the worst case is a temporary capture attempt,
which does not really like a big deal.

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:139: bool
ScreenCapturerWinDirectx::SelectSource(SourceId id) {
On 2017/02/10 19:58:25, Sergey Ulanov wrote:
> Should we also initialized duplicators and reset resolution_change_detector_
> here?

The initialization is implicitly called in
DxgiDuplicatorController::Instance()->ScreenCount() function. As long as we are
using the entire screen resolution instead of a single monitor, we do not need
to reset the resolution_change_detector_ here.

[commit-bot: I haz the power, 2017-02-11 02:49:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-11 02:49:53]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-02-15 01:38:52]

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
File webrtc/modules/desktop_capture/win/dxgi_texture.h (right):

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/dxgi_texture.h:59: DXGI_MAPPED_RECT rect_ =
{0};
BTW, style guide doesn't allow protected data fields, only methods.

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/dxgi_texture.h:65: virtual bool DoRelease() =
0;
On 2017/02/11 01:51:22, Hzj_jie wrote:
> On 2017/02/10 19:58:25, Sergey Ulanov wrote:
> > These two methods are intended to be overwritten by children. It's better to
> > make the protected to make this clear.
> 
> In C++, private functions are also overrideable, though not accessible. Do we
> always prefer to use protected here? These two functions should not be called
> directly by derived classes, though technically derived classes can still
access
> these functionalities.

Right, private methods can still be overridden, but putting them in the
protected makes it clear that child classes need to override these methods .
Children can still call these methods directly because they will override them,
so you don't gain anything by declaring them in the private section here.

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right):

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:71:
GetScreenRect(kFullDesktopScreenId, L"").size())) {
On 2017/02/11 01:51:22, Hzj_jie wrote:
> On 2017/02/10 19:58:25, Sergey Ulanov wrote:
> > Why is it kFullDesktopScreenId instead of current_screen_id_?
> > There are cases when resolution of the monitor we are capturing is changed,
> > while total resolution of desktop stays the same.
> 
> Three reasons,
> 1. it's not guaranteed GetScreenRect() returns the same size as DX APIs.

They maybe we should use DX API to get the screen size?

> 2. current_screen_id_ comes from DX APIs, which does not have a same meaning
as
> the parameter of GetScreenRect().

Is it possible to get displays resolutions using DX API before capturing?

> 3. It's not likely the total resolution won't change if resolutions of one or
> more monitors have changed. But the worst case is a temporary capture attempt,
> which does not really like a big deal.

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:139: bool
ScreenCapturerWinDirectx::SelectSource(SourceId id) {
On 2017/02/11 01:51:22, Hzj_jie wrote:
> On 2017/02/10 19:58:25, Sergey Ulanov wrote:
> > Should we also initialized duplicators and reset resolution_change_detector_
> > here?
> 
> The initialization is implicitly called in
> DxgiDuplicatorController::Instance()->ScreenCount() function. As long as we
are
> using the entire screen resolution instead of a single monitor, we do not need
> to reset the resolution_change_detector_ here.

If there are two monitors with the same resoltion and SelectScreen() is called
to select second monitor (when first one one selected previously), does the
capturer need to be reinitialized in this case? AFAICT
ScreenCapturerWinDirectx::CaptureFrame() will try to reuse old frames in this
case, and I'm not sure it's the right thing to do.

[Hzj_jie, 2017-02-15 02:51:06]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 02:51:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-15 02:57:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 02:57:06]

Dry run: Try jobs failed on following builders:
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/22665)

[Hzj_jie, 2017-02-15 03:19:53]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 03:19:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-15 03:20:16]

Patchset #4 (id:300001) has been deleted

[Hzj_jie, 2017-02-15 03:53:16]

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
File webrtc/modules/desktop_capture/win/dxgi_texture.h (right):

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/dxgi_texture.h:65: virtual bool DoRelease() =
0;
On 2017/02/15 01:38:51, Sergey Ulanov wrote:
> On 2017/02/11 01:51:22, Hzj_jie wrote:
> > On 2017/02/10 19:58:25, Sergey Ulanov wrote:
> > > These two methods are intended to be overwritten by children. It's better
to
> > > make the protected to make this clear.
> > 
> > In C++, private functions are also overrideable, though not accessible. Do
we
> > always prefer to use protected here? These two functions should not be
called
> > directly by derived classes, though technically derived classes can still
> access
> > these functionalities.
> 
> Right, private methods can still be overridden, but putting them in the
> protected makes it clear that child classes need to override these methods .
> Children can still call these methods directly because they will override
them,
> so you don't gain anything by declaring them in the private section here.

I have no opinion on this. Indeed, some modern languages do no t support private
overridable functions, which resolves this issue permanently.
I have updated the code here.

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right):

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:71:
GetScreenRect(kFullDesktopScreenId, L"").size())) {
On 2017/02/15 01:38:52, Sergey Ulanov wrote:
> On 2017/02/11 01:51:22, Hzj_jie wrote:
> > On 2017/02/10 19:58:25, Sergey Ulanov wrote:
> > > Why is it kFullDesktopScreenId instead of current_screen_id_?
> > > There are cases when resolution of the monitor we are capturing is
changed,
> > > while total resolution of desktop stays the same.
> > 
> > Three reasons,
> > 1. it's not guaranteed GetScreenRect() returns the same size as DX APIs.
> 
> They maybe we should use DX API to get the screen size?
Yes, technically, we can do this. But we need to initialize
DxgiDuplicatorController, which makes this issue become a cycle dependency.
> 
> > 2. current_screen_id_ comes from DX APIs, which does not have a same meaning
> as
> > the parameter of GetScreenRect().
> 
> Is it possible to get displays resolutions using DX API before capturing?
Yes, we have already used DX APIs to set the resolution of frames.
> 
> > 3. It's not likely the total resolution won't change if resolutions of one
or
> > more monitors have changed. But the worst case is a temporary capture
attempt,
> > which does not really like a big deal.
>

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:139: bool
ScreenCapturerWinDirectx::SelectSource(SourceId id) {
On 2017/02/15 01:38:52, Sergey Ulanov wrote:
> On 2017/02/11 01:51:22, Hzj_jie wrote:
> > On 2017/02/10 19:58:25, Sergey Ulanov wrote:
> > > Should we also initialized duplicators and reset
resolution_change_detector_
> > > here?
> > 
> > The initialization is implicitly called in
> > DxgiDuplicatorController::Instance()->ScreenCount() function. As long as we
> are
> > using the entire screen resolution instead of a single monitor, we do not
need
> > to reset the resolution_change_detector_ here.
> 
> If there are two monitors with the same resoltion and SelectScreen() is called
> to select second monitor (when first one one selected previously), does the
> capturer need to be reinitialized in this case? AFAICT
> ScreenCapturerWinDirectx::CaptureFrame() will try to reuse old frames in this
> case, and I'm not sure it's the right thing to do.

No, we only need to reinitialize if a new monitor has been attached, which is
covered by line 70, or a monitor has been removed, which is covered by line 116
& 154, or the resolution of a monitor has been changed, which is covered in
DxgiTexture.

If you are concerning about whether we should reuse the frame, I have update the
code here.

[commit-bot: I haz the power, 2017-02-15 04:33:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 04:33:23]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-02-15 22:39:38]

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right):

https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:71:
GetScreenRect(kFullDesktopScreenId, L"").size())) {
On 2017/02/15 03:53:16, Hzj_jie wrote:
> On 2017/02/15 01:38:52, Sergey Ulanov wrote:
> > On 2017/02/11 01:51:22, Hzj_jie wrote:
> > > On 2017/02/10 19:58:25, Sergey Ulanov wrote:
> > > > Why is it kFullDesktopScreenId instead of current_screen_id_?
> > > > There are cases when resolution of the monitor we are capturing is
> changed,
> > > > while total resolution of desktop stays the same.
> > > 
> > > Three reasons,
> > > 1. it's not guaranteed GetScreenRect() returns the same size as DX APIs.
> > 
> > They maybe we should use DX API to get the screen size?
> Yes, technically, we can do this. But we need to initialize
> DxgiDuplicatorController, which makes this issue become a cycle dependency.

Not sure why it's a cyclic dependency. This function should:
 1. Make sure that DxgiDuplicatorController is initialized.
 2. Get current resolution for the target screen.
 3. Verify that the resolution hasn't changed from the previous frame, and reset
frame queue if it did.
 4. Capture new frame.

If I understand correctly currently we cannot do (2) because
DxgiDuplicatorController caches screen size internally so SelectedDesktopSize()
returns out-of-date value after size changes. But I don't see any reason it
couldn't be fixed.

Anyway, I don't mind much the current approach, but at very least this code
deserves a comment to explain why it's implemented this way. Also a TODO() to
improve it.


> > 
> > > 2. current_screen_id_ comes from DX APIs, which does not have a same
meaning
> > as
> > > the parameter of GetScreenRect().
> > 
> > Is it possible to get displays resolutions using DX API before capturing?
> Yes, we have already used DX APIs to set the resolution of frames.
> > 
> > > 3. It's not likely the total resolution won't change if resolutions of one
> or
> > > more monitors have changed. But the worst case is a temporary capture
> attempt,
> > > which does not really like a big deal.
> > 
>

[Hzj_jie, 2017-02-15 23:15:22]

On 2017/02/15 22:39:38, Sergey Ulanov wrote:
>
https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
> File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc
(right):
> 
>
https://codereview.webrtc.org/2682913002/diff/220001/webrtc/modules/desktop_c...
> webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:71:
> GetScreenRect(kFullDesktopScreenId, L"").size())) {
> On 2017/02/15 03:53:16, Hzj_jie wrote:
> > On 2017/02/15 01:38:52, Sergey Ulanov wrote:
> > > On 2017/02/11 01:51:22, Hzj_jie wrote:
> > > > On 2017/02/10 19:58:25, Sergey Ulanov wrote:
> > > > > Why is it kFullDesktopScreenId instead of current_screen_id_?
> > > > > There are cases when resolution of the monitor we are capturing is
> > changed,
> > > > > while total resolution of desktop stays the same.
> > > > 
> > > > Three reasons,
> > > > 1. it's not guaranteed GetScreenRect() returns the same size as DX APIs.
> > > 
> > > They maybe we should use DX API to get the screen size?
> > Yes, technically, we can do this. But we need to initialize
> > DxgiDuplicatorController, which makes this issue become a cycle dependency.
> 
> Not sure why it's a cyclic dependency. This function should:
>  1. Make sure that DxgiDuplicatorController is initialized.
>  2. Get current resolution for the target screen.
>  3. Verify that the resolution hasn't changed from the previous frame, and
reset
> frame queue if it did.
>  4. Capture new frame.
> 
> If I understand correctly currently we cannot do (2) because
> DxgiDuplicatorController caches screen size internally so
SelectedDesktopSize()
> returns out-of-date value after size changes. But I don't see any reason it
> couldn't be fixed.
> 
> Anyway, I don't mind much the current approach, but at very least this code
> deserves a comment to explain why it's implemented this way. Also a TODO() to
> improve it.
> 
> 
> > > 
> > > > 2. current_screen_id_ comes from DX APIs, which does not have a same
> meaning
> > > as
> > > > the parameter of GetScreenRect().
> > > 
> > > Is it possible to get displays resolutions using DX API before capturing?
> > Yes, we have already used DX APIs to set the resolution of frames.
> > > 
> > > > 3. It's not likely the total resolution won't change if resolutions of
one
> > or
> > > > more monitors have changed. But the worst case is a temporary capture
> > attempt,
> > > > which does not really like a big deal.
> > > 
> >

I will surely add comments. But I do think fixing it is pretty difficult,
because the functions return the screen resolutions are all initialization
functions. One is DuplicateOutput(), the other is EnumDevices / EnumOutputs. It
looks like these two functions are both expensive, and we do not want to call
them each time in CaptureFrame(). Indeed the initialization of dxgi components
is the combination of these functions.

[Sergey Ulanov, 2017-02-15 23:21:19]

On 2017/02/15 23:15:22, Hzj_jie wrote:
> I will surely add comments. But I do think fixing it is pretty difficult,
> because the functions return the screen resolutions are all initialization
> functions. One is DuplicateOutput(), the other is EnumDevices / EnumOutputs.
It
> looks like these two functions are both expensive, and we do not want to call
> them each time in CaptureFrame(). Indeed the initialization of dxgi components
> is the combination of these functions.

There is IDXGIOutput::GetDesc() and I think it can be used to get size of each
output and detect when layout changes.

[Hzj_jie, 2017-02-15 23:22:30]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 23:22:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-15 23:54:24]

The CQ bit was checked by zijiehe@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 23:54:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[Hzj_jie, 2017-02-15 23:57:30]

On 2017/02/15 23:21:19, Sergey Ulanov wrote:
> On 2017/02/15 23:15:22, Hzj_jie wrote:
> > I will surely add comments. But I do think fixing it is pretty difficult,
> > because the functions return the screen resolutions are all initialization
> > functions. One is DuplicateOutput(), the other is EnumDevices / EnumOutputs.
> It
> > looks like these two functions are both expensive, and we do not want to
call
> > them each time in CaptureFrame(). Indeed the initialization of dxgi
components
> > is the combination of these functions.
> 
> There is IDXGIOutput::GetDesc() and I think it can be used to get size of each
> output and detect when layout changes.

Yes, there are IDXGIOutput::GetDesc() and IDXGIOutputDuplication::GetDesc(),
both return the resolution of a single monitor. But I cannot tell whether the
results would be updated without reinitialization.
No matter, I have updated the comments.

[commit-bot: I haz the power, 2017-02-16 01:08:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 01:08:24]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2017-02-16 18:36:09]

lgtm

[Hzj_jie, 2017-02-16 20:04:39]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2017-02-16 20:04:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-16 20:07:45]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1487275479437560,
"parent_rev": "94b9600e2e72d2e90ba83b8086370d55f234d3a9", "commit_rev":
"5fea5fb1836076d5df22a1f869ac8484d3b7dad0"}

[commit-bot: I haz the power, 2017-02-16 20:07:49]

Description was changed from

==========
[DesktopCapture] Detect screen resolution changes in DirectX capturer

This change adds a ResolutionChangeDetector to help dxgi components, say
DxgiDuplicatorController and DxgiTexture to detect resolution changes.

BUG=684162
==========

to

==========
[DesktopCapture] Detect screen resolution changes in DirectX capturer

This change adds a ResolutionChangeDetector to help dxgi components, say
DxgiDuplicatorController and DxgiTexture to detect resolution changes.

BUG=684162

Review-Url: https://codereview.webrtc.org/2682913002
Cr-Commit-Position: refs/heads/master@{#16654}
Committed:
https://chromium.googlesource.com/external/webrtc/+/5fea5fb1836076d5df22a1f86...
==========

[commit-bot: I haz the power, 2017-02-16 20:07:51]

Committed patchset #6 (id:360001) as
https://chromium.googlesource.com/external/webrtc/+/5fea5fb1836076d5df22a1f86...