Initial pass at integrating Differ into the chromoting host code....

Initial pass at integrating Differ into the chromoting host code.

BUG=none
TEST=run Win host; x11 client


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55297

[awong, 2010-07-20 01:53:29]

Here's a first pass at comments.

I'm a bit undertain about exposing CalculateInvalidRects() in the public API.
Alpha, Dave, do you have thoughts?  I'm also wondering if Capturer in general
exposes too many APIs.  Really, I would have expected a CaptureFlow class with a
"Run" function. The class would have been a state machine that ran through each
of the functions as they concluded.  Then you'd only have 1 active flow at any
time...

But that's just me talking without context, so maybe I'm not making any sense.

http://codereview.chromium.org/3013015/diff/5004/30002
File remoting/host/capturer.cc (right):

http://codereview.chromium.org/3013015/diff/5004/30002#newcode30
remoting/host/capturer.cc:30: RectVector::iterator it;
Is it worth it to make these sets instead?

You keep using these like they are sets....

http://codereview.chromium.org/3013015/diff/5004/30002#newcode50
remoting/host/capturer.cc:50: if (it == inval_rects_.end()) {
This is O(n*m) isn't it?  Are we sure both lists really small?

If we used sets, we could do a set union. :)

http://codereview.chromium.org/3013015/diff/5004/30003
File remoting/host/capturer.h (right):

http://codereview.chromium.org/3013015/diff/5004/30003#newcode50
remoting/host/capturer.h:50: //     This is where the bits for the inval rects
are packaged up and sent
inval -> invalid

http://codereview.chromium.org/3013015/diff/5004/30003#newcode138
remoting/host/capturer.h:138: void InvalidateRect(const gfx::Rect& inval_rect);
Call this InvalidateSingleRect?

Having functions that differ by just one letter annoys me.

http://codereview.chromium.org/3013015/diff/5004/30003#newcode191
remoting/host/capturer.h:191: static const int NUM_SCREEN_BUFFERS = 2;
Why did we change conventions?  We use kStyleNaming for the rest of chrome code
don't we?

http://codereview.chromium.org/3013015/diff/5004/30003#newcode204
remoting/host/capturer.h:204: // Rects that have been manually invalidated
(through InvalidateRect).
Why are these not protected?  Having a bunch of protected member variables is a
bad code-smell to me.

http://codereview.chromium.org/3013015/diff/5004/30004
File remoting/host/capturer_fake.cc (right):

