Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1309833002:
Send RTCP packets via RtcpPacket callback (Closed)
|
Created:
5 years, 4 months ago by sprang_webrtc Modified:
5 years ago Reviewers:
åsapersson CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRefactorings to send RTCP packets directly via the RtcpPacket callback, with some simplifications enabled by this. NACK now also sent via RtcpPacket.
BUG=webrtc:2450
R=asapersson@webrtc.org
Committed: https://crrev.com/f7c5776d4254e31e51107388a05c66d14108a8af
Cr-Commit-Position: refs/heads/master@{#10888}
Patch Set 1 #Patch Set 2 : Send RTCP messags directly via callback, refactoring, rebase #
Total comments: 22
Patch Set 3 : Addressed comment, rebase #Patch Set 4 : Rewritten to collect all RtcpPackets first, then send #Patch Set 5 : Cleanup #
Total comments: 6
Patch Set 6 : Addressed comments #Patch Set 7 : Rebase #
Messages
Total messages: 20 (6 generated)
sprang@webrtc.org changed reviewers: + asapersson@webrtc.org
Extended the scope of this CL to also include sending all data via the PacketReady callback, not only RtcpPacket for REMB. PTAL
https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:107: bool BuildPacket(const rtcp::RtcpPacket& packet) { empty.Append(packet); ? https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:499: if (!ctx->BuildPacket(report)) Shouldn't the packet be appended here (e.g. appended on an rtcp::Empty packet)? https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:881: return context.bytes_sent_ > 0 ? 0 : -1; And build the packet here? something like context.BuildAndSentPacket(); BuildAndSentPacket() { uint8_t buffer[IP_PACKET_SIZE]; return empty.BuildExternalBuffer(buffer, IP_PACKET_SIZE, &transport); }
https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:881: return context.bytes_sent_ > 0 ? 0 : -1; On 2015/10/13 08:50:12, åsapersson wrote: > And build the packet here? > > something like > context.BuildAndSentPacket(); > > BuildAndSentPacket() { > uint8_t buffer[IP_PACKET_SIZE]; > return empty.BuildExternalBuffer(buffer, IP_PACKET_SIZE, &transport); > } > That was my initial thought, but it has some drawbacks. Specifically, you would need to heap allocate all the RtcpPacket subclasses used, and then keep them both in the EmptyPacket and another structure, so that we can delete them after serialization. By doing the serialization directly, we can stack-allocate the messages instead, so they have a very clear scope, and should also have better performance. WDYT?
https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:88: buffer_{}, needed? https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:869: return false; return -1; https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:871: lock needed when sending? https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender.h (right): https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.h:153: remove comment https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.h:183: EXCLUSIVE_LOCKS_REQUIRED(critical_section_rtcp_sender_); lock needed?
https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:871: On 2015/11/16 14:54:39, åsapersson wrote: > lock needed when sending? No, the lock was needed before to protect the pointer to the transport (which could change a run-time). Since it's now set at construction we don't need the lock. https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender.h (right): https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.h:153: On 2015/11/16 14:54:39, åsapersson wrote: > remove comment Done. https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.h:183: EXCLUSIVE_LOCKS_REQUIRED(critical_section_rtcp_sender_); On 2015/11/16 14:54:39, åsapersson wrote: > lock needed? To read/update the flags. Problem?
https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:88: buffer_{}, On 2015/11/16 14:54:39, åsapersson wrote: > needed? see comment https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:869: return false; On 2015/11/16 14:54:39, åsapersson wrote: > return -1; see comment https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:871: On 2015/12/01 10:23:06, språng wrote: > On 2015/11/16 14:54:39, åsapersson wrote: > > lock needed when sending? > > No, the lock was needed before to protect the pointer to the transport (which > could change a run-time). Since it's now set at construction we don't need the > lock. Isn't the lock now held when sending (while it wasn't before)? https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender.h (right): https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.h:183: EXCLUSIVE_LOCKS_REQUIRED(critical_section_rtcp_sender_); On 2015/12/01 10:23:06, språng wrote: > On 2015/11/16 14:54:39, åsapersson wrote: > > lock needed? > > To read/update the flags. Problem? But not needed for this function, right?
https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:88: buffer_{}, On 2015/12/01 13:49:20, åsapersson wrote: > On 2015/11/16 14:54:39, åsapersson wrote: > > needed? > > see comment Done. https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:107: bool BuildPacket(const rtcp::RtcpPacket& packet) { On 2015/10/13 08:50:12, åsapersson wrote: > empty.Append(packet); ? Done. https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:499: if (!ctx->BuildPacket(report)) On 2015/10/13 08:50:12, åsapersson wrote: > Shouldn't the packet be appended here (e.g. appended on an rtcp::Empty packet)? Done. https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:869: return false; On 2015/12/01 13:49:20, åsapersson wrote: > On 2015/11/16 14:54:39, åsapersson wrote: > > return -1; > > see comment Done. https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:871: On 2015/12/01 13:49:20, åsapersson wrote: > On 2015/12/01 10:23:06, språng wrote: > > On 2015/11/16 14:54:39, åsapersson wrote: > > > lock needed when sending? > > > > No, the lock was needed before to protect the pointer to the transport (which > > could change a run-time). Since it's now set at construction we don't need the > > lock. > > Isn't the lock now held when sending (while it wasn't before)? Changed so we collect RtcpPacket instances first, as requested. Then release lock and send. https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender.h (right): https://codereview.webrtc.org/1309833002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.h:183: EXCLUSIVE_LOCKS_REQUIRED(critical_section_rtcp_sender_); On 2015/12/01 13:49:20, åsapersson wrote: > On 2015/12/01 10:23:06, språng wrote: > > On 2015/11/16 14:54:39, åsapersson wrote: > > > lock needed? > > > > To read/update the flags. Problem? > > But not needed for this function, right? Done.
lgtm with minor comments addressed https://codereview.webrtc.org/1309833002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet.cc (left): https://codereview.webrtc.org/1309833002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet.cc:616: prefer keeping new line https://codereview.webrtc.org/1309833002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet.h (left): https://codereview.webrtc.org/1309833002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet.h:88: prefer keeping new line https://codereview.webrtc.org/1309833002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1309833002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:884: return bytes_sent <= 0 ? -1 : 0; size_t, bytes_sent == 0
https://codereview.webrtc.org/1309833002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet.cc (left): https://codereview.webrtc.org/1309833002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet.cc:616: On 2015/12/03 15:53:57, åsapersson wrote: > prefer keeping new line Done. https://codereview.webrtc.org/1309833002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_packet.h (left): https://codereview.webrtc.org/1309833002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_packet.h:88: On 2015/12/03 15:53:57, åsapersson wrote: > prefer keeping new line Done. https://codereview.webrtc.org/1309833002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1309833002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:884: return bytes_sent <= 0 ? -1 : 0; On 2015/12/03 15:53:57, åsapersson wrote: > size_t, bytes_sent == 0 Done.
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from asapersson@webrtc.org Link to the patchset: https://codereview.webrtc.org/1309833002/#ps120001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1309833002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1309833002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/2144)
Description was changed from ========== Refactorings to send RTCP packets directly via the RtcpPacket callback, with some simplifications enabled by this. NACK now also sent via RtcpPacket. BUG=webrtc:2450 ========== to ========== Refactorings to send RTCP packets directly via the RtcpPacket callback, with some simplifications enabled by this. NACK now also sent via RtcpPacket. BUG=webrtc:2450 R=asapersson@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/f7c5776d4254e31e51107388a... ==========
Message was sent while issue was closed.
Description was changed from ========== Refactorings to send RTCP packets directly via the RtcpPacket callback, with some simplifications enabled by this. NACK now also sent via RtcpPacket. BUG=webrtc:2450 R=asapersson@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/f7c5776d4254e31e51107388a... ========== to ========== Refactorings to send RTCP packets directly via the RtcpPacket callback, with some simplifications enabled by this. NACK now also sent via RtcpPacket. BUG=webrtc:2450 R=asapersson@webrtc.org Committed: https://crrev.com/f7c5776d4254e31e51107388a05c66d14108a8af Cr-Commit-Position: refs/heads/master@{#10888} ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) manually as f7c5776d4254e31e51107388a05c66d14108a8af (presubmit successful).
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/f7c5776d4254e31e51107388a05c66d14108a8af Cr-Commit-Position: refs/heads/master@{#10888} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
