Simplify ScreenCapturer interface.

Simplify ScreenCapturer interface.

Removed Stop() and InvalidateRegion() from ScreenCapturer interface.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195887

[Sergey Ulanov, 2013-04-21 00:12:12]

+palmer@ for chromoting_messages.h

[Sergey Ulanov, 2013-04-21 00:12:13]

+palmer@ for chromoting_messages.h

[Wez, 2013-04-21 00:46:26]

On 2013/04/21 00:12:13, sergeyu wrote:
> +palmer@ for chromoting_messages.h

What's the rationale for removing those APIs?  How are capturers now stopped? 
And, if a capturer is "dumb", how does it know what region to capture?

[Sergey Ulanov, 2013-04-21 03:48:25]

On 2013/04/21 00:46:26, Wez wrote:
> What's the rationale for removing those APIs? How are capturers now stopped? 

InvalidateRegion() was never used. Stop() didn't make sense because because
frames are captured by request (i.e. one can "stop" it by not calling
CaptureFrame() :-)). Capturer can be deleted to free resources.

> And, if a capturer is "dumb", how does it know what region to capture?

I'll add an argument in Capture() that will be used to specify the region of the
screen that needs to be captured. WDYT?

[Wez, 2013-04-22 14:40:17]

On 2013/04/21 03:48:25, sergeyu wrote:
> On 2013/04/21 00:46:26, Wez wrote:
> > What's the rationale for removing those APIs? How are capturers now stopped?

> 
> InvalidateRegion() was never used.

Not in the current architecture, no, but that's because we never split out "dumb
capturers" from the dirty-notification based system.

Being able to explicitly mark areas for capture, even if the capturer already
handles dirty notifications, can be useful e.g. if the recipient can sometimes
lose its cache of the existing state of the framebuffer - in that case it simply
invalidates the entire framebuffer to cause the capturer to refresh it.

> Stop() didn't make sense because because
> frames are captured by request (i.e. one can "stop" it by not calling
> CaptureFrame() :-)). Capturer can be deleted to free resources.

SGTM.

> > And, if a capturer is "dumb", how does it know what region to capture?
> 
> I'll add an argument in Capture() that will be used to specify the region of
the
> screen that needs to be captured. WDYT?

See above; I prefer keeping InvalidateRegion() separate from CaptureFrame(), so
that a caller can just call CaptureFrame() and let the capturer do dirty
notifications, or poll, or if need be it can explicitly mark regions for
capture.  If you prefer a single API w/ an invalid region parameter then that's
OK, but please make sure the semantics are clear, e.g. does the region get added
to what would otherwise be captured, or replace it, etc.

[alexeypa (please no reviews), 2013-04-22 16:22:46]

> See above; I prefer keeping InvalidateRegion() separate from CaptureFrame(),
so
> that a caller can just call CaptureFrame() and let the capturer do dirty
> notifications, or poll, or if need be it can explicitly mark regions for
> capture.  If you prefer a single API w/ an invalid region parameter then
that's
> OK, but please make sure the semantics are clear, e.g. does the region get
added
> to what would otherwise be captured, or replace it, etc.

Keeping InvalidateRegion() separate from CaptureFrame() sounds like a useful
thing to do.

We can also completely decouple dirty notifications from the capturer. If you
think about it dirty notifications are most useful to VideoScheduler. For
instance it can skip a frame capture entirely if nothing has changed, or it can
re-capture some region of the remote side requested it.

Copying parts of the previous frame to the new one seems to be the only reason
why capturer needs to manage the frame queue and dirty notifications.

[alexeypa (please no reviews), 2013-04-22 16:23:18]

https://codereview.chromium.org/14305004/diff/4003/remoting/host/video_schedu...
File remoting/host/video_scheduler.cc (right):

https://codereview.chromium.org/14305004/diff/4003/remoting/host/video_schedu...
remoting/host/video_scheduler.cc:181: capturer_.reset();
|capturer_| can only be deleted only after the encoding thread stopped reading
the frames. Note that |capturer_| is passed to StopOnEncodeThread() and then to
StopOnNetworkThread().


This could be avoided if we move frames allocation (and frame queue management)
out of the capturer to the caller. The capturer can still provide code for
allocating frames (as a separate class most likely) but the caller would decide
when to allocate them. CaptureFrame() would check if the frame is compatible
with the current resolution and return an error if it is not.

This is out of scope for this CL I guess but it is something to consider.

[Wez, 2013-04-22 16:30:46]

On 2013/04/22 16:22:46, alexeypa wrote:
> > See above; I prefer keeping InvalidateRegion() separate from CaptureFrame(),
> so
> > that a caller can just call CaptureFrame() and let the capturer do dirty
> > notifications, or poll, or if need be it can explicitly mark regions for
> > capture.  If you prefer a single API w/ an invalid region parameter then
> that's
> > OK, but please make sure the semantics are clear, e.g. does the region get
> added
> > to what would otherwise be captured, or replace it, etc.
> 
> Keeping InvalidateRegion() separate from CaptureFrame() sounds like a useful
> thing to do.
> 
> We can also completely decouple dirty notifications from the capturer. If you
> think about it dirty notifications are most useful to VideoScheduler. For
> instance it can skip a frame capture entirely if nothing has changed, or it
can
> re-capture some region of the remote side requested it.

