Move decoder into separate thread, clean up API layering, and redo update protocl

This is a monster CL.

It started as an attempt to put the decoder onto another thread.  However, this became complicated due to multiple object ownership transfers and coupling between the decode layer and the network layer; the decoder's states were highly coupled with how the network packets were processed.

This could probably be broken up slightly, but at this point, it's easier to just commit as a whole  The refactor includes:
  1) Making the decoder interface unaware of "network packet" types.
  2) Making the network layer process packets in order.
  3) Threading through asynchronous APIs all over the place.
  4) Simplifying the rectangle update protocol.
  5) Cleaning up object lifetime and ownership semantics between the decode layer and the renderer.

As of right now, the Verbatim format is still broken on the encode side because it uses the old protocol.

BUG=52883, 57351
TEST=still connects to chromoting_simple_host

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

[awong, 2010-09-02 23:23:26]

This is going to be a completely monster CL.

Here's a snapshot.  If I check this in as is, the ToT encoder will not work with
the ToT decoder.

If you really want me to break it up, let me know.

[awong, 2010-09-04 00:25:54]

Okay, zlib decoder/encoder work again for windows.

I need to fix mac and unittests, but this is ready for an initial pass.

Please focus on architecture.  I'm going to make a pass myself to fix naming,
etc.

[awong, 2010-09-28 00:46:03]

This is ready for review.

Since there is soo much code churn, I'm going to bias towards adding TODOs and
filing bugs for anything major.

Please let me know if you have any questions.

[garykac, 2010-09-28 19:06:21]

LGTM + nits

http://codereview.chromium.org/3305001/diff/22001/23003
File remoting/base/decoder_row_based.cc (right):

