Add in a new FrameConsumer interface, Decode API, and a RectangleUpdateDecoder abstraction.

Add in a new FrameConsumer interface, Decode API, and a RectangleUpdateDecoder abstraction.

This should allow a decoder that can still request the view to allocate frames without being owned by the view itself.  This allows for cleaner threading semantics and reduced coupling of classes.  The new decoder API keeps the decoder from being aware of the network packet types tightening up the API layering.


BUG=52833
TEST=None. This code isn't used yet.

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

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

[awong, 2010-09-08 19:41:38]

next fragment of the big update.  The new decoder interface...

[awong, 2010-09-08 19:43:29]

Uh oh...this doesn't compile at all.  I'll ping you again when it's ready.

-Albert


On 2010/09/08 19:41:38, awong wrote:
> next fragment of the big update.  The new decoder interface...

[awong, 2010-09-24 02:57:07]

Finally ready for review (2 weeks later...stupid non-dev work...)

[garykac, 2010-09-24 23:07:37]

LGTM. Mostly nits.

I assume that some of the TODOs (like dropped-packet recovery) are in your
upcoming CL?

http://codereview.chromium.org/3335012/diff/10001/11001
File remoting/base/decoder.h (right):

http://codereview.chromium.org/3335012/diff/10001/11001#newcode95
remoting/base/decoder.h:95: // protocol like VP8.  However, it breaks the layer
violation of depending on
"breaks the layer violation"?

http://codereview.chromium.org/3335012/diff/10001/11001#newcode99
remoting/base/decoder.h:99: // Initizes the decoder to draw into the given
|frame|.  The |clip| specifies
Initizes?

http://codereview.chromium.org/3335012/diff/10001/11001#newcode101
remoting/base/decoder.h:101: // region must fit inside the dimensions of frame.
Failure to due so will
"to do so"

http://codereview.chromium.org/3335012/diff/10001/11001#newcode103
remoting/base/decoder.h:103: // overflow).
I'm assuming that we actively want to prevent any possibility of a buffer
overflow. Can we guarantee that it will (at worst) CHECK?

http://codereview.chromium.org/3335012/diff/10001/11001#newcode107
remoting/base/decoder.h:107: // Reset the decoder to an uninitialized state.
Released all references to
Release all ...

http://codereview.chromium.org/3335012/diff/10001/11001#newcode108
remoting/base/decoder.h:108: // the initialized |frame|.  Initialize() must be
called before the decoder
"Initialize() must be called before..." but an earlier comment says (in
reference to Initialize()): "Not all decoders will use this."

http://codereview.chromium.org/3335012/diff/10001/11002
File remoting/client/frame_consumer.h (right):

http://codereview.chromium.org/3335012/diff/10001/11002#newcode21
remoting/client/frame_consumer.h:21: // may be bigger. If the knowing the actual
frame size is important, be sure
If knowing the...

http://codereview.chromium.org/3335012/diff/10001/11002#newcode37
remoting/client/frame_consumer.h:37: base::TimeDelta duration,
What does it mean for an allocated frame to have a 'duration'?

http://codereview.chromium.org/3335012/diff/10001/11003
File remoting/client/rectangle_update_decoder.cc (right):

http://codereview.chromium.org/3335012/diff/10001/11003#newcode116
remoting/client/rectangle_update_decoder.cc:116: if (packet.flags() |
RectangleUpdatePacket::FIRST_PACKET) {
According to .proto, the first packet should also have a sequence number of 0. 
And any packet with a non-0 sequence number is not a first packet.

http://codereview.chromium.org/3335012/diff/10001/11003#newcode124
remoting/client/rectangle_update_decoder.cc:124: format.encoding() ==
EncodingInvalid) {
This isn't the only invalid encoding. Suggest:
  encoding <= EncodingInvalid || encoding >= EncodingMax
where EncodingMax is added at the end of the enum in chromotocol.proto.

http://codereview.chromium.org/3335012/diff/10001/11003#newcode180
remoting/client/rectangle_update_decoder.cc:180: // stream input.  The the very
least, don't crash the process.
...may feed us a corrupt stream or manually create a stream that changes
decoders. At the very least, ...

http://codereview.chromium.org/3335012/diff/10001/11003#newcode182
remoting/client/rectangle_update_decoder.cc:182: // For not though, assume that
only one encoding is used throughout.
for now,

http://codereview.chromium.org/3335012/diff/10001/11003#newcode185
remoting/client/rectangle_update_decoder.cc:185: // However, we need to verify
that flushing semantics of the decoder first.
verify the

http://codereview.chromium.org/3335012/diff/10001/11003#newcode195
remoting/client/rectangle_update_decoder.cc:195: }
else {Trace or Error invalid encoding}

http://codereview.chromium.org/3335012/diff/10001/11003#newcode199
remoting/client/rectangle_update_decoder.cc:199: // out the right behavior and
make this more reslient.
resilient

http://codereview.chromium.org/3335012/diff/10001/11004
File remoting/client/rectangle_update_decoder.h (right):

http://codereview.chromium.org/3335012/diff/10001/11004#newcode30
remoting/client/rectangle_update_decoder.h:30: // regsitered as data is
avaialable. DecodePacket may keep a reference to
should this be "on the registered FrameConsumer as data is available"?

http://codereview.chromium.org/3335012/diff/10001/11004#newcode45
remoting/client/rectangle_update_decoder.h:45: MessageLoop* message_loop_;
Do we have a convention for showing ownership of ptrs like this?

[awong, 2010-09-27 20:36:12]

Thanks for the review! Fixed comments.

http://codereview.chromium.org/3335012/diff/10001/11001
File remoting/base/decoder.h (right):

