Add VideoReader and VideoWriter interfaces.

Add VideoReader and VideoWriter interfaces.

Implemented VideoReader and VideoWriter for RTP and Protobuf.

BUG=53986
TEST=None

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

[awong, 2010-11-02 17:59:31]

overall looks solid.  Couple of quesitons.

Also, we want to move everything into the protocol namespace.  Is that happening
on another pass?

http://codereview.chromium.org/4229003/diff/5001/6016
File remoting/protocol/chromotocol_config.h (right):

http://codereview.chromium.org/4229003/diff/5001/6016#newcode29
remoting/protocol/chromotocol_config.h:29: CODEC_ZIP,
alphabetical ordering.

http://codereview.chromium.org/4229003/diff/5001/6017
File remoting/protocol/message_decoder.h (right):

http://codereview.chromium.org/4229003/diff/5001/6017#newcode14
remoting/protocol/message_decoder.h:14: #include
"remoting/base/multiple_array_input_stream.h"
Does this include need to be here?

http://codereview.chromium.org/4229003/diff/5001/6018
File remoting/protocol/message_reader.h (right):

http://codereview.chromium.org/4229003/diff/5001/6018#newcode13
remoting/protocol/message_reader.h:13: #include "net/base/io_buffer.h"
You shouldn't need this I don't think.

http://codereview.chromium.org/4229003/diff/5001/6020
File remoting/protocol/protobuf_video_reader.h (right):

http://codereview.chromium.org/4229003/diff/5001/6020#newcode27
remoting/protocol/protobuf_video_reader.h:27: void OnNewData(VideoPacket*
packet);
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/4229003/diff/5001/6035
File remoting/protocol/video_reader.h (right):

