Add VideoPacket struct for video packets. Refactor Decode interface to use it.

Add VideoPacket struct for video packets. Refactor Decode interface to use it.
Various cleanups.
BUG=None
TEST=Unittests.

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

[Alpha Left Google, 2010-11-04 23:32:05]

First pass of review.

This change is too big.. :(
It involves not just the new declaration of VideoPacket but also
chromotingconfig and switching to use VideoPacket and elimination of
ChromotingHostMessage.. Should think about using git branches to break up into
multiple smaller patches next time.

Overall I'm still confused which channel we get the video packet message? Do we
use both RTP packets and VideoPacketMessage on the video channel (which is UDP
based)?

http://codereview.chromium.org/4476003/diff/5001/6003
File remoting/base/decoder.h (right):

http://codereview.chromium.org/4476003/diff/5001/6003#newcode26
remoting/base/decoder.h:26: // DecodeResult is returned from DecodePacket() and
indicates current state of
instead of DONE it should update available since depends on the context, it can
either mean the frame is decoded or a certain region is updated and can be
displayed.

http://codereview.chromium.org/4476003/diff/5001/6003#newcode46
remoting/base/decoder.h:46: virtual DecodeResult DecodePacket(const VideoPacket*
packet) = 0;
Having a synchronous API for decoding is probably easy to get things done,
though we need to reconsider this API when we do hardware decoding.. :(

However this is definitely a good start.

http://codereview.chromium.org/4476003/diff/5001/6003#newcode46
remoting/base/decoder.h:46: virtual DecodeResult DecodePacket(const VideoPacket*
packet) = 0;
why not const ref?

http://codereview.chromium.org/4476003/diff/5001/6003#newcode51
remoting/base/decoder.h:51: virtual UpdatedRects* updated_rects() = 0;
const UpdateRects&?

http://codereview.chromium.org/4476003/diff/5001/6013
File remoting/base/util.h (right):

http://codereview.chromium.org/4476003/diff/5001/6013#newcode12
remoting/base/util.h:12: int GetBytesPerPixel(media::VideoFrame::Format format);
media::VideoFrame should has this method now so don't need this, if not should
move this there.

http://codereview.chromium.org/4476003/diff/5001/6014
File remoting/base/video_packet.cc (right):

http://codereview.chromium.org/4476003/diff/5001/6014#newcode11
remoting/base/video_packet.cc:11: VideoPacket::VideoPacket()
if this is a struct don't need to separate the ctor and dtor in .cc file.

http://codereview.chromium.org/4476003/diff/5001/6015
File remoting/base/video_packet.h (right):

http://codereview.chromium.org/4476003/diff/5001/6015#newcode20
remoting/base/video_packet.h:20: FIRST_PACKET = 1,
kPacketXXX

http://codereview.chromium.org/4476003/diff/5001/6015#newcode25
remoting/base/video_packet.h:25: ENCODING_UNDEFINED,
kEncodingXXX

http://codereview.chromium.org/4476003/diff/5001/6015#newcode44
remoting/base/video_packet.h:44: // |data| holds the data buffer for this
packet.
|buffer|

http://codereview.chromium.org/4476003/diff/5001/6015#newcode47
remoting/base/video_packet.h:47: // |data_start| points to the beginning of the
payload in |data|.
|payload| and |buffer|

http://codereview.chromium.org/4476003/diff/5001/6029
File remoting/host/session_manager.cc (left):

http://codereview.chromium.org/4476003/diff/5001/6029#oldcode257
remoting/host/session_manager.cc:257: network_loop_->PostTask(
don't remove this code, we need to use it later.

http://codereview.chromium.org/4476003/diff/5001/6029#oldcode350
remoting/host/session_manager.cc:350: void
SessionManager::DoSendInit(scoped_refptr<ConnectionToClient> connection,
keep this method around but leave it empty.

http://codereview.chromium.org/4476003/diff/5001/6031
File remoting/proto/internal.proto (left):

http://codereview.chromium.org/4476003/diff/5001/6031#oldcode19
remoting/proto/internal.proto:19: message ChromotingHostMessage {
very cool!

http://codereview.chromium.org/4476003/diff/5001/6038
File remoting/protocol/message_decoder_unittest.cc (right):

http://codereview.chromium.org/4476003/diff/5001/6038#newcode86
remoting/protocol/message_decoder_unittest.cc:86: EXPECT_EQ((index % 2) != 0,
message->key_event().pressed());
instead of using index to verify the results should a the results to a list of
samples so we can compare.

http://codereview.chromium.org/4476003/diff/5001/6047
File remoting/protocol/video_stub.h (right):

http://codereview.chromium.org/4476003/diff/5001/6047#newcode8
remoting/protocol/video_stub.h:8: #include "remoting/base/video_packet.h"
should forward declare this,

http://codereview.chromium.org/4476003/diff/5001/6047#newcode22
remoting/protocol/video_stub.h:22: virtual void ProcessVideoPacket(const
VideoPacket* video_packet,
why is this not const &? other stub takes const ref too.

[Sergey Ulanov, 2010-11-05 00:47:11]

> This change is too big.. :(
> It involves not just the new declaration of VideoPacket but also
> chromotingconfig and switching to use VideoPacket and elimination of
> ChromotingHostMessage.. Should think about using git branches to break up into
> multiple smaller patches next time.
Yes, I agree that it is rather big, but I would have two days on it instead of
one, if I had to split it into multiple independent pieces.

> Overall I'm still confused which channel we get the video packet message? Do
we
> use both RTP packets and VideoPacketMessage on the video channel (which is UDP
> based)?
ProtobufVideoReader and ProtobufVideoWriter read/write from/to the video channel
(PseudoTCP).
RtpVideoReader and RtcpVideoWriter read/write from/to the video-rtp and
video-rtcp channels (UDP).
Whether Protobud or RTP is used is determined by the negotiated config
(currently it is Protobuf by default).

http://codereview.chromium.org/4476003/diff/5001/6003
File remoting/base/decoder.h (right):

http://codereview.chromium.org/4476003/diff/5001/6003#newcode26
remoting/base/decoder.h:26: // DecodeResult is returned from DecodePacket() and
indicates current state of
On 2010/11/04 23:32:05, Alpha wrote:
> instead of DONE it should update available since depends on the context, it
can
> either mean the frame is decoded or a certain region is updated and can be
> displayed.
How about DECODE_FRAME_DONE?

http://codereview.chromium.org/4476003/diff/5001/6003#newcode51
remoting/base/decoder.h:51: virtual UpdatedRects* updated_rects() = 0;
On 2010/11/04 23:32:05, Alpha wrote:
> const UpdateRects&?
No. This methods gives ownership to the caller.
The reason is that the caller may want to use returned UpdatedRects later, and
decoder should be able to continue processing the next frame without waiting for
the previous one to be displayed.

http://codereview.chromium.org/4476003/diff/5001/6013
File remoting/base/util.h (right):

http://codereview.chromium.org/4476003/diff/5001/6013#newcode12
remoting/base/util.h:12: int GetBytesPerPixel(media::VideoFrame::Format format);
On 2010/11/04 23:32:05, Alpha wrote:
> media::VideoFrame should has this method now so don't need this, if not should
> move this there.
Added TODO.

http://codereview.chromium.org/4476003/diff/5001/6014
File remoting/base/video_packet.cc (right):

http://codereview.chromium.org/4476003/diff/5001/6014#newcode11
remoting/base/video_packet.cc:11: VideoPacket::VideoPacket()
On 2010/11/04 23:32:05, Alpha wrote:
> if this is a struct don't need to separate the ctor and dtor in .cc file.
There is no rule that says that constructor and destructor need to be inlined. I
think it makes sense to put them in .cc. We may need to add more code in this
file later.

http://codereview.chromium.org/4476003/diff/5001/6015
File remoting/base/video_packet.h (right):

http://codereview.chromium.org/4476003/diff/5001/6015#newcode20
remoting/base/video_packet.h:20: FIRST_PACKET = 1,
On 2010/11/04 23:32:05, Alpha wrote:
> kPacketXXX
http://www.chromium.org/developers/coding-style: 
Though the style guide says to use kConstantNaming for enums now, we still use
MACRO_STYLE naming for enums for consistency with existing code.

http://codereview.chromium.org/4476003/diff/5001/6015#newcode25
remoting/base/video_packet.h:25: ENCODING_UNDEFINED,
On 2010/11/04 23:32:05, Alpha wrote:
> kEncodingXXX
see above.

http://codereview.chromium.org/4476003/diff/5001/6015#newcode44
remoting/base/video_packet.h:44: // |data| holds the data buffer for this
packet.
On 2010/11/04 23:32:05, Alpha wrote:
> |buffer|

Done.

http://codereview.chromium.org/4476003/diff/5001/6015#newcode47
remoting/base/video_packet.h:47: // |data_start| points to the beginning of the
payload in |data|.
On 2010/11/04 23:32:05, Alpha wrote:
> |payload| and |buffer|

Done.

http://codereview.chromium.org/4476003/diff/5001/6029
File remoting/host/session_manager.cc (left):

http://codereview.chromium.org/4476003/diff/5001/6029#oldcode257
remoting/host/session_manager.cc:257: network_loop_->PostTask(
On 2010/11/04 23:32:05, Alpha wrote:
> don't remove this code, we need to use it later.
When will we need it? We don't need the InitClient message anymore. We can
always add it later if we need it. There is a lot of refactoring needed in this
code anyway, so I think it is better to keep it clean. I'll put it back if there
is a reason we will need it later.

http://codereview.chromium.org/4476003/diff/5001/6029#oldcode350
remoting/host/session_manager.cc:350: void
SessionManager::DoSendInit(scoped_refptr<ConnectionToClient> connection,
On 2010/11/04 23:32:05, Alpha wrote:
> keep this method around but leave it empty.
screen resolution is sent as part of session description. Don't see the reason
why we would need to keep this method around.

http://codereview.chromium.org/4476003/diff/5001/6038
File remoting/protocol/message_decoder_unittest.cc (right):

http://codereview.chromium.org/4476003/diff/5001/6038#newcode86
remoting/protocol/message_decoder_unittest.cc:86: EXPECT_EQ((index % 2) != 0,
message->key_event().pressed());
On 2010/11/04 23:32:05, Alpha wrote:
> instead of using index to verify the results should a the results to a list of
> samples so we can compare.
I agree, but this code will have to be changed again when we get rid of
ChromotingClientMessage, so I just added TODO.

http://codereview.chromium.org/4476003/diff/5001/6047
File remoting/protocol/video_stub.h (right):

http://codereview.chromium.org/4476003/diff/5001/6047#newcode8
remoting/protocol/video_stub.h:8: #include "remoting/base/video_packet.h"
On 2010/11/04 23:32:05, Alpha wrote:
> should forward declare this,

Done.

http://codereview.chromium.org/4476003/diff/5001/6047#newcode22
remoting/protocol/video_stub.h:22: virtual void ProcessVideoPacket(const
VideoPacket* video_packet,
On 2010/11/04 23:32:05, Alpha wrote:
> why is this not const &? other stub takes const ref too.
This function may use |video_packet| after it returns. If you pass reference
that it means that the caller is free to delete this packet after the function
returns.

[Sergey Ulanov, 2010-11-05 00:50:54]

On Thu, Nov 4, 2010 at 5:47 PM, <sergeyu@chromium.org> wrote:

>  This change is too big.. :(
>> It involves not just the new declaration of VideoPacket but also
>> chromotingconfig and switching to use VideoPacket and elimination of
>> ChromotingHostMessage.. Should think about using git branches to break up
>> into
>> multiple smaller patches next time.
>>
> Yes, I agree that it is rather big, but I would have two days on it instead
> of
> one, if I had to split it into multiple independent pieces.


I meant "I would have to spend two days on it instead of one".

[awong, 2010-11-05 01:31:53]

High level comments:

1) I don't think we're in sync on the motivation of separating VideoPacket from
VideoPacketMessage.  Right now, it's a dumb copy, which is something to be
avoided if possible.  We should has this out in person.

2) About the code review size, I agree with Alpha on the size issue here.  I'm
guilty of submitting overly long CLs too, but the last time I did this, I broke
a whole bunch of things that you and Gary have been spending days cleaning up. 
The problem is that it's not really possible to review a CL thoroughly if it's >
~500 lines of actual code.  The time cost should be framed against making the
reviewer unable to do a good job, which is a pretty high risk...

Having said that, this one didn't actually feel too onerous on me, but it did
have 3 different concepts flying through there (VideoPacket, refactor the
interface, and remove the old message) which would have been easier to keep
straight if they were separated.

http://codereview.chromium.org/4476003/diff/5001/6047
File remoting/protocol/video_stub.h (right):

http://codereview.chromium.org/4476003/diff/5001/6047#newcode22
remoting/protocol/video_stub.h:22: virtual void ProcessVideoPacket(const
VideoPacket* video_packet,
On 2010/11/05 00:47:11, sergeyu wrote:
> On 2010/11/04 23:32:05, Alpha wrote:
> > why is this not const &? other stub takes const ref too.
> This function may use |video_packet| after it returns. If you pass reference
> that it means that the caller is free to delete this packet after the function
> returns.

We should be consistent across the whole API on this.  So either change this to
a reference, or the other to a pointers.

I agree with Sergey, but don't feel super strongly about it. Do you care Alpha? 
If not, I can submit a CL to flip the convention to pointers.

http://codereview.chromium.org/4476003/diff/15002/17004
File remoting/base/decoder_row_based.cc (right):

http://codereview.chromium.org/4476003/diff/15002/17004#newcode159
remoting/base/decoder_row_based.cc:159: UpdatedRects*
DecoderRowBased::updated_rects() {
Stylistically, we shouldn't be returning a pointer that the caller needs to own
when you call an "underscore" based function.

Maybe make this take the update rects as an input parameter that you append to?

http://codereview.chromium.org/4476003/diff/22001/23015
File remoting/base/video_packet.h (right):

http://codereview.chromium.org/4476003/diff/22001/23015#newcode17
remoting/base/video_packet.h:17: struct VideoPacket {
I'm not entirely sure I understand why we are using this struct.

Originally, I was expecting that our "VideoPacket" was going to be a collection
of protobufs.

As such, it doesn't seem to have much value beyond the actual video packet.  Am
I missing something?

http://codereview.chromium.org/4476003/diff/22001/23015#newcode18
remoting/base/video_packet.h:18: // These values must be the same as in
remoting/proto/video.proto.
Why don't we just use the values from that proto?

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

http://codereview.chromium.org/4476003/diff/22001/23020#newcode23
remoting/host/capturer.cc:23: 
remove extra space.

http://codereview.chromium.org/4476003/diff/22001/23020#newcode26
remoting/host/capturer.cc:26: int Capturer::width() const { return width_; }
All our code in the .cc files put the braces in separte lines even for simple
functions.  Do this here and for height() as well?

http://codereview.chromium.org/4476003/diff/22001/23029
File remoting/proto/video.proto (right):

http://codereview.chromium.org/4476003/diff/22001/23029#newcode15
remoting/proto/video.proto:15: required int32 x = 1;
Let's make these things all optional, and validate them by hand.

Yes it's more code, but since we're dealing with networks, and the protobuf
code, IIRC, just aborts if we don't get a required field isn't to us.  We want
to be the ones in control of aborting. :)

[Sergey Ulanov, 2010-11-06 01:59:39]

Ok, I removed the new VideoPacket struct from this change now. Will add it
later. What is left is Decoder interface refactoring and some cleanups.

[awong, 2010-11-09 01:54:38]

Thanks for migrating back to the proto!

I've got a couple more comments.

http://codereview.chromium.org/4476003/diff/44001/45003
File remoting/base/decoder.h (right):

http://codereview.chromium.org/4476003/diff/44001/45003#newcode51
remoting/base/decoder.h:51: virtual UpdatedRects* GetUpdatedRects() = 0;
I would prefer that this appends to an UpdateRects* that you pass in.

http://codereview.chromium.org/4476003/diff/44001/45004
File remoting/base/decoder_row_based.cc (right):

http://codereview.chromium.org/4476003/diff/44001/45004#newcode52
remoting/base/decoder_row_based.cc:52: return state_ != kUninitialized && state_
!= kError;
Positive filters are usually a little more robust against adding states to the
the enum in the future.  Switch to that?

http://codereview.chromium.org/4476003/diff/44001/45004#newcode75
remoting/base/decoder_row_based.cc:75: if (packet->flags() &
VideoPacket::FIRST_PACKET) {
Can we pull the state transition logic out into a DetermineDecoderState()
function or something?

http://codereview.chromium.org/4476003/diff/44001/45015
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/4476003/diff/44001/45015#newcode75
remoting/client/chromoting_client.h:75: void Initialize();
Minor annoyance unrelated to this CL, but we should settle on Init versus
Initialize.  I think Initialize() is winning.

ajwong@ajwong:~/src/git-chrome/src$ find remoting/ -name '*.h' -exec grep -H
Init\(\) {} \; | grep -v ': *//'  | wc -l
2
ajwong@ajwong:~/src/git-chrome/src$ find remoting/ -name '*.h' -exec grep -H
Initialize\(\) {} \; | grep -v ': *//'  | wc -l
6

[Sergey Ulanov, 2010-11-09 21:34:42]

Addressed all comments.

http://codereview.chromium.org/4476003/diff/44001/remoting/base/decoder.h
File remoting/base/decoder.h (right):

http://codereview.chromium.org/4476003/diff/44001/remoting/base/decoder.h#new...
remoting/base/decoder.h:51: virtual UpdatedRects* GetUpdatedRects() = 0;
On 2010/11/09 01:54:38, awong wrote:
> I would prefer that this appends to an UpdateRects* that you pass in.

Done.

http://codereview.chromium.org/4476003/diff/44001/remoting/base/decoder_row_b...
File remoting/base/decoder_row_based.cc (right):

http://codereview.chromium.org/4476003/diff/44001/remoting/base/decoder_row_b...
remoting/base/decoder_row_based.cc:52: return state_ != kUninitialized && state_
!= kError;
On 2010/11/09 01:54:38, awong wrote:
> Positive filters are usually a little more robust against adding states to the
> the enum in the future.  Switch to that?

Done.

http://codereview.chromium.org/4476003/diff/44001/remoting/base/decoder_row_b...
remoting/base/decoder_row_based.cc:75: if (packet->flags() &
VideoPacket::FIRST_PACKET) {
On 2010/11/09 01:54:38, awong wrote:
> Can we pull the state transition logic out into a DetermineDecoderState()
> function or something?

Done.

http://codereview.chromium.org/4476003/diff/44001/remoting/client/chromoting_...
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/4476003/diff/44001/remoting/client/chromoting_...
remoting/client/chromoting_client.h:75: void Initialize();
On 2010/11/09 01:54:38, awong wrote:
> Minor annoyance unrelated to this CL, but we should settle on Init versus
> Initialize.  I think Initialize() is winning.
> 
> ajwong@ajwong:~/src/git-chrome/src$ find remoting/ -name '*.h' -exec grep -H
> Init\(\) {} \; | grep -v ': *//'  | wc -l
> 2
> ajwong@ajwong:~/src/git-chrome/src$ find remoting/ -name '*.h' -exec grep -H
> Initialize\(\) {} \; | grep -v ': *//'  | wc -l
> 6
> 
Agree

[awong, 2010-11-09 21:39:30]

LGTM