Enable cropping window capturing for Win7 when Aero is disabled.

Enable cropping window capturing for Win7 when Aero is disabled.

BUG=webrtc:496110
R=sergeyu@chromium.org

Committed: https://chromium.googlesource.com/external/webrtc/+/d848d5e74aee56f454311756dc2032fad26d79ad

[jiayl2, 2015-06-22 21:55:34]

jiayl@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[jiayl2, 2015-06-22 21:55:35]

Any idea why I cannot start try jobs for this CL?

[jiayl2, 2015-07-08 22:33:43]

jiayl@webrtc.org changed reviewers:
- kjellander@webrtc.org

[jiayl2, 2015-07-09 17:06:37]

Patchset #1 (id:1) has been deleted

[jiayl2, 2015-07-09 17:06:44]

Patchset #1 (id:20001) has been deleted

[jiayl2, 2015-07-09 17:07:44]

jiayl@webrtc.org changed reviewers:
+ sergeyu@chromium.org

[jiayl2, 2015-07-09 17:07:45]

PTAL

[Sergey Ulanov, 2015-07-09 18:41:39]

https://codereview.webrtc.org/1199073002/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/win/window_capture_utils.cc (right):

https://codereview.webrtc.org/1199073002/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/win/window_capture_utils.cc:51: static HMODULE
dwmapi_library = LoadLibrary(L"dwmapi.dll");
IsAeroEnabled() is called for every frame. There is probably some performance
penalty for calling LoadLibrary() every time. I think it's better to load the
DLL only once and keep it loaded after that.

[jiayl2, 2015-07-09 18:44:57]

https://codereview.webrtc.org/1199073002/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/win/window_capture_utils.cc (right):

https://codereview.webrtc.org/1199073002/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/win/window_capture_utils.cc:51: static HMODULE
dwmapi_library = LoadLibrary(L"dwmapi.dll");
On 2015/07/09 18:41:38, Sergey Ulanov wrote:
> IsAeroEnabled() is called for every frame. There is probably some performance
> penalty for calling LoadLibrary() every time. I think it's better to load the
> DLL only once and keep it loaded after that.

LoadLibrary is called only once since it's static.

[Sergey Ulanov, 2015-07-09 23:02:39]

https://codereview.webrtc.org/1199073002/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/win/window_capture_utils.cc (right):

https://codereview.webrtc.org/1199073002/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/win/window_capture_utils.cc:51: static HMODULE
dwmapi_library = LoadLibrary(L"dwmapi.dll");
On 2015/07/09 18:44:57, jiayl2 wrote:
> On 2015/07/09 18:41:38, Sergey Ulanov wrote:
> > IsAeroEnabled() is called for every frame. There is probably some
performance
> > penalty for calling LoadLibrary() every time. I think it's better to load
the
> > DLL only once and keep it loaded after that.
> 
> LoadLibrary is called only once since it's static.

Ah I missed that. Then the problem is that it's not thread-safe (see
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static...).
I think you want something similar to NSSContext::Instance() to initialize it
safely here.
Also you want to avoid calling GetProcAddress() every time as well.

[Sergey Ulanov, 2015-07-09 23:06:24]

https://codereview.webrtc.org/1199073002/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/win/window_capture_utils.cc (right):

https://codereview.webrtc.org/1199073002/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/win/window_capture_utils.cc:51: static HMODULE
dwmapi_library = LoadLibrary(L"dwmapi.dll");
On 2015/07/09 23:02:39, Sergey Ulanov wrote:
> On 2015/07/09 18:44:57, jiayl2 wrote:
> > On 2015/07/09 18:41:38, Sergey Ulanov wrote:
> > > IsAeroEnabled() is called for every frame. There is probably some
> performance
> > > penalty for calling LoadLibrary() every time. I think it's better to load
> the
> > > DLL only once and keep it loaded after that.
> > 
> > LoadLibrary is called only once since it's static.
> 
> Ah I missed that. Then the problem is that it's not thread-safe (see
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static...).
> I think you want something similar to NSSContext::Instance() to initialize it
> safely here.

Or you can also use this pattern:
https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.cc&l=30

> Also you want to avoid calling GetProcAddress() every time as well.

[jiayl2, 2015-07-13 22:37:33]

Patchset #2 (id:60001) has been deleted

[jiayl2, 2015-07-13 22:37:41]

Patchset #2 (id:80001) has been deleted

[jiayl2, 2015-07-13 22:48:19]

Patchset #2 (id:100001) has been deleted

[jiayl2, 2015-07-13 23:24:34]

PTAL

https://codereview.webrtc.org/1199073002/diff/40001/webrtc/modules/desktop_ca...
File webrtc/modules/desktop_capture/win/window_capture_utils.cc (right):