http://codereview.chromium.org/4229003/diff/5001/6035#newcode16
remoting/protocol/video_reader.h:16: class VideoReader {
Is this interface parallel to the MessageReader?  Is the application level code
supposed to actually pass in the callback directly?  I was expecting to see a
consumption interface that can be handed to the connection layer classes
(ConnectionToHost and ConnecitonToCilent) that this would eventually feed into.

Can we get a file comment explaining who/what should be using this?

http://codereview.chromium.org/4229003/diff/5001/6035#newcode19
remoting/protocol/video_reader.h:19: // video packets in protobuf stream. It
should not be used here.
Briefly describe also what should be used instead.

[Sergey Ulanov, 2010-11-02 21:01:23]

http://codereview.chromium.org/4229003/diff/5001/6016
File remoting/protocol/chromotocol_config.h (right):

http://codereview.chromium.org/4229003/diff/5001/6016#newcode29
remoting/protocol/chromotocol_config.h:29: CODEC_ZIP,
On 2010/11/02 17:59:31, awong wrote:
> alphabetical ordering.

Is there requirement to put enum values in alphabetical order? Even the style
guide shows the following as a valid order:
   enum UrlTableErrors {
     kOK = 0,
     kErrorOutOfMemory,
     kErrorMalformedInput,
   };
In this particular case I ordered from the simplest to more complicated.

http://codereview.chromium.org/4229003/diff/5001/6017
File remoting/protocol/message_decoder.h (right):

http://codereview.chromium.org/4229003/diff/5001/6017#newcode14
remoting/protocol/message_decoder.h:14: #include
"remoting/base/multiple_array_input_stream.h"
On 2010/11/02 17:59:31, awong wrote:
> Does this include need to be here?

Yes, because the code below calls ParseFromZeroCopyStream(), and without this
include compiler doesn't know that MultipleArrayInputStream implements
google::protobuf::io::ZeroCopyInputStream.

http://codereview.chromium.org/4229003/diff/5001/6018
File remoting/protocol/message_reader.h (right):

http://codereview.chromium.org/4229003/diff/5001/6018#newcode13
remoting/protocol/message_reader.h:13: #include "net/base/io_buffer.h"
On 2010/11/02 17:59:31, awong wrote:
> You shouldn't need this I don't think.
The code below converts net::IOBuffer* to scoped_refptr<net::IOBuffer>
(implicitly) when calling message_decoder_.ParseMessages(). This doesn't work
without this include.

http://codereview.chromium.org/4229003/diff/5001/6020
File remoting/protocol/protobuf_video_reader.h (right):

http://codereview.chromium.org/4229003/diff/5001/6020#newcode27
remoting/protocol/protobuf_video_reader.h:27: void OnNewData(VideoPacket*
packet);
On 2010/11/02 17:59:31, awong wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/4229003/diff/5001/6035
File remoting/protocol/video_reader.h (right):

http://codereview.chromium.org/4229003/diff/5001/6035#newcode16
remoting/protocol/video_reader.h:16: class VideoReader {
On 2010/11/02 17:59:31, awong wrote:
> Is this interface parallel to the MessageReader?  Is the application level
code
> supposed to actually pass in the callback directly?  I was expecting to see a
> consumption interface that can be handed to the connection layer classes
> (ConnectionToHost and ConnecitonToCilent) that this would eventually feed
into.
> 
> Can we get a file comment explaining who/what should be using this?

Ok, I've added VideoStub interface, and it is used now on the client side
instead of the callback. Not sure if we need to use it on the host side too.

Added comments here and in video_reader.h explaining what this class is for.

http://codereview.chromium.org/4229003/diff/5001/6035#newcode19
remoting/protocol/video_reader.h:19: // video packets in protobuf stream. It
should not be used here.
On 2010/11/02 17:59:31, awong wrote:
> Briefly describe also what should be used instead.

Done.

[Sergey Ulanov, 2010-11-02 21:43:07]

On 2010/11/02 17:59:31, awong wrote:
> overall looks solid.  Couple of quesitons.
> 
> Also, we want to move everything into the protocol namespace.  Is that
happening
> on another pass?
Yes, I will move everything to the protocol namespace in a different change.

[awong, 2010-11-03 00:14:35]

LGTM w/nits

I'm assuming that the VideoWriter being exposed in ClientConnection is just a
termporary thing, right?

http://codereview.chromium.org/4229003/diff/5001/6016
File remoting/protocol/chromotocol_config.h (right):

http://codereview.chromium.org/4229003/diff/5001/6016#newcode29
remoting/protocol/chromotocol_config.h:29: CODEC_ZIP,
On 2010/11/02 21:01:23, sergeyu wrote:
> On 2010/11/02 17:59:31, awong wrote:
> > alphabetical ordering.
> 
> Is there requirement to put enum values in alphabetical order? Even the style
> guide shows the following as a valid order:
>    enum UrlTableErrors {
>      kOK = 0,
>      kErrorOutOfMemory,
>      kErrorMalformedInput,
>    };
> In this particular case I ordered from the simplest to more complicated.


No there is no requirement.  I'm fine leaving as is.  Add a comment about the
complexity ordering so no one tries to alphabetize it later.

http://codereview.chromium.org/4229003/diff/28002/48005
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/4229003/diff/28002/48005#newcode29
remoting/client/chromoting_client.h:29: public VideoStub {
VideoStub should be implemented by another class. Add a TODO?

http://codereview.chromium.org/4229003/diff/28002/48012
File remoting/host/client_connection.h (right):

http://codereview.chromium.org/4229003/diff/28002/48012#newcode106
remoting/host/client_connection.h:106: scoped_ptr<VideoWriter> video_writer_;
I was expecting that this would just be the VideoStub.

Is this just a TODO?

http://codereview.chromium.org/4229003/diff/28002/48019
File remoting/protocol/protobuf_video_reader.cc (right):

http://codereview.chromium.org/4229003/diff/28002/48019#newcode28
remoting/protocol/protobuf_video_reader.cc:28: packet,
NewRunnableFunction(&ProtobufVideoReader::DeletePacket, packet));
how about just newing a "DeleteTask" from base/task.h?

http://codereview.chromium.org/4229003/diff/28002/48031
File remoting/protocol/rtp_writer.h (right):

http://codereview.chromium.org/4229003/diff/28002/48031#newcode18
remoting/protocol/rtp_writer.h:18: // Initializes the writer. Must be called on
the thread the sockets belong
sockets belong -> socket belongs

http://codereview.chromium.org/4229003/diff/28002/48031#newcode22
remoting/protocol/rtp_writer.h:22: void SendPacket(const char* buffer, int
packet_size, uint32 timestamp);
Comment explaining what buffer should contain?

http://codereview.chromium.org/4229003/diff/28002/48031#newcode24
remoting/protocol/rtp_writer.h:24: int GetPendingMessages();
comment explaning what this returns?

http://codereview.chromium.org/4229003/diff/28002/48035
File remoting/protocol/video_reader.h (right):

http://codereview.chromium.org/4229003/diff/28002/48035#newcode24
remoting/protocol/video_reader.h:24: virtual ~VideoReader() { }
Since you already have a .cc file, might as well not inline the
consturctor/destructor...

http://codereview.chromium.org/4229003/diff/28002/48035#newcode26
remoting/protocol/video_reader.h:26: // Initializies the reader. Doesn't take
ownershipt of neither |connection|
ownershipt -> ownership
neither -> either
nor -> or

double-negatives are fun.

http://codereview.chromium.org/4229003/diff/28002/48035#newcode31
remoting/protocol/video_reader.h:31: // Closes the reader. The stub is called
after Close().
The stub should not be called right?

http://codereview.chromium.org/4229003/diff/28002/48038
File remoting/protocol/video_writer.h (right):

http://codereview.chromium.org/4229003/diff/28002/48038#newcode31
remoting/protocol/video_writer.h:31: virtual void SendPacket(const VideoPacket&
packet) = 0;
Eventually this is going to be something other than packet, right?

[Sergey Ulanov, 2010-11-03 00:50:56]

http://codereview.chromium.org/4229003/diff/5001/6016
File remoting/protocol/chromotocol_config.h (right):

http://codereview.chromium.org/4229003/diff/5001/6016#newcode29
remoting/protocol/chromotocol_config.h:29: CODEC_ZIP,
On 2010/11/03 00:14:36, awong wrote:
> On 2010/11/02 21:01:23, sergeyu wrote:
> > On 2010/11/02 17:59:31, awong wrote:
> > > alphabetical ordering.
> > 
> > Is there requirement to put enum values in alphabetical order? Even the
style
> > guide shows the following as a valid order:
> >    enum UrlTableErrors {
> >      kOK = 0,
> >      kErrorOutOfMemory,
> >      kErrorMalformedInput,
> >    };
> > In this particular case I ordered from the simplest to more complicated.
> 
> 
> No there is no requirement.  I'm fine leaving as is.  Add a comment about the
> complexity ordering so no one tries to alphabetize it later.

I don't think this is necessary. I've never seen comments about a reason for
particular order of enum values, don't think we need it here. And enum values
are almost never ordered alphabetically.
In this particular case there are no requirements for any particular order, I
just shuffled them here to match the order in video.proto.

http://codereview.chromium.org/4229003/diff/28002/48005
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/4229003/diff/28002/48005#newcode29
remoting/client/chromoting_client.h:29: public VideoStub {
On 2010/11/03 00:14:36, awong wrote:
> VideoStub should be implemented by another class. Add a TODO?

Done.

http://codereview.chromium.org/4229003/diff/28002/48012
File remoting/host/client_connection.h (right):

http://codereview.chromium.org/4229003/diff/28002/48012#newcode106
remoting/host/client_connection.h:106: scoped_ptr<VideoWriter> video_writer_;
On 2010/11/03 00:14:36, awong wrote:
> I was expecting that this would just be the VideoStub.
> 
> Is this just a TODO?

We need VideoWriter here. There are some things that VideoWriter doesn't that
are not part of VideoStub interface. For example when closing connection
video_writer_->Close() must be called, but Close() is not in the VideoStub
interface.

http://codereview.chromium.org/4229003/diff/28002/48019
File remoting/protocol/protobuf_video_reader.cc (right):

http://codereview.chromium.org/4229003/diff/28002/48019#newcode28
remoting/protocol/protobuf_video_reader.cc:28: packet,
NewRunnableFunction(&ProtobufVideoReader::DeletePacket, packet));
On 2010/11/03 00:14:36, awong wrote:
> how about just newing a "DeleteTask" from base/task.h?

Done.

http://codereview.chromium.org/4229003/diff/28002/48031
File remoting/protocol/rtp_writer.h (right):

http://codereview.chromium.org/4229003/diff/28002/48031#newcode18
remoting/protocol/rtp_writer.h:18: // Initializes the writer. Must be called on
the thread the sockets belong
On 2010/11/03 00:14:36, awong wrote:
> sockets belong -> socket belongs
There are two sockets: one for rtp, another one for rtcp.

http://codereview.chromium.org/4229003/diff/28002/48031#newcode22
remoting/protocol/rtp_writer.h:22: void SendPacket(const char* buffer, int
packet_size, uint32 timestamp);
On 2010/11/03 00:14:36, awong wrote:
> Comment explaining what buffer should contain?

Renamed it to |payload|.

http://codereview.chromium.org/4229003/diff/28002/48031#newcode24
remoting/protocol/rtp_writer.h:24: int GetPendingMessages();
On 2010/11/03 00:14:36, awong wrote:
> comment explaning what this returns?

Done.

http://codereview.chromium.org/4229003/diff/28002/48035
File remoting/protocol/video_reader.h (right):

http://codereview.chromium.org/4229003/diff/28002/48035#newcode24
remoting/protocol/video_reader.h:24: virtual ~VideoReader() { }
On 2010/11/03 00:14:36, awong wrote:
> Since you already have a .cc file, might as well not inline the
> consturctor/destructor...

Done.

http://codereview.chromium.org/4229003/diff/28002/48035#newcode26
remoting/protocol/video_reader.h:26: // Initializies the reader. Doesn't take
ownershipt of neither |connection|
On 2010/11/03 00:14:36, awong wrote:
> ownershipt -> ownership
> neither -> either
> nor -> or
> 
> double-negatives are fun.

Done.

http://codereview.chromium.org/4229003/diff/28002/48035#newcode31
remoting/protocol/video_reader.h:31: // Closes the reader. The stub is called
after Close().
On 2010/11/03 00:14:36, awong wrote:
> The stub should not be called right?

Done.

http://codereview.chromium.org/4229003/diff/28002/48038
File remoting/protocol/video_writer.h (right):

http://codereview.chromium.org/4229003/diff/28002/48038#newcode31
remoting/protocol/video_writer.h:31: virtual void SendPacket(const VideoPacket&
packet) = 0;
On 2010/11/03 00:14:36, awong wrote:
> Eventually this is going to be something other than packet, right?
Right. There is TODO about it in VideoStub and TODO to implement VideoStub in
this class.

[awong, 2010-11-03 00:54:35]

On Tue, Nov 2, 2010 at 5:50 PM, <sergeyu@chromium.org> wrote:

>
> http://codereview.chromium.org/4229003/diff/5001/6016
> File remoting/protocol/chromotocol_config.h (right):
>
> http://codereview.chromium.org/4229003/diff/5001/6016#newcode29
> remoting/protocol/chromotocol_config.h:29: CODEC_ZIP,
> On 2010/11/03 00:14:36, awong wrote:
>
>> On 2010/11/02 21:01:23, sergeyu wrote:
>> > On 2010/11/02 17:59:31, awong wrote:
>> > > alphabetical ordering.
>> >
>> > Is there requirement to put enum values in alphabetical order? Even
>>
> the style
>
>> > guide shows the following as a valid order:
>> >    enum UrlTableErrors {
>> >      kOK = 0,
>> >      kErrorOutOfMemory,
>> >      kErrorMalformedInput,
>> >    };
>> > In this particular case I ordered from the simplest to more
>>
> complicated.
>
>
>  No there is no requirement.  I'm fine leaving as is.  Add a comment
>>
> about the
>
>> complexity ordering so no one tries to alphabetize it later.
>>
>
> I don't think this is necessary. I've never seen comments about a reason
> for particular order of enum values, don't think we need it here. And
> enum values are almost never ordered alphabetically.
> In this particular case there are no requirements for any particular
> order, I just shuffled them here to match the order in video.proto.
>
>
> http://codereview.chromium.org/4229003/diff/28002/48005
> File remoting/client/chromoting_client.h (right):
>
> http://codereview.chromium.org/4229003/diff/28002/48005#newcode29
> remoting/client/chromoting_client.h:29: public VideoStub {
> On 2010/11/03 00:14:36, awong wrote:
>
>> VideoStub should be implemented by another class. Add a TODO?
>>
>
> Done.
>
>
> http://codereview.chromium.org/4229003/diff/28002/48012
> File remoting/host/client_connection.h (right):
>
> http://codereview.chromium.org/4229003/diff/28002/48012#newcode106
> remoting/host/client_connection.h:106: scoped_ptr<VideoWriter>
> video_writer_;
> On 2010/11/03 00:14:36, awong wrote:
>
>> I was expecting that this would just be the VideoStub.
>>
>
>  Is this just a TODO?
>>
>
> We need VideoWriter here. There are some things that VideoWriter doesn't
> that are not part of VideoStub interface. For example when closing
> connection video_writer_->Close() must be called, but Close() is not in
> the VideoStub interface.


erp..I got confused about what ClientConnection was.   You're right.


>  http://codereview.chromium.org/4229003/diff/28002/48019
> File remoting/protocol/protobuf_video_reader.cc (right):
>
> http://codereview.chromium.org/4229003/diff/28002/48019#newcode28
> remoting/protocol/protobuf_video_reader.cc:28: packet,
> NewRunnableFunction(&ProtobufVideoReader::DeletePacket, packet));
> On 2010/11/03 00:14:36, awong wrote:
>
>> how about just newing a "DeleteTask" from base/task.h?
>>
>
> Done.
>
>
> http://codereview.chromium.org/4229003/diff/28002/48031
> File remoting/protocol/rtp_writer.h (right):
>
> http://codereview.chromium.org/4229003/diff/28002/48031#newcode18
> remoting/protocol/rtp_writer.h:18: // Initializes the writer. Must be
> called on the thread the sockets belong
> On 2010/11/03 00:14:36, awong wrote:
>
>> sockets belong -> socket belongs
>>
> There are two sockets: one for rtp, another one for rtcp.
>
>
> http://codereview.chromium.org/4229003/diff/28002/48031#newcode22
> remoting/protocol/rtp_writer.h:22: void SendPacket(const char* buffer,
> int packet_size, uint32 timestamp);
> On 2010/11/03 00:14:36, awong wrote:
>
>> Comment explaining what buffer should contain?
>>
>
> Renamed it to |payload|.
>
>
> http://codereview.chromium.org/4229003/diff/28002/48031#newcode24
> remoting/protocol/rtp_writer.h:24: int GetPendingMessages();
> On 2010/11/03 00:14:36, awong wrote:
>
>> comment explaning what this returns?
>>
>
> Done.
>
>
> http://codereview.chromium.org/4229003/diff/28002/48035
> File remoting/protocol/video_reader.h (right):
>
> http://codereview.chromium.org/4229003/diff/28002/48035#newcode24
> remoting/protocol/video_reader.h:24: virtual ~VideoReader() { }
> On 2010/11/03 00:14:36, awong wrote:
>
>> Since you already have a .cc file, might as well not inline the
>> consturctor/destructor...
>>
>
> Done.
>
>
> http://codereview.chromium.org/4229003/diff/28002/48035#newcode26
> remoting/protocol/video_reader.h:26: // Initializies the reader. Doesn't
> take ownershipt of neither |connection|
> On 2010/11/03 00:14:36, awong wrote:
>
>> ownershipt -> ownership
>> neither -> either
>> nor -> or
>>
>
>  double-negatives are fun.
>>
>
> Done.
>
>
> http://codereview.chromium.org/4229003/diff/28002/48035#newcode31
> remoting/protocol/video_reader.h:31: // Closes the reader. The stub is
> called after Close().
> On 2010/11/03 00:14:36, awong wrote:
>
>> The stub should not be called right?
>>
>
> Done.
>
>
> http://codereview.chromium.org/4229003/diff/28002/48038
> File remoting/protocol/video_writer.h (right):
>
> http://codereview.chromium.org/4229003/diff/28002/48038#newcode31
> remoting/protocol/video_writer.h:31: virtual void SendPacket(const
> VideoPacket& packet) = 0;
> On 2010/11/03 00:14:36, awong wrote:
>
>> Eventually this is going to be something other than packet, right?
>>
> Right. There is TODO about it in VideoStub and TODO to implement
> VideoStub in this class.
>
>
> http://codereview.chromium.org/4229003/show
>