Wrap ScreenCapturer with ScreenCapturerDifferWrapper

Wrap ScreenCapturer with ScreenCapturerDifferWrapper
This change is to add an DesktopCapturerOptions accurate_updated_region() with
default value as false to indicate whether a pixel-wise differentiation is
required. And ScreenCapturer::Create() function will wrap the implementation
with ScreenCapturerDifferWrapper.

Chromoting will use this option to filter out unchanged frames.

BUG=314516
Committed: https://crrev.com/e9a3c7f43c697c3c1e7c8956baebeaf2a1a867f6
Cr-Commit-Position: refs/heads/master@{#14248}

[Hzj_jie, 2016-09-07 19:08:18]

Patchset #1 (id:1) has been deleted

[Hzj_jie, 2016-09-07 21:27:40]

Description was changed from

==========
[WebRTC] Wrap ScreenCapturerWinDirectx with ScreenCapturerDifferWrapper
Windows is known to return larger updated region. So this change is to add
ScreenCapturerWinDirectx::Create function to create a ScreenCapturerWinDirectx
wrapped by ScreenCapturerDifferWrapper.
After this change, ScreenCapturerWinDirectx should have a on-par performance on
the data size it returned with ScreenCapturerWinGDI.

BUG=314516
==========

to

==========
[WebRTC] Wrap ScreenCapturerWinDirectx with ScreenCapturerDifferWrapper
Windows is known to return larger updated region. So this change is to add
ScreenCapturerWinDirectx::Create function to create a ScreenCapturerWinDirectx
wrapped by ScreenCapturerDifferWrapper.
After this change, ScreenCapturerWinDirectx should have a on-par performance on
the data size it returned with ScreenCapturerWinGDI.

BUG=314516
==========

[Hzj_jie, 2016-09-07 21:27:40]

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

[Hzj_jie, 2016-09-07 21:29:17]

The android build breaks do not relate to this change. (gclient runhooks failed)

[Sergey Ulanov, 2016-09-07 22:47:23]

I'm not sure we need to worry about narrowing updated_region() for Desktop
Capture API. DesktopCaptureDevice uses it to optimize YUV conversion, but it
doesn't get passed to the encoder. YUV conversion is relatively cheap compared
to encoding and shouldn't be much more expensive than frame comparison
implemented in ScreenCapturerDifferWrapper. So I'm not sure if
ScreenCapturerDifferWrapper would provide any real benefits for that case. It's
still useful to have it for Chromoting where update_region is fed to the
encoder.

I think it's better to keep ScreenCapturer::Create() as is and let the caller
create ScreenCapturerDifferWrapper if necessary.

https://codereview.chromium.org/2314323002/diff/20001/webrtc/modules/desktop_...
File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right):

https://codereview.chromium.org/2314323002/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:35: return
std::unique_ptr<ScreenCapturer>();
return nullptr;