Separating the two things gets messy; it makes it harder to work with e.g. the
DXGI 1.2 capture interface, which combines frame-capture with dirty
notifications, and to make good use of non-idempotent dirty notifications (e.g.
screen-to-screen copy events).

> Copying parts of the previous frame to the new one seems to be the only reason
> why capturer needs to manage the frame queue and dirty notifications.

I'm actually planning to move that out of the core capturers, since AFAICT it's
not needed for Chromoting, since we use the active map.

[Wez, 2013-04-22 16:35:59]

On 2013/04/22 16:23:18, alexeypa wrote:
>
https://codereview.chromium.org/14305004/diff/4003/remoting/host/video_schedu...
> File remoting/host/video_scheduler.cc (right):
> 
>
https://codereview.chromium.org/14305004/diff/4003/remoting/host/video_schedu...
> remoting/host/video_scheduler.cc:181: capturer_.reset();
> |capturer_| can only be deleted only after the encoding thread stopped reading
> the frames. Note that |capturer_| is passed to StopOnEncodeThread() and then
to
> StopOnNetworkThread().
> 
> 
> This could be avoided if we move frames allocation (and frame queue
management)
> out of the capturer to the caller. The capturer can still provide code for
> allocating frames (as a separate class most likely) but the caller would
decide
> when to allocate them. CaptureFrame() would check if the frame is compatible
> with the current resolution and return an error if it is not.

I think it's best to have frame allocation handled by the capturers (possibly
using a caller-supplied buffer-allocator) but have frames be owned - when the
caller deletes a frame the implementation might post them back to the capturer
thread, either for the capturer to re-use, or to be torn down there, depending
upon the needs of the implementation.

> This is out of scope for this CL I guess but it is something to consider.

[Wez, 2013-04-22 16:36:00]

On 2013/04/22 16:23:18, alexeypa wrote:
>
https://codereview.chromium.org/14305004/diff/4003/remoting/host/video_schedu...
> File remoting/host/video_scheduler.cc (right):
> 
>
https://codereview.chromium.org/14305004/diff/4003/remoting/host/video_schedu...
> remoting/host/video_scheduler.cc:181: capturer_.reset();
> |capturer_| can only be deleted only after the encoding thread stopped reading
> the frames. Note that |capturer_| is passed to StopOnEncodeThread() and then
to
> StopOnNetworkThread().
> 
> 
> This could be avoided if we move frames allocation (and frame queue
management)
> out of the capturer to the caller. The capturer can still provide code for
> allocating frames (as a separate class most likely) but the caller would
decide
> when to allocate them. CaptureFrame() would check if the frame is compatible
> with the current resolution and return an error if it is not.

I think it's best to have frame allocation handled by the capturers (possibly
using a caller-supplied buffer-allocator) but have frames be owned - when the
caller deletes a frame the implementation might post them back to the capturer
thread, either for the capturer to re-use, or to be torn down there, depending
upon the needs of the implementation.

> This is out of scope for this CL I guess but it is something to consider.

[Sergey Ulanov, 2013-04-22 18:06:46]

On 2013/04/22 14:40:17, Wez wrote:
> On 2013/04/21 03:48:25, sergeyu wrote:
> > On 2013/04/21 00:46:26, Wez wrote:
> > > What's the rationale for removing those APIs? How are capturers now
stopped?
> 
> > 
> > InvalidateRegion() was never used.
> 
> Not in the current architecture, no, but that's because we never split out
"dumb
> capturers" from the dirty-notification based system.
> 
> Being able to explicitly mark areas for capture, even if the capturer already
> handles dirty notifications, can be useful e.g. if the recipient can sometimes
> lose its cache of the existing state of the framebuffer - in that case it
simply
> invalidates the entire framebuffer to cause the capturer to refresh it.
> 
> > Stop() didn't make sense because because
> > frames are captured by request (i.e. one can "stop" it by not calling
> > CaptureFrame() :-)). Capturer can be deleted to free resources.
> 
> SGTM.
> 
> > > And, if a capturer is "dumb", how does it know what region to capture?
> > 
> > I'll add an argument in Capture() that will be used to specify the region of
> the
> > screen that needs to be captured. WDYT?
> 
> See above; I prefer keeping InvalidateRegion() separate from CaptureFrame(),
so
> that a caller can just call CaptureFrame() and let the capturer do dirty
> notifications, or poll, or if need be it can explicitly mark regions for
> capture.  If you prefer a single API w/ an invalid region parameter then
that's
> OK, but please make sure the semantics are clear, e.g. does the region get
added
> to what would otherwise be captured, or replace it, etc.

The reason I don't like InvlidateRegion() is that it makes the dump captures
more complicated - they need to maintain state between captures for
InvlidateRegion(). There is also the question of what should happen if
InvalidateRegion() is called between Capture() and OnFrameCaptured().
Anyway InvlidateRegion() is not used at the moment, so it doesn't make sense to
keep it for now. Let's debate this issue further for the webrtc CL - will send
it to you today.

