Use RtpPacketToSend in RtpSenderVideo

Use RtpPacketToSend in RtpSenderVideo.
This reduce reparsing rtp packet while sending.

BUG=webrtc:5261
Committed: https://crrev.com/7411061982ed43190ca16432f498802e00924d37
Cr-Commit-Position: refs/heads/master@{#14465}

[danilchap, 2016-08-08 07:39:37]

Description was changed from

==========
[Splitting] Propogate use of RtpPacketToSend to RtpSenderVideo/RtpSenderAudio
Remove now unused packetization function in the RtpSender.
==========

to

==========
[Draft] Propogate use of RtpPacketToSend to RtpSenderVideo/RtpSenderAudio
Remove now unused packetization function in the RtpSender.
==========

[danilchap, 2016-08-08 07:40:48]

Description was changed from

==========
[Draft] Propogate use of RtpPacketToSend to RtpSenderVideo/RtpSenderAudio
Remove now unused packetization function in the RtpSender.
==========

to

==========
Propogate use of RtpPacketToSend to RtpSenderVideo/RtpSenderAudio
Remove now unused packetization function in the RtpSender.
==========

[brandtr, 2016-08-15 08:31:30]

brandtr@webrtc.org changed reviewers:
+ brandtr@webrtc.org

[brandtr, 2016-08-15 08:31:31]

Some very minor comments :)

https://codereview.webrtc.org/2217383002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/producer_fec.h (right):

https://codereview.webrtc.org/2217383002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/producer_fec.h:52: void BuildRedPacket(int
red_pl_type,
For consistency after rebase, consider replacing with 'red_payload_type'. (See
row 51.)

https://codereview.webrtc.org/2217383002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2217383002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:101: // Do it old, slow way
for now.
Not sure what this comment means?

[danilchap, 2016-08-15 08:55:16]

https://codereview.webrtc.org/2217383002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/producer_fec.h (right):

https://codereview.webrtc.org/2217383002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/producer_fec.h:52: void BuildRedPacket(int
red_pl_type,
On 2016/08/15 08:31:31, brandtr wrote:
> For consistency after rebase, consider replacing with 'red_payload_type'. (See
> row 51.)

Done.

https://codereview.webrtc.org/2217383002/diff/100001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2217383002/diff/100001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:101: // Do it old, slow way
for now.
On 2016/08/15 08:31:31, brandtr wrote:
> Not sure what this comment means?
Mean this function is too drafty.
Not relevant any more, replaced with better (TODO) comments.

[danilchap, 2016-08-19 17:20:17]

danilchap@webrtc.org changed reviewers:
- brandtr@webrtc.org

[danilchap, 2016-09-02 18:21:37]

Description was changed from

==========
Propogate use of RtpPacketToSend to RtpSenderVideo/RtpSenderAudio
Remove now unused packetization function in the RtpSender.
==========

to

==========
Propogate use of RtpPacketToSend to RtpSenderVideo
==========

[danilchap, 2016-09-02 18:23:44]

Patchset #8 (id:140001) has been deleted

[danilchap, 2016-09-02 18:26:32]

Patchset #8 (id:160001) has been deleted

[danilchap, 2016-09-08 07:43:51]

Description was changed from

==========
Propogate use of RtpPacketToSend to RtpSenderVideo
==========

to

==========
Use RtpPacketToSend in RtpSenderVideo.
This reduce reparsing rtp packet while sending.
==========

[danilchap, 2016-09-08 07:44:03]

Patchset #13 (id:280001) has been deleted

[danilchap, 2016-09-08 10:25:23]

Patchset #14 (id:320001) has been deleted

[danilchap, 2016-09-12 19:12:58]

danilchap@webrtc.org changed reviewers:
+ asapersson@webrtc.org, sprang@webrtc.org

[danilchap, 2016-09-27 07:53:06]

ptal, is it reviewable or should I slice it into several chunks?

[sprang_webrtc, 2016-09-27 10:45:00]

https://codereview.webrtc.org/2217383002/diff/400001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right):

https://codereview.webrtc.org/2217383002/diff/400001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_packet.cc:277:
buffer_.SetSize(payload_offset_);  // Reset payload size to avoid copying it.
I'm not sure I follow this. How does doing this SetSize before the next two
lines below help avoid a copy?

https://codereview.webrtc.org/2217383002/diff/400001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2217383002/diff/400001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:296:
std::unique_ptr<RtpPacketToSend> packet(new RtpPacketToSend(*rtp_header));
Have there been any discussion re going from stack to heap allocation? Are we
thinking RTP packets are small enough or short-lived enough that memory
fragmentation won't be problem for instance?
Maybe we should look into using a buffer pool?