http://codereview.chromium.org/3335012/diff/10001/11001#newcode95
remoting/base/decoder.h:95: // protocol like VP8.  However, it breaks the layer
violation of depending on
On 2010/09/24 23:07:41, garykac wrote:
> "breaks the layer violation"?

Done.

http://codereview.chromium.org/3335012/diff/10001/11001#newcode99
remoting/base/decoder.h:99: // Initizes the decoder to draw into the given
|frame|.  The |clip| specifies
On 2010/09/24 23:07:41, garykac wrote:
> Initizes?

Done.

http://codereview.chromium.org/3335012/diff/10001/11001#newcode101
remoting/base/decoder.h:101: // region must fit inside the dimensions of frame.
Failure to due so will
On 2010/09/24 23:07:41, garykac wrote:
> "to do so"

Done.

http://codereview.chromium.org/3335012/diff/10001/11001#newcode103
remoting/base/decoder.h:103: // overflow).
On 2010/09/24 23:07:41, garykac wrote:
> I'm assuming that we actively want to prevent any possibility of a buffer
> overflow. Can we guarantee that it will (at worst) CHECK?

Done.

http://codereview.chromium.org/3335012/diff/10001/11001#newcode107
remoting/base/decoder.h:107: // Reset the decoder to an uninitialized state.
Released all references to
On 2010/09/24 23:07:41, garykac wrote:
> Release all ...

Done.

http://codereview.chromium.org/3335012/diff/10001/11001#newcode108
remoting/base/decoder.h:108: // the initialized |frame|.  Initialize() must be
called before the decoder
On 2010/09/24 23:07:41, garykac wrote:
> "Initialize() must be called before..." but an earlier comment says (in
> reference to Initialize()): "Not all decoders will use this."

Stale comment in Initialize(). removed.

http://codereview.chromium.org/3335012/diff/10001/11002
File remoting/client/frame_consumer.h (right):

http://codereview.chromium.org/3335012/diff/10001/11002#newcode21
remoting/client/frame_consumer.h:21: // may be bigger. If the knowing the actual
frame size is important, be sure
On 2010/09/24 23:07:41, garykac wrote:
> If knowing the...

Reworded.

http://codereview.chromium.org/3335012/diff/10001/11002#newcode37
remoting/client/frame_consumer.h:37: base::TimeDelta duration,
On 2010/09/24 23:07:41, garykac wrote:
> What does it mean for an allocated frame to have a 'duration'?

This is for video codecs.  It's how long the frame should display for since some
codecs (including I'm betting ours eventually) can display display for a long
period of time. Think still frame.

http://codereview.chromium.org/3335012/diff/10001/11003
File remoting/client/rectangle_update_decoder.cc (right):

http://codereview.chromium.org/3335012/diff/10001/11003#newcode116
remoting/client/rectangle_update_decoder.cc:116: if (packet.flags() |
RectangleUpdatePacket::FIRST_PACKET) {
On 2010/09/24 23:07:41, garykac wrote:
> According to .proto, the first packet should also have a sequence number of 0.

> And any packet with a non-0 sequence number is not a first packet. 

I don't like this requirement...I'm going to modify the protocol buffer
documentation.

http://codereview.chromium.org/3335012/diff/10001/11003#newcode124
remoting/client/rectangle_update_decoder.cc:124: format.encoding() ==
EncodingInvalid) {
On 2010/09/24 23:07:41, garykac wrote:
> This isn't the only invalid encoding. Suggest:
>   encoding <= EncodingInvalid || encoding >= EncodingMax
> where EncodingMax is added at the end of the enum in chromotocol.proto.
> 

I think that should be caught by packet.IsValid(), which I just added.

http://codereview.chromium.org/3335012/diff/10001/11003#newcode180
remoting/client/rectangle_update_decoder.cc:180: // stream input.  The the very
least, don't crash the process.
On 2010/09/24 23:07:41, garykac wrote:
> ...may feed us a corrupt stream or manually create a stream that changes
> decoders. At the very least, ...

Done.

http://codereview.chromium.org/3335012/diff/10001/11003#newcode182
remoting/client/rectangle_update_decoder.cc:182: // For not though, assume that
only one encoding is used throughout.
On 2010/09/24 23:07:41, garykac wrote:
> for now,

Done.

http://codereview.chromium.org/3335012/diff/10001/11003#newcode185
remoting/client/rectangle_update_decoder.cc:185: // However, we need to verify
that flushing semantics of the decoder first.
On 2010/09/24 23:07:41, garykac wrote:
> verify the

Done.

http://codereview.chromium.org/3335012/diff/10001/11003#newcode195
remoting/client/rectangle_update_decoder.cc:195: }
On 2010/09/24 23:07:41, garykac wrote:
> else {Trace or Error invalid encoding}

Done.

http://codereview.chromium.org/3335012/diff/10001/11003#newcode199
remoting/client/rectangle_update_decoder.cc:199: // out the right behavior and
make this more reslient.
On 2010/09/24 23:07:41, garykac wrote:
> resilient

Done.

http://codereview.chromium.org/3335012/diff/10001/11004
File remoting/client/rectangle_update_decoder.h (right):

http://codereview.chromium.org/3335012/diff/10001/11004#newcode30
remoting/client/rectangle_update_decoder.h:30: // regsitered as data is
avaialable. DecodePacket may keep a reference to
On 2010/09/24 23:07:41, garykac wrote:
> should this be "on the registered FrameConsumer as data is available"?

Done.

http://codereview.chromium.org/3335012/diff/10001/11004#newcode45
remoting/client/rectangle_update_decoder.h:45: MessageLoop* message_loop_;
On 2010/09/24 23:07:41, garykac wrote:
> Do we have a convention for showing ownership of ptrs like this?

Done.