[Sergey Ulanov, 2013-04-22 18:12:50]

On 2013/04/22 16:36:00, Wez wrote:
> On 2013/04/22 16:23:18, alexeypa wrote:
> >
>
https://codereview.chromium.org/14305004/diff/4003/remoting/host/video_schedu...
> > File remoting/host/video_scheduler.cc (right):
> > 
> >
>
https://codereview.chromium.org/14305004/diff/4003/remoting/host/video_schedu...
> > remoting/host/video_scheduler.cc:181: capturer_.reset();
> > |capturer_| can only be deleted only after the encoding thread stopped
reading
> > the frames. Note that |capturer_| is passed to StopOnEncodeThread() and then
> to
> > StopOnNetworkThread().
> > 
> > 
> > This could be avoided if we move frames allocation (and frame queue
> management)
> > out of the capturer to the caller. The capturer can still provide code for
> > allocating frames (as a separate class most likely) but the caller would
> decide
> > when to allocate them. CaptureFrame() would check if the frame is compatible
> > with the current resolution and return an error if it is not.
> 
> I think it's best to have frame allocation handled by the capturers (possibly
> using a caller-supplied buffer-allocator) but have frames be owned - when the
> caller deletes a frame the implementation might post them back to the capturer
> thread, either for the capturer to re-use, or to be torn down there, depending
> upon the needs of the implementation.

I agree that it's better to keep frame allocation the capturer:
 - Whoever allocates the frame need to know screen size. We could add GetSize()
in the capturer interface, but that creates a race when screen size is changed
between GetSize() and Capture().
 - On Windows we need to allocate bitmap that is associated with the right
device context.

> 
> > This is out of scope for this CL I guess but it is something to consider.

[Sergey Ulanov, 2013-04-22 18:18:56]

https://codereview.chromium.org/14305004/diff/4003/remoting/host/video_schedu...
File remoting/host/video_scheduler.cc (right):

https://codereview.chromium.org/14305004/diff/4003/remoting/host/video_schedu...
remoting/host/video_scheduler.cc:181: capturer_.reset();
On 2013/04/22 16:23:19, alexeypa wrote:
> |capturer_| can only be deleted only after the encoding thread stopped reading
> the frames. Note that |capturer_| is passed to StopOnEncodeThread() and then
to
> StopOnNetworkThread().
> 

Oh, right. Do we need to call Stop() here at all?

With the refactoring I'm working on I'm going to fix this problem: it will be
safe to delete capturer while the frames still being processed (will upload it
for review today).

> 
> This could be avoided if we move frames allocation (and frame queue
management)
> out of the capturer to the caller. The capturer can still provide code for
> allocating frames (as a separate class most likely) but the caller would
decide
> when to allocate them. CaptureFrame() would check if the frame is compatible
> with the current resolution and return an error if it is not.
> 
> This is out of scope for this CL I guess but it is something to consider.

As I mentioned in previous response I think it's the captured who should be
allocating frames - only it has all details needed to allocated it properly.

[alexeypa (please no reviews), 2013-04-22 18:36:37]

lgtm, once the issue with stopping capturer after all frames have been processed
by the encoder is solved.

https://codereview.chromium.org/14305004/diff/4003/remoting/host/video_schedu...
File remoting/host/video_scheduler.cc (right):

https://codereview.chromium.org/14305004/diff/4003/remoting/host/video_schedu...
remoting/host/video_scheduler.cc:181: capturer_.reset();
> Oh, right. Do we need to call Stop() here at all?

Yes. It clears |delegate_| preventing the capturer from using 

> As I mentioned in previous response I think it's the captured who should be
> allocating frames - only it has all details needed to allocated it properly.
> 

It is true that frame allocation is capturer specific. What I don't like about
it is that the capturer has to manage the queue of frames. One issue that you
mentioned elsewhere - "what if InvalidateRegion() is called in between
CaptureFrame() and OnCaptureCompleted()?" is directly related to the frame queue
management. We "solve" it by making the capturer single-threaded. We, probably,
should express this arrangement in the capturer interface.

[palmer, 2013-04-22 19:58:49]

IPC security LGTM

[Sergey Ulanov, 2013-04-22 20:59:35]

On 2013/04/22 18:36:37, alexeypa wrote:
> lgtm, once the issue with stopping capturer after all frames have been
processed
> by the encoder is solved.

PTAL. Fixed VideoScheduler to delete the capturer on the capture thread only
after encoder is stopped (that's important for IpcVideoFrameCapturer) and to
ignore delete calls when it's being stopped.

[alexeypa (please no reviews), 2013-04-22 21:02:32]

lgtm

[commit-bot: I haz the power, 2013-04-22 21:40:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/14305004/33002

[commit-bot: I haz the power, 2013-04-22 22:14:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-22 22:28:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/14305004/24002

[commit-bot: I haz the power, 2013-04-23 00:05:09]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-23 18:54:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/14305004/24002

[Sergey Ulanov, 2013-04-23 19:57:49]

Committed patchset #5 manually as r195887 (presubmit successful).