[danilchap, 2016-09-27 11:47:05]

https://codereview.webrtc.org/2217383002/diff/400001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right):

https://codereview.webrtc.org/2217383002/diff/400001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_packet.cc:277:
buffer_.SetSize(payload_offset_);  // Reset payload size to avoid copying it.
On 2016/09/27 10:45:00, språng wrote:
> I'm not sure I follow this. How does doing this SetSize before the next two
> lines below help avoid a copy?

comment expanded.
this optimization was added to RtpPacket (though used only when creating red
packet from media_packet) because it use internal knowledge how
CopyOnWriteBuffer buffer_ works.

https://codereview.webrtc.org/2217383002/diff/400001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2217383002/diff/400001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:296:
std::unique_ptr<RtpPacketToSend> packet(new RtpPacketToSend(*rtp_header));
On 2016/09/27 10:45:00, språng wrote:
> Have there been any discussion re going from stack to heap allocation? Are we
> thinking RTP packets are small enough or short-lived enough that memory
> fragmentation won't be problem for instance?
> Maybe we should look into using a buffer pool?

Acknowledged.
No, there was no discussion about potential problems going from stack to heap.
RtpPackets do not live too long (max few seconds while kept in packet history),
so dedicated memory management like buffer pool is probably a preliminary
optimization.

e.g. buffer pool would require some far from trivial changes to work well with
CopyOnWriteBuffer that was chosen to pass raw packet to network code over thread
boundaries.

[sprang_webrtc, 2016-09-27 12:16:37]

lgtm

https://codereview.webrtc.org/2217383002/diff/400001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right):

https://codereview.webrtc.org/2217383002/diff/400001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:296:
std::unique_ptr<RtpPacketToSend> packet(new RtpPacketToSend(*rtp_header));
On 2016/09/27 11:47:05, danilchap wrote:
> On 2016/09/27 10:45:00, språng wrote:
> > Have there been any discussion re going from stack to heap allocation? Are
we
> > thinking RTP packets are small enough or short-lived enough that memory
> > fragmentation won't be problem for instance?
> > Maybe we should look into using a buffer pool?
> 
> Acknowledged.
> No, there was no discussion about potential problems going from stack to heap.
> RtpPackets do not live too long (max few seconds while kept in packet
history),
> so dedicated memory management like buffer pool is probably a preliminary
> optimization.
> 
> e.g. buffer pool would require some far from trivial changes to work well with
> CopyOnWriteBuffer that was chosen to pass raw packet to network code over
thread
> boundaries.

Fair enough. Might be worth keeping an eye on the perf bots/stats though.

[danilchap, 2016-10-02 17:04:38]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2016-10-02 17:04:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-02 17:09:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-02 17:09:51]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8858)

[danilchap, 2016-10-02 17:24:44]

Description was changed from

==========
Use RtpPacketToSend in RtpSenderVideo.
This reduce reparsing rtp packet while sending.
==========

to

==========
Use RtpPacketToSend in RtpSenderVideo.
This reduce reparsing rtp packet while sending.

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

[danilchap, 2016-10-02 17:25:03]

The CQ bit was checked by danilchap@webrtc.org

[commit-bot: I haz the power, 2016-10-02 17:25:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-10-02 17:54:32]

Description was changed from

==========
Use RtpPacketToSend in RtpSenderVideo.
This reduce reparsing rtp packet while sending.

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

to

==========
Use RtpPacketToSend in RtpSenderVideo.
This reduce reparsing rtp packet while sending.

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

[commit-bot: I haz the power, 2016-10-02 17:54:33]

Committed patchset #18 (id:420001)

[commit-bot: I haz the power, 2016-10-02 17:54:56]

Description was changed from

==========
Use RtpPacketToSend in RtpSenderVideo.
This reduce reparsing rtp packet while sending.

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

to

==========
Use RtpPacketToSend in RtpSenderVideo.
This reduce reparsing rtp packet while sending.

BUG=webrtc:5261

Committed: https://crrev.com/7411061982ed43190ca16432f498802e00924d37
Cr-Commit-Position: refs/heads/master@{#14465}
==========

[commit-bot: I haz the power, 2016-10-02 17:54:58]

Patchset 18 (id:??) landed as
https://crrev.com/7411061982ed43190ca16432f498802e00924d37
Cr-Commit-Position: refs/heads/master@{#14465}