RtpPacketHistory rewritten to use RtpPacket class.

RtpPacketHistory rewritten to use RtpPacket class.
RtpSender updated to use new version of RtpPacketHistory.

BUG=webrtc:5261
R=asapersson@webrtc.org

Committed: https://crrev.com/31e4e806b18b88055c9342625c988939b4ff79a3
Cr-Commit-Position: refs/heads/master@{#13626}

[danilchap, 2016-05-04 12:04:24]

Patchset #1 (id:1) has been deleted

[danilchap, 2016-05-04 12:04:30]

Patchset #1 (id:20001) has been deleted

[danilchap, 2016-05-04 12:04:37]

Patchset #1 (id:40001) has been deleted

[danilchap, 2016-05-04 12:06:40]

Patchset #1 (id:60001) has been deleted

[danilchap, 2016-05-04 12:52:03]

danilchap@webrtc.org changed reviewers:
+ philipel@webrtc.org, stefan@webrtc.org, terelius@webrtc.org

[danilchap, 2016-05-04 12:52:04]

RtpPacketHistory copy packets, so RtpPacket adjusted accordingly:
  Buffer replaced with CopyOnWriteBuffer delaying memcpy until it is really
needed.
  RtpPacket::size() now match buffer_.size() to make buffer extraction trivial
and safe. (helpful in tests, might be helpful for sending packets later).
Storing RtpPacket object instead of raw buffer removes limit of max
IP_PACKET_SIZE bytes per packet.

RtpSender changed together with RtpPacketHistory to avoid writing temporary
adapter functions.
Number of calls to Parse reduced to 1 with intention to remove it too.
In RtpSender without pacer storing packet to history delayed until packet is
actually send downcode. This avoids extra packet copy and allows to merge
RtpPacketHistory::SetSent into RtpPacketHistory::PutRtpPacket.

[philipel, 2016-05-06 09:42:03]

Only one comment, but in general I don't know enough about
RtpPacket/History/Sender to give particularly valuable feedback.

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_packet_history.h (right):

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_packet_history.h:81:
RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(RtpPacketHistory);
Is there a reason you use constructormagic.h in this file and not in
rtp_packet.h?

[danilchap, 2016-05-06 10:29:52]

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_packet_history.h (right):

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_packet_history.h:81:
RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(RtpPacketHistory);
On 2016/05/06 09:42:02, philipel wrote:
> Is there a reason you use constructormagic.h in this file and not in
> rtp_packet.h?

This file use constructor because it seems appropriate: History class not
expected to be copyable or have a default constructor.

rtp_packet.h do not use constructormagic any more because it turn out RtpPacket
better be copyable.
There is no macro to disallow default constructor without disallowing copy
constructor.

[danilchap, 2016-05-09 10:24:58]

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:978: return
SendToNetwork(std::move(packet), storage, priority) ? 0 : -1;
Here payload_length is ignored and packet->payload_size() used instead downcode.
It might be different: in case of rtx and red old code counts rtx/red headers as
headers while new code counts this 2/1 bytes as payload.
Wonder if this difference worse solving: see next comment.

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1006: payload_length, false);
This is the only place in this function that cares if bytes counted as payload
or headers.
But pacer code give impression it should be [total] length here instead of
payload_length. Is it correct impression?

[stefan-webrtc, 2016-05-09 11:49:22]

stefan@webrtc.org changed reviewers:
+ asapersson@webrtc.org

[stefan-webrtc, 2016-05-09 11:49:23]