http://codereview.chromium.org/3305001/diff/22001/23003#newcode50
remoting/base/decoder_row_based.cc:50: const gfx::Rect& clip, int
bytes_per_src_pixel) {
nit: align

http://codereview.chromium.org/3305001/diff/22001/23003#newcode88
remoting/base/decoder_row_based.cc:88: // TODO(ajwong): This should be
bytes_per_dst_pixel shouldn't this.?
Yes. and it looks like stride should be something like dst_stride as well.

http://codereview.chromium.org/3305001/diff/22001/23003#newcode100
remoting/base/decoder_row_based.cc:100: in + used, in_size - used, out +
row_pos_, row_size - row_pos_,
row_size is clip-width * bytes_per_src_pixel. It seems like this should be based
on dst_pixel_width.

This code assumes src/dst pixel sizes and strides are the same.

http://codereview.chromium.org/3305001/diff/22001/23004
File remoting/base/decoder_row_based.h (right):

http://codereview.chromium.org/3305001/diff/22001/23004#newcode36
remoting/base/decoder_row_based.h:36: enum State {
Can we rename this to something like DecoderState?

http://codereview.chromium.org/3305001/diff/22001/23004#newcode39
remoting/base/decoder_row_based.h:39: kError,
Do we care about potential conflicts with other kReady or kError definitions? 
If so, kDecoderUninitialized, kDecoderReady and kDecoderError might be better.

http://codereview.chromium.org/3305001/diff/22001/23018
File remoting/base/encoder_zlib.h (right):

http://codereview.chromium.org/3305001/diff/22001/23018#newcode34
remoting/base/encoder_zlib.h:34: // Markes a packets as the first in a series of
rectangle updates.
Marks

http://codereview.chromium.org/3305001/diff/22001/23018#newcode39
remoting/base/encoder_zlib.h:39: // encoded rectangle data.  Will resize the
buffer to size.
resize buffer to |size|.

http://codereview.chromium.org/3305001/diff/22001/23023
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/3305001/diff/22001/23023#newcode101
remoting/client/chromoting_client.h:101: bool message_being_processed_;
This is safe only if it is only accessed on message_loop(). Do we have a
commenting convention for noting that?

http://codereview.chromium.org/3305001/diff/22001/23036
File remoting/host/capturer.cc (right):

http://codereview.chromium.org/3305001/diff/22001/23036#newcode48
remoting/host/capturer.cc:48: TraceContext::tracer()->PrintString("Started
CalculateInvalidRects");
We'll probably need to remove these trace statements at some point because the
string data will make the binary larger than it needs to be.

[awong, 2010-09-30 01:55:54]

address comments, rebased, and.... disabled unittests + vp8 from the build. :(
:( :(

Bugs appropriately filed.  Will fix next after this checkin.

Assuming the try bots go green, I will commit.

http://codereview.chromium.org/3305001/diff/22001/23003
File remoting/base/decoder_row_based.cc (right):

http://codereview.chromium.org/3305001/diff/22001/23003#newcode50
remoting/base/decoder_row_based.cc:50: const gfx::Rect& clip, int
bytes_per_src_pixel) {
On 2010/09/28 19:06:21, garykac wrote:
> nit: align

Done.

http://codereview.chromium.org/3305001/diff/22001/23003#newcode100
remoting/base/decoder_row_based.cc:100: in + used, in_size - used, out +
row_pos_, row_size - row_pos_,
On 2010/09/28 19:06:21, garykac wrote:
> row_size is clip-width * bytes_per_src_pixel. It seems like this should be
based
> on dst_pixel_width.
> 
> This code assumes src/dst pixel sizes and strides are the same.


Yep...added a TODO.

http://codereview.chromium.org/3305001/diff/22001/23004
File remoting/base/decoder_row_based.h (right):

http://codereview.chromium.org/3305001/diff/22001/23004#newcode36
remoting/base/decoder_row_based.h:36: enum State {
On 2010/09/28 19:06:21, garykac wrote:
> Can we rename this to something like DecoderState?

Since it's a private enum, I think it's okay...

http://codereview.chromium.org/3305001/diff/22001/23004#newcode39
remoting/base/decoder_row_based.h:39: kError,
On 2010/09/28 19:06:21, garykac wrote:
> Do we care about potential conflicts with other kReady or kError definitions? 
> If so, kDecoderUninitialized, kDecoderReady and kDecoderError might be better.

Same thing...since it's a private enum scoped to the class, pretty sure it's
okay.

http://codereview.chromium.org/3305001/diff/22001/23018
File remoting/base/encoder_zlib.h (right):

http://codereview.chromium.org/3305001/diff/22001/23018#newcode34
remoting/base/encoder_zlib.h:34: // Markes a packets as the first in a series of
rectangle updates.
On 2010/09/28 19:06:21, garykac wrote:
> Marks

Done.

http://codereview.chromium.org/3305001/diff/22001/23018#newcode39
remoting/base/encoder_zlib.h:39: // encoded rectangle data.  Will resize the
buffer to size.
On 2010/09/28 19:06:21, garykac wrote:
> resize buffer to |size|.

Done.

http://codereview.chromium.org/3305001/diff/22001/23023
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/3305001/diff/22001/23023#newcode101
remoting/client/chromoting_client.h:101: bool message_being_processed_;
On 2010/09/28 19:06:21, garykac wrote:
> This is safe only if it is only accessed on message_loop(). Do we have a
> commenting convention for noting that?
> 

In general, all member variables in our code are supposed to only be access via
one thread.  I think it's okay as is.

http://codereview.chromium.org/3305001/diff/22001/23036
File remoting/host/capturer.cc (right):

http://codereview.chromium.org/3305001/diff/22001/23036#newcode48
remoting/host/capturer.cc:48: TraceContext::tracer()->PrintString("Started
CalculateInvalidRects");
On 2010/09/28 19:06:21, garykac wrote:
> We'll probably need to remove these trace statements at some point because the
> string data will make the binary larger than it needs to be.

Yes, completely agree.  Filed a bug.

http://crbug.com/57373