Packetizer/Depacketizer for RTP.

Packetizer/Depacketizer for RTP.

BUG=None
TEST=None

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

[Alpha Left Google, 2010-11-15 23:46:35]

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_reader.h
File remoting/protocol/rtp_reader.h (left):

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_reade...
remoting/protocol/rtp_reader.h:38: typedef Callback1<const RtpPacket&>::Type
OnMessageCallback;
why this is changed to non-const?

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils.cc
File remoting/protocol/rtp_utils.cc (right):

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:114: // I: 1 bit
Doesn't say what is RSV in the description.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:170: CHECK_GT(buffer_size, 0);
don't use CHECK here.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:172: buffer[0] =
This doesn't seem to match the diagram above..

For example B is bit 7 but according to this state it is bit 4.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:184: CHECK_LT(pos, buffer_size);
and here.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:200: bool picture_id_present =
ExtractBits(buffer[0], 1, 4) != 0;
What about 
= !!ExtractBits(...);

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:201: descriptor->non_reference_frame =
ExtractBits(buffer[0], 1, 3) != 0;
ditto.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:203: descriptor->frame_beginning =
ExtractBits(buffer[0], 1, 0) != 0;
ditto.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils.h
File remoting/protocol/rtp_utils.h (right):

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.h:28: enum FragmentationInfo {
Need explanation for these values

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_video...
File remoting/protocol/rtp_video_reader.cc (right):

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_video...
remoting/protocol/rtp_video_reader.cc:44: void
RtpVideoReader::OnRtpPacket(RtpPacket* rtp_packet) {
Looks like this can be const *, you can still delete it.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_video...
remoting/protocol/rtp_video_reader.cc:73: CHECK_LT(packet_index,
static_cast<int>(packets_queue_.size()));
can we not use CHECK but return failure so that we can skip this packet?

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_writer.h
File remoting/protocol/rtp_writer.h (right):

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_write...
remoting/protocol/rtp_writer.h:9: #include "remoting/protocol/rtp_utils.h"
order of include.

[Sergey Ulanov, 2010-11-16 00:22:55]

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_reader.h
File remoting/protocol/rtp_reader.h (left):

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_reade...
remoting/protocol/rtp_reader.h:38: typedef Callback1<const RtpPacket&>::Type
OnMessageCallback;
On 2010/11/15 23:46:35, Alpha wrote:
> why this is changed to non-const?
Changed back to const.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils.cc
File remoting/protocol/rtp_utils.cc (right):

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:114: // I: 1 bit
On 2010/11/15 23:46:35, Alpha wrote:
> Doesn't say what is RSV in the description.

Done.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:170: CHECK_GT(buffer_size, 0);
On 2010/11/15 23:46:35, Alpha wrote:
> don't use CHECK here.
Why? I think, it is better to use CHECK here to prevent potential security risk.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:172: buffer[0] =
On 2010/11/15 23:46:35, Alpha wrote:
> This doesn't seem to match the diagram above..
> 
> For example B is bit 7 but according to this state it is bit 4.

B is frame_beginning. It is 7'th bit in the first byte, i.e. lowest bit (keep in
mind that the diagrams are big-endian).
I = (picture_id != kuint32max), it is 3'rd bit.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:184: CHECK_LT(pos, buffer_size);
On 2010/11/15 23:46:35, Alpha wrote:
> and here.
Same reason as above. The check is fairly cheap, and we risk creating security
holes without it.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:200: bool picture_id_present =
ExtractBits(buffer[0], 1, 4) != 0;
On 2010/11/15 23:46:35, Alpha wrote:
> What about 
> = !!ExtractBits(...);
IMHO, !=0 looks nicer, no?

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils.h
File remoting/protocol/rtp_utils.h (right):

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.h:28: enum FragmentationInfo {
On 2010/11/15 23:46:35, Alpha wrote:
> Need explanation for these values
These are documented in the spec. Added comment with reference to the spec.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_video...
File remoting/protocol/rtp_video_reader.cc (right):

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_video...
remoting/protocol/rtp_video_reader.cc:44: void
RtpVideoReader::OnRtpPacket(RtpPacket* rtp_packet) {
On 2010/11/15 23:46:35, Alpha wrote:
> Looks like this can be const *, you can still delete it.
Agree. Changed this to const

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_video...
remoting/protocol/rtp_video_reader.cc:73: CHECK_LT(packet_index,
static_cast<int>(packets_queue_.size()));
On 2010/11/15 23:46:35, Alpha wrote:
> can we not use CHECK but return failure so that we can skip this packet?
The code above guarantees that packet_index <
static_cast<int>(packets_queue_.size()), so CHECK is enough here.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_writer.h
File remoting/protocol/rtp_writer.h (right):

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_write...
remoting/protocol/rtp_writer.h:9: #include "remoting/protocol/rtp_utils.h"
On 2010/11/15 23:46:35, Alpha wrote:
> order of include.

Done.

[Alpha Left Google, 2010-11-16 00:47:07]

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils.cc
File remoting/protocol/rtp_utils.cc (right):

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:172: buffer[0] =
On 2010/11/16 00:22:55, sergeyu wrote:
> On 2010/11/15 23:46:35, Alpha wrote:
> > This doesn't seem to match the diagram above..
> > 
> > For example B is bit 7 but according to this state it is bit 4.
> 
> B is frame_beginning. It is 7'th bit in the first byte, i.e. lowest bit (keep
in
> mind that the diagrams are big-endian).
> I = (picture_id != kuint32max), it is 3'rd bit.

I see.. if this is the case this code won't work on BE system isn't it?
Shouldn't we just assume this is a regular integer and does a SetBE(value) at
the end?

[Sergey Ulanov, 2010-11-16 01:06:10]

I've added comments about order of bits on the diagrams.

On Mon, Nov 15, 2010 at 4:47 PM, <hclam@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils.cc
> File remoting/protocol/rtp_utils.cc (right):
>
>
>
http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
> remoting/protocol/rtp_utils.cc:172: buffer[0] =
> On 2010/11/16 00:22:55, sergeyu wrote:
>
>> On 2010/11/15 23:46:35, Alpha wrote:
>> > This doesn't seem to match the diagram above..
>> >
>> > For example B is bit 7 but according to this state it is bit 4.
>>
>
>  B is frame_beginning. It is 7'th bit in the first byte, i.e. lowest
>>
> bit (keep in
>
>> mind that the diagrams are big-endian).
>> I = (picture_id != kuint32max), it is 3'rd bit.
>>
>
> I see.. if this is the case this code won't work on BE system isn't it?
> Shouldn't we just assume this is a regular integer and does a
> SetBE(value) at the end?
>
>
> http://codereview.chromium.org/4925001/
>

[Alpha Left Google, 2010-11-16 01:06:42]

LGTM.

[awong, 2010-11-16 01:08:40]

Seems like Alpha's got this review.  I'll just send the comments that I had. 
LGTM if he says so.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils.cc
File remoting/protocol/rtp_utils.cc (right):

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:38: //
+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+
nice diagram.:)

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:170: CHECK_GT(buffer_size, 0);
On 2010/11/15 23:46:35, Alpha wrote:
> don't use CHECK here.

I think CHECK makes sense.  allowing a negative size in production code sounds
dangerous.

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:173: ((uint8)(descriptor.picture_id !=
kuint32max) << 4) +
Since we're playing in bits, I'm personally happier seeing | over +.  Any reason
to choose +?

http://codereview.chromium.org/4925001/diff/17001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:46: return kRtpBaseHeaderSize + header.sources *
4;
magic number....why *4?

http://codereview.chromium.org/4925001/diff/17001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:194: 
I personally find 0x80 to be easier to read -- espcially since it's symmetric
with the mask below -- but whatever works.

http://codereview.chromium.org/4925001/diff/17001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:210: while (extension) {
Can we invert this conditional, and early return on the degenerate case of not
having a picture_id?

Then the rest of the code can be moved out one level...

[Sergey Ulanov, 2010-11-16 01:52:25]

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils.cc
File remoting/protocol/rtp_utils.cc (right):

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:38: //
+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+
On 2010/11/16 01:08:40, awong wrote:
> nice diagram.:)
I didn't draw it myself, it's from RTP spec :)

http://codereview.chromium.org/4925001/diff/10001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:173: ((uint8)(descriptor.picture_id !=
kuint32max) << 4) +
On 2010/11/16 01:08:40, awong wrote:
> Since we're playing in bits, I'm personally happier seeing | over +.  Any
reason
> to choose +?
Agree that | is better here. Fixed here and in PackRtpHeader.

http://codereview.chromium.org/4925001/diff/17001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:46: return kRtpBaseHeaderSize + header.sources *
4;
On 2010/11/16 01:08:40, awong wrote:
> magic number....why *4?
Size of each CSRC in the header. Added kBytesPerCSRC.

http://codereview.chromium.org/4925001/diff/17001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:194: 
On 2010/11/16 01:08:40, awong wrote:
> I personally find 0x80 to be easier to read -- espcially since it's symmetric
> with the mask below -- but whatever works.

Done.

http://codereview.chromium.org/4925001/diff/17001/remoting/protocol/rtp_utils...
remoting/protocol/rtp_utils.cc:210: while (extension) {
On 2010/11/16 01:08:40, awong wrote:
> Can we invert this conditional, and early return on the degenerate case of not
> having a picture_id?
> 
> Then the rest of the code can be moved out one level...

Done.