Åsa, can you take a look at the changes to rtp_sender.cc as well, in case I've
missed something?

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc (right):

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc:82: if
(stored_packets_[prev_index_].packet &&
Should we check for non-zero length as well, or is that implicit?

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc:159:
std::unique_ptr<RtpPacketToSend> RtpPacketHistory::GetPacket(int index) {
Should this be called something like GetPacketCopyToSend() now that it actually
allocates new memory?

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_packet_history.h (right):

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_packet_history.h:47:
std::unique_ptr<RtpPacketToSend> GetPacketAndSetSendTime(
Maybe rename to GetRtpPacketToSend()?

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (left):

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:570: rtp_parser.Parse(&rtp_header);
Woho!

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:553: if (!packet) {
Remove {}

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:557: if
(!PrepareAndSendPacket(std::move(packet), true, false)) {
Remove {}

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:903: if
(!SendPacketToNetwork(*packet_to_send, options)) {
Remove {}

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1020: if
(transport_feedback_observer_)
{}

[danilchap, 2016-05-09 13:45:22]

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc (right):

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc:82: if
(stored_packets_[prev_index_].packet &&
On 2016/05/09 11:49:23, stefan-webrtc (holmer) wrote:
> Should we check for non-zero length as well, or is that implicit?

This is implicit: history used length = 0 to indicate no packet was stored.
now, with pointer to packet class, nullptr is used to indicate no packet was
stored.
packet class ensure it is in valid state, in particular it's size always >= 12.

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc:159:
std::unique_ptr<RtpPacketToSend> RtpPacketHistory::GetPacket(int index) {
On 2016/05/09 11:49:23, stefan-webrtc (holmer) wrote:
> Should this be called something like GetPacketCopyToSend() now that it
actually
> allocates new memory?

While this function creates new RtpPacketToSend and allocate memory for that
class, it doesn't allocate memory or memcpy the packet/payload itself since
CopyOnWriteBuffer class used for the raw packet.

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_packet_history.h (right):

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_packet_history.h:47:
std::unique_ptr<RtpPacketToSend> GetPacketAndSetSendTime(
On 2016/05/09 11:49:23, stefan-webrtc (holmer) wrote:
> Maybe rename to GetRtpPacketToSend()?

this function still change internal history state by setting send time.
If that functionality will not be needed (i.e.. if pacer would keep packet
instead of seq_no and put it to the history after sending it to network) then
this name would make more sense.

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:553: if (!packet) {
On 2016/05/09 11:49:23, stefan-webrtc (holmer) wrote:
> Remove {}

Done.
What is rule of thumb for not having {}?

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:557: if
(!PrepareAndSendPacket(std::move(packet), true, false)) {
On 2016/05/09 11:49:23, stefan-webrtc (holmer) wrote:
> Remove {}

Done.

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1020: if
(transport_feedback_observer_)
On 2016/05/09 11:49:23, stefan-webrtc (holmer) wrote:
> {}

Done.

[stefan-webrtc, 2016-05-12 12:22:34]

lg, but I'd like the other reviewers to take a look too before I approve

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc (right):

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc:159:
std::unique_ptr<RtpPacketToSend> RtpPacketHistory::GetPacket(int index) {
On 2016/05/09 13:45:22, danilchap wrote:
> On 2016/05/09 11:49:23, stefan-webrtc (holmer) wrote:
> > Should this be called something like GetPacketCopyToSend() now that it
> actually
> > allocates new memory?
> 
> While this function creates new RtpPacketToSend and allocate memory for that
> class, it doesn't allocate memory or memcpy the packet/payload itself since
> CopyOnWriteBuffer class used for the raw packet.
> 

Ah, I see. Looks good then.

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/1945773002/diff/80001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:553: if (!packet) {
On 2016/05/09 13:45:22, danilchap wrote:
> On 2016/05/09 11:49:23, stefan-webrtc (holmer) wrote:
> > Remove {}
> 
> Done.
> What is rule of thumb for not having {}?

I usually go by "if statement <=2 lines, no {}, otherwise always {}"

[danilchap, 2016-05-19 12:30:47]

Description was changed from

==========
RtpPacketHistory rewritten to use RtpPacket class.
RtpSender updated to use RtpPacketHistory.

BUG=webrtc:5261
==========

to

==========
RtpPacketHistory rewritten to use RtpPacket class.
RtpSender updated to use new version of RtpPacketHistory.

BUG=webrtc:5261
==========

[danilchap, 2016-05-20 13:58:47]

danilchap@webrtc.org changed reviewers:
- philipel@webrtc.org, terelius@webrtc.org

[stefan-webrtc, 2016-08-01 11:09:17]

Åsa, could you take a look at this too?

[åsapersson, 2016-08-03 13:40:45]

lgtm

https://codereview.webrtc.org/1945773002/diff/240001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc (right):

https://codereview.webrtc.org/1945773002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc:114:
EXPECT_EQ(capture_time_ms, packet_out->capture_time_ms());
maybe add a test that a retransmit is not retransmitted

[danilchap, 2016-08-03 14:12:45]

https://codereview.webrtc.org/1945773002/diff/240001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc (right):

https://codereview.webrtc.org/1945773002/diff/240001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc:114:
EXPECT_EQ(capture_time_ms, packet_out->capture_time_ms());
On 2016/08/03 13:40:45, åsapersson wrote:
> maybe add a test that a retransmit is not retransmitted

Done.

[danilchap, 2016-08-03 16:27:56]

Description was changed from

==========
RtpPacketHistory rewritten to use RtpPacket class.
RtpSender updated to use new version of RtpPacketHistory.

BUG=webrtc:5261
==========

to

==========
RtpPacketHistory rewritten to use RtpPacket class.
RtpSender updated to use new version of RtpPacketHistory.

BUG=webrtc:5261
R=asapersson@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/31e4e806b18b88055c9342625...
==========

[danilchap, 2016-08-03 16:27:58]

Committed patchset #11 (id:280001) manually as
31e4e806b18b88055c9342625c988939b4ff79a3 (presubmit successful).

[commit-bot: I haz the power, 2016-08-03 16:28:01]

Description was changed from

==========
RtpPacketHistory rewritten to use RtpPacket class.
RtpSender updated to use new version of RtpPacketHistory.

BUG=webrtc:5261
R=asapersson@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/31e4e806b18b88055c9342625...
==========

to

==========
RtpPacketHistory rewritten to use RtpPacket class.
RtpSender updated to use new version of RtpPacketHistory.

BUG=webrtc:5261
R=asapersson@webrtc.org

Committed: https://crrev.com/31e4e806b18b88055c9342625c988939b4ff79a3
Cr-Commit-Position: refs/heads/master@{#13626}
==========

[stefan-webrtc, 2016-08-04 13:48:56]

lgtm