https://codereview.webrtc.org/1199073002/diff/40001/webrtc/modules/desktop_ca...
webrtc/modules/desktop_capture/win/window_capture_utils.cc:51: static HMODULE
dwmapi_library = LoadLibrary(L"dwmapi.dll");
On 2015/07/09 23:06:23, Sergey Ulanov wrote:
> On 2015/07/09 23:02:39, Sergey Ulanov wrote:
> > On 2015/07/09 18:44:57, jiayl2 wrote:
> > > On 2015/07/09 18:41:38, Sergey Ulanov wrote:
> > > > IsAeroEnabled() is called for every frame. There is probably some
> > performance
> > > > penalty for calling LoadLibrary() every time. I think it's better to
load
> > the
> > > > DLL only once and keep it loaded after that.
> > > 
> > > LoadLibrary is called only once since it's static.
> > 
> > Ah I missed that. Then the problem is that it's not thread-safe (see
> >
>
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static...).
> > I think you want something similar to NSSContext::Instance() to initialize
it
> > safely here.
> 
> Or you can also use this pattern:
> https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.cc&l=30
> 
> > Also you want to avoid calling GetProcAddress() every time as well.
> 

Done.

[Sergey Ulanov, 2015-07-14 20:15:51]

https://codereview.chromium.org/1199073002/diff/120001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/win/window_capture_utils.cc (right):

https://codereview.chromium.org/1199073002/diff/120001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/win/window_capture_utils.cc:51: static
rtc::GlobalLockPod lock;
I'm not sure it's safe to have this static inside the function. Make it global?

https://codereview.chromium.org/1199073002/diff/120001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/win/window_capture_utils.cc:64:
is_composition_enabled_func(&aero_enabled);
You can store is_composition_enabled_func between IsAeroEnabled() calls, but you
should call it every time instead of remembering the result. Aero can be
enabled/disabled dynamically without restarting any applications.

[jiayl2, 2015-07-14 22:03:37]

Patchset #3 (id:140001) has been deleted

[jiayl2, 2015-07-14 22:25:32]

PTAL

https://codereview.webrtc.org/1199073002/diff/120001/webrtc/modules/desktop_c...
File webrtc/modules/desktop_capture/win/window_capture_utils.cc (right):

https://codereview.webrtc.org/1199073002/diff/120001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/window_capture_utils.cc:51: static
rtc::GlobalLockPod lock;
On 2015/07/14 20:15:50, Sergey Ulanov wrote:
> I'm not sure it's safe to have this static inside the function. Make it
global?

Removed

https://codereview.webrtc.org/1199073002/diff/120001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/window_capture_utils.cc:64:
is_composition_enabled_func(&aero_enabled);
On 2015/07/14 20:15:50, Sergey Ulanov wrote:
> You can store is_composition_enabled_func between IsAeroEnabled() calls, but
you
> should call it every time instead of remembering the result. Aero can be
> enabled/disabled dynamically without restarting any applications.

The static method way doesn't work since it doesn't know when to free the
library.
Changed to use a object to hold the library/function.

[Sergey Ulanov, 2015-07-14 23:11:17]

https://codereview.webrtc.org/1199073002/diff/160001/webrtc/modules/desktop_c...
File webrtc/modules/desktop_capture/win/window_capture_utils.cc (right):

https://codereview.webrtc.org/1199073002/diff/160001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/window_capture_utils.cc:46:
AeroChecker::AeroChecker() : dwmapi_library_(NULL), func_(NULL) {}
nit: s/NULL/nullptr/

https://codereview.webrtc.org/1199073002/diff/160001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/window_capture_utils.cc:57: dwmapi_library_ =
LoadLibrary(L"dwmapi.dll");
Move this to the constructor so you don't try to load the library every time on
XP.

[jiayl2, 2015-07-15 23:15:21]

PTAL

https://codereview.webrtc.org/1199073002/diff/160001/webrtc/modules/desktop_c...
File webrtc/modules/desktop_capture/win/window_capture_utils.cc (right):

https://codereview.webrtc.org/1199073002/diff/160001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/window_capture_utils.cc:46:
AeroChecker::AeroChecker() : dwmapi_library_(NULL), func_(NULL) {}
On 2015/07/14 23:11:16, Sergey Ulanov wrote:
> nit: s/NULL/nullptr/

Done.

https://codereview.webrtc.org/1199073002/diff/160001/webrtc/modules/desktop_c...
webrtc/modules/desktop_capture/win/window_capture_utils.cc:57: dwmapi_library_ =
LoadLibrary(L"dwmapi.dll");
On 2015/07/14 23:11:16, Sergey Ulanov wrote:
> Move this to the constructor so you don't try to load the library every time
on
> XP.

Done.

[Sergey Ulanov, 2015-07-16 00:39:04]

lgtm

[jiayl2, 2015-07-16 15:49:51]

Committed patchset #4 (id:180001) manually as
d848d5e74aee56f454311756dc2032fad26d79ad (presubmit successful).