Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2292883002:
Use RtpPacketToSend in RtpSenderAudio (Closed)
|
Created:
4 years, 3 months ago by danilchap Modified:
4 years, 3 months ago Reviewers:
ossu CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUse RtpPacketToSend in RtpSenderAudio.
this eliminates reparsing of rtp packet on send audio path
BUG=webrtc:5261
Committed: https://crrev.com/84c8528f1e17aa73d3cb3af3860ce1821d6c67b6
Cr-Commit-Position: refs/heads/master@{#14072}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : Named constants #
Total comments: 9
Patch Set 4 : Feedback #Messages
Total messages: 13 (6 generated)
Description was changed from ========== [Draft] Propogate use of RtpPacketToSend to RtpSenderAudio BUG=webrtc:5261 ========== to ========== Use RtpPacketToSend to RtpSenderAudio. this eliminates reparsing of rtp packet on send audio path BUG=webrtc:5261 ==========
Description was changed from ========== Use RtpPacketToSend to RtpSenderAudio. this eliminates reparsing of rtp packet on send audio path BUG=webrtc:5261 ========== to ========== Use RtpPacketToSend in RtpSenderAudio. this eliminates reparsing of rtp packet on send audio path BUG=webrtc:5261 ==========
danilchap@webrtc.org changed reviewers: + ossu@webrtc.org
Looks good in general - seems to do what it did before but using RtpPacketToSend. :) I've a couple of questions though. https://codereview.webrtc.org/2292883002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc (right): https://codereview.webrtc.org/2292883002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc:253: packet->set_capture_time_ms(clock_->TimeInMilliseconds()); This is clearly around as good as it was before (when it was set by the SendToNetwork call) but is it correct? Should it be recorded earlier in the chain and passed on? https://codereview.webrtc.org/2292883002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc:255: packet->SetExtension<AudioLevel>(frame_type == kAudioFrameSpeech, So this is in place of the UpdateAudioLevel call (below), I expect. Are we moving away from calling RtpSender and instead to call RtpPacketToSend? If so, could UpdateAudioLevel be removed with this CL? I see it only called from unittests outside of this method. https://codereview.webrtc.org/2292883002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc:331: constexpr size_t kBaseRtpHeaderSize = 12; Why not use kRtpHeaderSize in rtp_rtcp_defines.h?
https://codereview.webrtc.org/2292883002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc (right): https://codereview.webrtc.org/2292883002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc:253: packet->set_capture_time_ms(clock_->TimeInMilliseconds()); On 2016/09/05 11:42:14, ossu wrote: > This is clearly around as good as it was before (when it was set by the > SendToNetwork call) but is it correct? Should it be recorded earlier in the > chain and passed on? In theory yes, it is better to record it about same time rtp_timestamp is generated. RtpRtcpModule::SendOutgoingData accepts this parameter, but Audio channel currently sets it to -1 (aka no value). In practice it is not that important as long as path between audio capture and this function is not that long. https://codereview.webrtc.org/2292883002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc:255: packet->SetExtension<AudioLevel>(frame_type == kAudioFrameSpeech, On 2016/09/05 11:42:14, ossu wrote: > So this is in place of the UpdateAudioLevel call (below), I expect. > Are we moving away from calling RtpSender and instead to call RtpPacketToSend? > If so, could UpdateAudioLevel be removed with this CL? I see it only called from > unittests outside of this method. Yes, plan is to move serialization logic from rtp_sender to rtp_packet. Yes, UpdateAudioLevel and relative members can be removed after this CL. Planing to do it later, after updating video path and tests too, meanwhile making this CL smaller. https://codereview.webrtc.org/2292883002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc:331: constexpr size_t kBaseRtpHeaderSize = 12; On 2016/09/05 11:42:14, ossu wrote: > Why not use kRtpHeaderSize in rtp_rtcp_defines.h? No good enough reason. Changed.
lgtm https://codereview.webrtc.org/2292883002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc (right): https://codereview.webrtc.org/2292883002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc:253: packet->set_capture_time_ms(clock_->TimeInMilliseconds()); On 2016/09/05 12:20:18, danilchap wrote: > On 2016/09/05 11:42:14, ossu wrote: > > This is clearly around as good as it was before (when it was set by the > > SendToNetwork call) but is it correct? Should it be recorded earlier in the > > chain and passed on? > > In theory yes, it is better to record it about same time rtp_timestamp is > generated. RtpRtcpModule::SendOutgoingData accepts this parameter, but Audio > channel currently sets it to -1 (aka no value). > > In practice it is not that important as long as path between audio capture and > this function is not that long. Acknowledged. https://codereview.webrtc.org/2292883002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc:255: packet->SetExtension<AudioLevel>(frame_type == kAudioFrameSpeech, On 2016/09/05 12:20:18, danilchap wrote: > On 2016/09/05 11:42:14, ossu wrote: > > So this is in place of the UpdateAudioLevel call (below), I expect. > > Are we moving away from calling RtpSender and instead to call RtpPacketToSend? > > If so, could UpdateAudioLevel be removed with this CL? I see it only called > from > > unittests outside of this method. > > Yes, plan is to move serialization logic from rtp_sender to rtp_packet. > Yes, UpdateAudioLevel and relative members can be removed after this CL. > Planing to do it later, after updating video path and tests too, meanwhile > making this CL smaller. Acknowledged. https://codereview.webrtc.org/2292883002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_audio.cc:331: constexpr size_t kBaseRtpHeaderSize = 12; On 2016/09/05 12:20:18, danilchap wrote: > On 2016/09/05 11:42:14, ossu wrote: > > Why not use kRtpHeaderSize in rtp_rtcp_defines.h? > > No good enough reason. > Changed. Acknowledged.
The CQ bit was checked by danilchap@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Use RtpPacketToSend in RtpSenderAudio. this eliminates reparsing of rtp packet on send audio path BUG=webrtc:5261 ========== to ========== Use RtpPacketToSend in RtpSenderAudio. this eliminates reparsing of rtp packet on send audio path BUG=webrtc:5261 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Use RtpPacketToSend in RtpSenderAudio. this eliminates reparsing of rtp packet on send audio path BUG=webrtc:5261 ========== to ========== Use RtpPacketToSend in RtpSenderAudio. this eliminates reparsing of rtp packet on send audio path BUG=webrtc:5261 Committed: https://crrev.com/84c8528f1e17aa73d3cb3af3860ce1821d6c67b6 Cr-Commit-Position: refs/heads/master@{#14072} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/84c8528f1e17aa73d3cb3af3860ce1821d6c67b6 Cr-Commit-Position: refs/heads/master@{#14072} |