https://codereview.chromium.org/2314323002/diff/20001/webrtc/modules/desktop_...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:40: return
std::unique_ptr<ScreenCapturer>(new ScreenCapturerDifferWrapper(
I think it's better to keep this code in ScreenCapturer::Create(). It doesn't
look right that ScreenCapturerWinDirectx::Create() creates a capturer that isn't
ScreenCapturerWinDirectx

[Hzj_jie, 2016-09-07 23:33:15]

On 2016/09/07 22:47:23, Sergey Ulanov wrote:
> I'm not sure we need to worry about narrowing updated_region() for Desktop
> Capture API. DesktopCaptureDevice uses it to optimize YUV conversion, but it
> doesn't get passed to the encoder. YUV conversion is relatively cheap compared
> to encoding and shouldn't be much more expensive than frame comparison
> implemented in ScreenCapturerDifferWrapper. So I'm not sure if
> ScreenCapturerDifferWrapper would provide any real benefits for that case.
It's
> still useful to have it for Chromoting where update_region is fed to the
> encoder.
> 
> I think it's better to keep ScreenCapturer::Create() as is and let the caller
> create ScreenCapturerDifferWrapper if necessary.
> 
>
https://codereview.chromium.org/2314323002/diff/20001/webrtc/modules/desktop_...
> File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc
(right):
> 
>
https://codereview.chromium.org/2314323002/diff/20001/webrtc/modules/desktop_...
> webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:35: return
> std::unique_ptr<ScreenCapturer>();
> return nullptr;
> 
>
https://codereview.chromium.org/2314323002/diff/20001/webrtc/modules/desktop_...
> webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:40: return
> std::unique_ptr<ScreenCapturer>(new ScreenCapturerDifferWrapper(
> I think it's better to keep this code in ScreenCapturer::Create(). It doesn't
> look right that ScreenCapturerWinDirectx::Create() creates a capturer that
isn't
> ScreenCapturerWinDirectx

Emmm, that seems like a different scenario than chromoting. How about other
capturer implementations? Should we just remove Differ from them directly?
Typically Differ uses 5ms for each frame, it's not a trivial impact.

Another solution is to add an option in DesktopCaptureOptions to control whether
a ScreenCapturerDifferWrapper is necessary.

[Hzj_jie, 2016-09-08 23:18:22]

On 2016/09/07 23:33:15, Hzj_jie wrote:
> On 2016/09/07 22:47:23, Sergey Ulanov wrote:
> > I'm not sure we need to worry about narrowing updated_region() for Desktop
> > Capture API. DesktopCaptureDevice uses it to optimize YUV conversion, but it
> > doesn't get passed to the encoder. YUV conversion is relatively cheap
compared
> > to encoding and shouldn't be much more expensive than frame comparison
> > implemented in ScreenCapturerDifferWrapper. So I'm not sure if
> > ScreenCapturerDifferWrapper would provide any real benefits for that case.
> It's
> > still useful to have it for Chromoting where update_region is fed to the
> > encoder.
> > 
> > I think it's better to keep ScreenCapturer::Create() as is and let the
caller
> > create ScreenCapturerDifferWrapper if necessary.
> > 
> >
>
https://codereview.chromium.org/2314323002/diff/20001/webrtc/modules/desktop_...
> > File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/2314323002/diff/20001/webrtc/modules/desktop_...
> > webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:35: return
> > std::unique_ptr<ScreenCapturer>();
> > return nullptr;
> > 
> >
>
https://codereview.chromium.org/2314323002/diff/20001/webrtc/modules/desktop_...
> > webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:40: return
> > std::unique_ptr<ScreenCapturer>(new ScreenCapturerDifferWrapper(
> > I think it's better to keep this code in ScreenCapturer::Create(). It
doesn't
> > look right that ScreenCapturerWinDirectx::Create() creates a capturer that
> isn't
> > ScreenCapturerWinDirectx
> 
> Emmm, that seems like a different scenario than chromoting. How about other
> capturer implementations? Should we just remove Differ from them directly?
> Typically Differ uses 5ms for each frame, it's not a trivial impact.
> 
> Another solution is to add an option in DesktopCaptureOptions to control
whether
> a ScreenCapturerDifferWrapper is necessary.

Talked offline with Sergey.

I have tested the impact on my machine,
ScreenCapturerDifferWrapper uses ~1.5ms to do the differ. An accurate updated
region saves encoder about 2ms. The impact of ScreenCapturerDifferWrapper to
chromoting is not significant.

I will go ahead to make three changes,
1. Modify this change to add a new option accurate_updated_region() in
DesktopCaptureOptions, with default value false. If this option is true, we will
wrap the ScreenCapturer implementation with ScreenCapturerDifferWrapper.
2. Enable this option in Chromoting.
3. Remove Differ from each ScreenCapturer implementation.

The second and third changes need to be happened in a same release cycle,
otherwise, we will differentiate the frames twice.

[Hzj_jie, 2016-09-08 23:35:18]

Description was changed from

==========
[WebRTC] Wrap ScreenCapturerWinDirectx with ScreenCapturerDifferWrapper
Windows is known to return larger updated region. So this change is to add
ScreenCapturerWinDirectx::Create function to create a ScreenCapturerWinDirectx
wrapped by ScreenCapturerDifferWrapper.
After this change, ScreenCapturerWinDirectx should have a on-par performance on
the data size it returned with ScreenCapturerWinGDI.

BUG=314516
==========

to

==========
[WebRTC] Wrap ScreenCapturer with ScreenCapturerDifferWrapper
This change is to add an DesktopCapturerOptions accurate_updated_region() with
default value as false to indicate whether a pixel-wise differentiation is
required. And ScreenCapturer::Create() function will wrap the implementation
with ScreenCapturerDifferWrapper.

Chromoting will use this option to filter out unchanged frames.

BUG=314516
==========

[Hzj_jie, 2016-09-08 23:36:32]

Patchset #2 (id:40001) has been deleted

[Hzj_jie, 2016-09-08 23:40:06]

Patchset #2 (id:60001) has been deleted

[Hzj_jie, 2016-09-08 23:49:34]

Patchset #2 (id:80001) has been deleted

[Sergey Ulanov, 2016-09-09 00:03:40]

https://codereview.chromium.org/2314323002/diff/100001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/desktop_capture_options.h (right):

https://codereview.chromium.org/2314323002/diff/100001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/desktop_capture_options.h:76: // accurate updated
region.
This comment makes it sound like updated_region() will only contain pixels that
have changed, which is not true. Suggest rewording:

 // Flag that should be set if the consumer uses updated_region() 
 // and the capturer should try to provide correct updated_region()
 // for the frames it generates (e.g. by comparing each frame with 
 // the previous one).

https://codereview.chromium.org/2314323002/diff/100001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/desktop_capture_options.h:77: bool
accurate_updated_region() const { return accurate_updated_region_; }
I don't think 'accurate' is the word to use here. updated_region() is always an
approximation. 
Maybe detect_updated_region?

[Sergey Ulanov, 2016-09-09 17:33:51]

https://codereview.chromium.org/2314323002/diff/120001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/screen_capturer_win.cc (right):

https://codereview.chromium.org/2314323002/diff/120001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/screen_capturer_win.cc:27: if
(options.allow_directx_capturer()) {
Do you still need to check IsSupported() here?

https://codereview.chromium.org/2314323002/diff/120001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/screen_capturer_win.cc:30: if (!capturer) {
else is still appropriate here.

[Hzj_jie, 2016-09-14 19:06:36]

Description was changed from

==========
[WebRTC] Wrap ScreenCapturer with ScreenCapturerDifferWrapper
This change is to add an DesktopCapturerOptions accurate_updated_region() with
default value as false to indicate whether a pixel-wise differentiation is
required. And ScreenCapturer::Create() function will wrap the implementation
with ScreenCapturerDifferWrapper.

Chromoting will use this option to filter out unchanged frames.

BUG=314516
==========

to

==========
Wrap ScreenCapturer with ScreenCapturerDifferWrapper
This change is to add an DesktopCapturerOptions accurate_updated_region() with
default value as false to indicate whether a pixel-wise differentiation is
required. And ScreenCapturer::Create() function will wrap the implementation
with ScreenCapturerDifferWrapper.

Chromoting will use this option to filter out unchanged frames.

BUG=314516
==========

[Hzj_jie, 2016-09-14 19:35:31]

https://codereview.chromium.org/2314323002/diff/120001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/screen_capturer_win.cc (right):

https://codereview.chromium.org/2314323002/diff/120001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/screen_capturer_win.cc:27: if
(options.allow_directx_capturer()) {
On 2016/09/09 17:33:51, Sergey Ulanov wrote:
> Do you still need to check IsSupported() here?

Oh, yes, I still need to check IsSupport here.

This should be able to be caught by the test case. I will also update the
screen_capturer_unittest.cc

https://codereview.chromium.org/2314323002/diff/120001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/screen_capturer_win.cc:30: if (!capturer) {
On 2016/09/09 17:33:51, Sergey Ulanov wrote:
> else is still appropriate here.

Done.

[Sergey Ulanov, 2016-09-15 23:51:36]

lgtm when my comments are addressed

https://codereview.chromium.org/2314323002/diff/180001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/desktop_capture_options.h (right):

https://codereview.chromium.org/2314323002/diff/180001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/desktop_capture_options.h:77: // generates (e.g.
by comparing each frame with the previous one).
Currently this flag is ignored in WindowCapturer implementations. Please mention
it here and add a TODO to fix it.

https://codereview.chromium.org/2314323002/diff/180001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right):

https://codereview.chromium.org/2314323002/diff/180001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:19: #include
"webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h"
Don't need this include.

[Hzj_jie, 2016-09-16 00:31:18]

https://codereview.chromium.org/2314323002/diff/180001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/desktop_capture_options.h (right):

https://codereview.chromium.org/2314323002/diff/180001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/desktop_capture_options.h:77: // generates (e.g.
by comparing each frame with the previous one).
On 2016/09/15 23:51:35, Sergey Ulanov wrote:
> Currently this flag is ignored in WindowCapturer implementations. Please
mention
> it here and add a TODO to fix it.

Done.

https://codereview.chromium.org/2314323002/diff/180001/webrtc/modules/desktop...
File webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc (right):

https://codereview.chromium.org/2314323002/diff/180001/webrtc/modules/desktop...
webrtc/modules/desktop_capture/win/screen_capturer_win_directx.cc:19: #include
"webrtc/modules/desktop_capture/screen_capturer_differ_wrapper.h"
On 2016/09/15 23:51:36, Sergey Ulanov wrote:
> Don't need this include.

Oops, this file has only a comment change.

[Hzj_jie, 2016-09-16 00:31:30]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2016-09-16 00:31:30]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2314323002/#ps220001
(title: "Resolve review comments")

[commit-bot: I haz the power, 2016-09-16 00:31:32]

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, 2016-09-16 02:31:18]

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

[commit-bot: I haz the power, 2016-09-16 02:31:19]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[Hzj_jie, 2016-09-16 06:55:06]

The CQ bit was checked by zijiehe@chromium.org

[commit-bot: I haz the power, 2016-09-16 06:55:15]

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, 2016-09-16 07:03:16]

Description was changed from

==========
Wrap ScreenCapturer with ScreenCapturerDifferWrapper
This change is to add an DesktopCapturerOptions accurate_updated_region() with
default value as false to indicate whether a pixel-wise differentiation is
required. And ScreenCapturer::Create() function will wrap the implementation
with ScreenCapturerDifferWrapper.

Chromoting will use this option to filter out unchanged frames.

BUG=314516
==========

to

==========
Wrap ScreenCapturer with ScreenCapturerDifferWrapper
This change is to add an DesktopCapturerOptions accurate_updated_region() with
default value as false to indicate whether a pixel-wise differentiation is
required. And ScreenCapturer::Create() function will wrap the implementation
with ScreenCapturerDifferWrapper.

Chromoting will use this option to filter out unchanged frames.

BUG=314516
==========

[commit-bot: I haz the power, 2016-09-16 07:03:17]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2016-09-16 07:03:27]

Description was changed from

==========
Wrap ScreenCapturer with ScreenCapturerDifferWrapper
This change is to add an DesktopCapturerOptions accurate_updated_region() with
default value as false to indicate whether a pixel-wise differentiation is
required. And ScreenCapturer::Create() function will wrap the implementation
with ScreenCapturerDifferWrapper.

Chromoting will use this option to filter out unchanged frames.

BUG=314516
==========

to

==========
Wrap ScreenCapturer with ScreenCapturerDifferWrapper
This change is to add an DesktopCapturerOptions accurate_updated_region() with
default value as false to indicate whether a pixel-wise differentiation is
required. And ScreenCapturer::Create() function will wrap the implementation
with ScreenCapturerDifferWrapper.

Chromoting will use this option to filter out unchanged frames.

BUG=314516

Committed: https://crrev.com/e9a3c7f43c697c3c1e7c8956baebeaf2a1a867f6
Cr-Commit-Position: refs/heads/master@{#14248}
==========

[commit-bot: I haz the power, 2016-09-16 07:03:28]

Patchset 8 (id:??) landed as
https://crrev.com/e9a3c7f43c697c3c1e7c8956baebeaf2a1a867f6
Cr-Commit-Position: refs/heads/master@{#14248}