http://codereview.chromium.org/3013015/diff/5004/30004#newcode23
remoting/host/capturer_fake.cc:23: void
CapturerFake::ScreenConfigurationChanged() {
Do we need to DCHECK_EQ() the thread here?

http://codereview.chromium.org/3013015/diff/5004/30005
File remoting/host/capturer_fake.h (right):

http://codereview.chromium.org/3013015/diff/5004/30005#newcode24
remoting/host/capturer_fake.h:24: virtual void CalculateInvalidRects();
Should CalculateInvalidRects() be exposed?  Why can't CaptureRects just call it
internally when necessary?

http://codereview.chromium.org/3013015/diff/5004/30008
File remoting/host/capturer_gdi.cc (right):

http://codereview.chromium.org/3013015/diff/5004/30008#newcode13
remoting/host/capturer_gdi.cc:13: static const int PIXELS_PER_METER = 3780;
revert the style again?

http://codereview.chromium.org/3013015/diff/5004/30008#newcode89
remoting/host/capturer_gdi.cc:89: printf("CalculateInvalidRects\n");
printf?

Let's start using LOG* instead of printfs.

http://codereview.chromium.org/3013015/diff/5004/30008#newcode103
remoting/host/capturer_gdi.cc:103: void* prev_buffer =
static_cast<void*>(buffers_[prev_buffer_id]);
Why do we need a static_cast to void*?  Isn't buffers[i] already a void*?

http://codereview.chromium.org/3013015/diff/5004/30008#newcode106
remoting/host/capturer_gdi.cc:106: scoped_ptr<RectVector> rects(new
RectVector());
Allocate on the stack instead of newing with a scoped_ptr?

http://codereview.chromium.org/3013015/diff/5004/30008#newcode112
remoting/host/capturer_gdi.cc:112: inval_rects_ = *rects.get();
Suggest suing inval_rects.swap(rects) instead.  Avoids a wasted copy.

http://codereview.chromium.org/3013015/diff/5004/30008#newcode118
remoting/host/capturer_gdi.cc:118: //capture_fullscreen_ = false;
Can we remove this TODO now?

http://codereview.chromium.org/3013015/diff/5004/30008#newcode123
remoting/host/capturer_gdi.cc:123: printf("CaptureRects\n");
no more printfs!

http://codereview.chromium.org/3013015/diff/5004/30021
File remoting/host/session_manager.cc (right):

http://codereview.chromium.org/3013015/diff/5004/30021#newcode21
remoting/host/session_manager.cc:21: static const double kDefaultCaptureRate =
1.0;//20.0;
What's going on with the commented out value?

[Alpha Left Google, 2010-07-20 19:24:09]

http://codereview.chromium.org/3013015/diff/5004/30002
File remoting/host/capturer.cc (right):

http://codereview.chromium.org/3013015/diff/5004/30002#newcode31
remoting/host/capturer.cc:31: it = find(inval_rects_.begin(),
inval_rects_.end(), inval_rect);
I think you can use set or you can use the unique() in <algorithm> to eliminate
all duplicates.

http://codereview.chromium.org/3013015/diff/5004/30005
File remoting/host/capturer_fake.h (right):

http://codereview.chromium.org/3013015/diff/5004/30005#newcode25
remoting/host/capturer_fake.h:25: virtual void CaptureRects(const RectVector&
rects,
I think these two methods are protected or private.

http://codereview.chromium.org/3013015/diff/5004/30008
File remoting/host/capturer_gdi.cc (right):

http://codereview.chromium.org/3013015/diff/5004/30008#newcode96
remoting/host/capturer_gdi.cc:96: printf("  CalcDiffs\n");
Why printf here?

http://codereview.chromium.org/3013015/diff/5004/30008#newcode124
remoting/host/capturer_gdi.cc:124: Capturer::DataPlanes planes;
I think we need some explanation here since you assume that capture has already
been done.

http://codereview.chromium.org/3013015/diff/5004/30011
File remoting/host/capturer_linux.h (right):

http://codereview.chromium.org/3013015/diff/5004/30011#newcode18
remoting/host/capturer_linux.h:18: virtual void ScreenConfigurationChanged();
I think these methods are now private.

http://codereview.chromium.org/3013015/diff/5004/30021
File remoting/host/session_manager.cc (right):

http://codereview.chromium.org/3013015/diff/5004/30021#newcode213
remoting/host/session_manager.cc:213: capturer_->CalculateInvalidRects();
Instead of having these methods public, can we hide them in
CaptureInvalidRects()? SessionManager probably doesn't need to know the call
sequence.

Instead of having a no-op in CapturerMac for CalculateInvalidRects(), it will
just do the right thing inside.

What I think is needed to expose is:
1. InvalidRect()
2. CaptureInvalidRects()

Much of the implementation details like whether rect is provided or not can be
hidden inside the capturer.

[garykac, 2010-07-20 23:06:02]

Still fixing up some unittext failures due to the change from vector -> set. 
But this is ready for a checkup.

http://codereview.chromium.org/3013015/diff/5004/30002
File remoting/host/capturer.cc (right):

http://codereview.chromium.org/3013015/diff/5004/30002#newcode30
remoting/host/capturer.cc:30: RectVector::iterator it;
On 2010/07/20 01:53:30, awong wrote:
> Is it worth it to make these sets instead?
> 
> You keep using these like they are sets....

Hey! I'm just using what was there.  But you're right, I should have noticed
this and fixed it up. I've changed this to be a set.

http://codereview.chromium.org/3013015/diff/5004/30002#newcode50
remoting/host/capturer.cc:50: if (it == inval_rects_.end()) {
On 2010/07/20 01:53:30, awong wrote:
> This is O(n*m) isn't it?  Are we sure both lists really small?
> 
> If we used sets, we could do a set union. :)

Done.

http://codereview.chromium.org/3013015/diff/5004/30003
File remoting/host/capturer.h (right):

http://codereview.chromium.org/3013015/diff/5004/30003#newcode50
remoting/host/capturer.h:50: //     This is where the bits for the inval rects
are packaged up and sent
On 2010/07/20 01:53:30, awong wrote:
> inval -> invalid

Done.

http://codereview.chromium.org/3013015/diff/5004/30003#newcode138
remoting/host/capturer.h:138: void InvalidateRect(const gfx::Rect& inval_rect);
On 2010/07/20 01:53:30, awong wrote:
> Call this InvalidateSingleRect?
> 
> Having functions that differ by just one letter annoys me.

Removed. It was intended for convenience to avoid building a vector for a single
rect.

http://codereview.chromium.org/3013015/diff/5004/30003#newcode191
remoting/host/capturer.h:191: static const int NUM_SCREEN_BUFFERS = 2;
On 2010/07/20 01:53:30, awong wrote:
> Why did we change conventions?  We use kStyleNaming for the rest of chrome
code
> don't we?

Reverted. There was a recent thread about using UPPER_CASE, but that apparently
was only for enums.

http://codereview.chromium.org/3013015/diff/5004/30003#newcode204
remoting/host/capturer.h:204: // Rects that have been manually invalidated
(through InvalidateRect).
On 2010/07/20 01:53:30, awong wrote:
> Why are these not protected?  Having a bunch of protected member variables is
a
> bad code-smell to me.

Done.

http://codereview.chromium.org/3013015/diff/5004/30004
File remoting/host/capturer_fake.cc (right):

http://codereview.chromium.org/3013015/diff/5004/30004#newcode23
remoting/host/capturer_fake.cc:23: void
CapturerFake::ScreenConfigurationChanged() {
On 2010/07/20 01:53:30, awong wrote:
> Do we need to DCHECK_EQ() the thread here?

The capturer doesn't have the context to check its thread.  We could pass it in
and double-check, but I don't think there's a need since the SessionManager is
already performing these thread checks.

http://codereview.chromium.org/3013015/diff/5004/30005
File remoting/host/capturer_fake.h (right):

http://codereview.chromium.org/3013015/diff/5004/30005#newcode24
remoting/host/capturer_fake.h:24: virtual void CalculateInvalidRects();
On 2010/07/20 01:53:30, awong wrote:
> Should CalculateInvalidRects() be exposed?  Why can't CaptureRects just call
it
> internally when necessary?

Moved private

http://codereview.chromium.org/3013015/diff/5004/30005#newcode25
remoting/host/capturer_fake.h:25: virtual void CaptureRects(const RectVector&
rects,
On 2010/07/20 19:24:09, Alpha wrote:
> I think these two methods are protected or private.

Done.

http://codereview.chromium.org/3013015/diff/5004/30008
File remoting/host/capturer_gdi.cc (right):

http://codereview.chromium.org/3013015/diff/5004/30008#newcode13
remoting/host/capturer_gdi.cc:13: static const int PIXELS_PER_METER = 3780;
On 2010/07/20 01:53:30, awong wrote:
> revert the style again?

Done.

http://codereview.chromium.org/3013015/diff/5004/30008#newcode89
remoting/host/capturer_gdi.cc:89: printf("CalculateInvalidRects\n");
On 2010/07/20 01:53:30, awong wrote:
> printf?
> 
> Let's start using LOG* instead of printfs.

This was temp debugging stuff.  LOGs can get lost in the spew of LOG messages,
so printfs can be easier to find. This was supposed to be deleted, however...

http://codereview.chromium.org/3013015/diff/5004/30008#newcode96
remoting/host/capturer_gdi.cc:96: printf("  CalcDiffs\n");
On 2010/07/20 19:24:09, Alpha wrote:
> Why printf here?

Done.

http://codereview.chromium.org/3013015/diff/5004/30008#newcode103
remoting/host/capturer_gdi.cc:103: void* prev_buffer =
static_cast<void*>(buffers_[prev_buffer_id]);
On 2010/07/20 01:53:30, awong wrote:
> Why do we need a static_cast to void*?  Isn't buffers[i] already a void*?

Done.

http://codereview.chromium.org/3013015/diff/5004/30008#newcode106
remoting/host/capturer_gdi.cc:106: scoped_ptr<RectVector> rects(new
RectVector());
On 2010/07/20 01:53:30, awong wrote:
> Allocate on the stack instead of newing with a scoped_ptr?

Done.

http://codereview.chromium.org/3013015/diff/5004/30008#newcode112
remoting/host/capturer_gdi.cc:112: inval_rects_ = *rects.get();
On 2010/07/20 01:53:30, awong wrote:
> Suggest suing inval_rects.swap(rects) instead.  Avoids a wasted copy.

Made this a call to InvalidateRects to avoid have inval_rects_ visible here.

http://codereview.chromium.org/3013015/diff/5004/30008#newcode118
remoting/host/capturer_gdi.cc:118: //capture_fullscreen_ = false;
On 2010/07/20 01:53:30, awong wrote:
> Can we remove this TODO now?

x11client won't be happy, but sure.

http://codereview.chromium.org/3013015/diff/5004/30008#newcode123
remoting/host/capturer_gdi.cc:123: printf("CaptureRects\n");
On 2010/07/20 01:53:30, awong wrote:
> no more printfs!

Done.

http://codereview.chromium.org/3013015/diff/5004/30008#newcode124
remoting/host/capturer_gdi.cc:124: Capturer::DataPlanes planes;
On 2010/07/20 19:24:09, Alpha wrote:
> I think we need some explanation here since you assume that capture has
already
> been done.

Done.

http://codereview.chromium.org/3013015/diff/5004/30011
File remoting/host/capturer_linux.h (right):

http://codereview.chromium.org/3013015/diff/5004/30011#newcode18
remoting/host/capturer_linux.h:18: virtual void ScreenConfigurationChanged();
On 2010/07/20 19:24:09, Alpha wrote:
> I think these methods are now private.

Done.

http://codereview.chromium.org/3013015/diff/5004/30021
File remoting/host/session_manager.cc (right):

http://codereview.chromium.org/3013015/diff/5004/30021#newcode21
remoting/host/session_manager.cc:21: static const double kDefaultCaptureRate =
1.0;//20.0;
On 2010/07/20 01:53:30, awong wrote:
> What's going on with the commented out value?

Leftover debug stuff.

http://codereview.chromium.org/3013015/diff/5004/30021#newcode213
remoting/host/session_manager.cc:213: capturer_->CalculateInvalidRects();
On 2010/07/20 19:24:09, Alpha wrote:
> Instead of having these methods public, can we hide them in
> CaptureInvalidRects()? SessionManager probably doesn't need to know the call
> sequence.
> 
> Instead of having a no-op in CapturerMac for CalculateInvalidRects(), it will
> just do the right thing inside.
> 
> What I think is needed to expose is:
> 1. InvalidRect()
> 2. CaptureInvalidRects()
> 
> Much of the implementation details like whether rect is provided or not can be
> hidden inside the capturer.

Done.

[Alpha Left Google, 2010-07-20 23:55:13]

LGTM after a few changes.

http://codereview.chromium.org/3013015/diff/38012/27023
File remoting/host/client_connection.cc (right):

http://codereview.chromium.org/3013015/diff/38012/27023#newcode49
remoting/host/client_connection.cc:49: printf("SendInitClientMessage\n");
remove these printfs.

http://codereview.chromium.org/3013015/diff/38012/27029
File remoting/host/mock_objects.h (right):

http://codereview.chromium.org/3013015/diff/38012/27029#newcode25
remoting/host/mock_objects.h:25: MOCK_METHOD0(CalculateInvalidRects, void());
I think this is not public anymore so you can remove this.

[garykac, 2010-07-21 00:12:34]

http://codereview.chromium.org/3013015/diff/38012/27023
File remoting/host/client_connection.cc (right):

http://codereview.chromium.org/3013015/diff/38012/27023#newcode49
remoting/host/client_connection.cc:49: printf("SendInitClientMessage\n");
On 2010/07/20 23:55:13, Alpha wrote:
> remove these printfs.

Done.

http://codereview.chromium.org/3013015/diff/38012/27029
File remoting/host/mock_objects.h (right):

http://codereview.chromium.org/3013015/diff/38012/27029#newcode25
remoting/host/mock_objects.h:25: MOCK_METHOD0(CalculateInvalidRects, void());
On 2010/07/20 23:55:13, Alpha wrote:
> I think this is not public anymore so you can remove this.

Needs to be implemented by MockCapturer because it's a pure virtual method.