Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2228493002:
Generate random rtp packets with RtpPacketToSend instead of RtpSender (Closed)
|
Created:
4 years, 4 months ago by danilchap Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, pbos-webrtc, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionGenerate random rtp packets with RtpPacketToSend instead of RtpSender
in rtc event log unittest
R=stefan@webrtc.org, terelius@webrtc.org
Committed: https://crrev.com/bcdad0f3a8e9d71991a540ac5c7bd0a2bea90d1f
Cr-Commit-Position: refs/heads/master@{#13705}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 15 (5 generated)
Description was changed from ========== Generate random rtp packets with RtpPacketToSend instead of RtpSender in rtc event log unittest ========== to ========== Generate random rtp packets with RtpPacketToSend instead of RtpSender in rtc event log unittest ==========
danilchap@webrtc.org changed reviewers: + terelius@webrtc.org
Nice! https://codereview.webrtc.org/2228493002/diff/1/webrtc/call/rtc_event_log_uni... File webrtc/call/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/2228493002/diff/1/webrtc/call/rtc_event_log_uni... webrtc/call/rtc_event_log_unittest.cc:128: rtp_packet.SetExtension<TransmissionOffset>(prng->Rand(0x00ffffff)); These functions set the extension if and only if that extension type has been configured in the RtpHeaderExtensionMap?
https://codereview.webrtc.org/2228493002/diff/1/webrtc/call/rtc_event_log_uni... File webrtc/call/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/2228493002/diff/1/webrtc/call/rtc_event_log_uni... webrtc/call/rtc_event_log_unittest.cc:128: rtp_packet.SetExtension<TransmissionOffset>(prng->Rand(0x00ffffff)); On 2016/08/09 11:43:00, terelius wrote: > These functions set the extension if and only if that extension type has been > configured in the RtpHeaderExtensionMap? In this case (calling functions before setting payload) - yes. Those functions also return false when extension is not configured in the RtpHeaderExtensionMap or the map is absent (nullptr).
https://codereview.webrtc.org/2228493002/diff/1/webrtc/call/rtc_event_log_uni... File webrtc/call/rtc_event_log_unittest.cc (right): https://codereview.webrtc.org/2228493002/diff/1/webrtc/call/rtc_event_log_uni... webrtc/call/rtc_event_log_unittest.cc:128: rtp_packet.SetExtension<TransmissionOffset>(prng->Rand(0x00ffffff)); On 2016/08/09 12:49:23, danilchap wrote: > On 2016/08/09 11:43:00, terelius wrote: > > These functions set the extension if and only if that extension type has been > > configured in the RtpHeaderExtensionMap? > > In this case (calling functions before setting payload) - yes. > Those functions also return false when extension is not configured in the > RtpHeaderExtensionMap or the map is absent (nullptr). So you can only set new extensions before payload, and only if they have been configured in RtpHeaderExtensionMap. Is that correct?
On 2016/08/09 13:31:32, terelius wrote: > https://codereview.webrtc.org/2228493002/diff/1/webrtc/call/rtc_event_log_uni... > File webrtc/call/rtc_event_log_unittest.cc (right): > > https://codereview.webrtc.org/2228493002/diff/1/webrtc/call/rtc_event_log_uni... > webrtc/call/rtc_event_log_unittest.cc:128: > rtp_packet.SetExtension<TransmissionOffset>(prng->Rand(0x00ffffff)); > On 2016/08/09 12:49:23, danilchap wrote: > > On 2016/08/09 11:43:00, terelius wrote: > > > These functions set the extension if and only if that extension type has > been > > > configured in the RtpHeaderExtensionMap? > > > > In this case (calling functions before setting payload) - yes. > > Those functions also return false when extension is not configured in the > > RtpHeaderExtensionMap or the map is absent (nullptr). > > So you can only set new extensions before payload, and only if they have been > configured in RtpHeaderExtensionMap. Is that correct? Almost: you can also reserve new extension before payload and set it after. You can't set an extension that is not configured in RtpHeaderExtensionMap because you need a mapping from extension to id. And you need to set or reserve extensions in advance for another technical reason: it's better to know how much space extensions would occupy before setting payload to avoid relocating it. Once extension is reserved (or set) you may change it's value after payload is populated.
On 2016/08/09 13:53:09, danilchap wrote: > On 2016/08/09 13:31:32, terelius wrote: > > > https://codereview.webrtc.org/2228493002/diff/1/webrtc/call/rtc_event_log_uni... > > File webrtc/call/rtc_event_log_unittest.cc (right): > > > > > https://codereview.webrtc.org/2228493002/diff/1/webrtc/call/rtc_event_log_uni... > > webrtc/call/rtc_event_log_unittest.cc:128: > > rtp_packet.SetExtension<TransmissionOffset>(prng->Rand(0x00ffffff)); > > On 2016/08/09 12:49:23, danilchap wrote: > > > On 2016/08/09 11:43:00, terelius wrote: > > > > These functions set the extension if and only if that extension type has > > been > > > > configured in the RtpHeaderExtensionMap? > > > > > > In this case (calling functions before setting payload) - yes. > > > Those functions also return false when extension is not configured in the > > > RtpHeaderExtensionMap or the map is absent (nullptr). > > > > So you can only set new extensions before payload, and only if they have been > > configured in RtpHeaderExtensionMap. Is that correct? > > Almost: you can also reserve new extension before payload and set it after. > > You can't set an extension that is not configured in RtpHeaderExtensionMap > because you need a mapping from extension to id. > And you need to set or reserve extensions in advance for another technical > reason: it's better to know how much space extensions would occupy before > setting payload to avoid relocating it. > Once extension is reserved (or set) you may change it's value after payload is > populated. Acknowledged.
lgtm
danilchap@webrtc.org changed reviewers: + stefan@webrtc.org
Stefan, can you check from owner point of review.
lgtm
Description was changed from ========== Generate random rtp packets with RtpPacketToSend instead of RtpSender in rtc event log unittest ========== to ========== Generate random rtp packets with RtpPacketToSend instead of RtpSender in rtc event log unittest R=stefan@webrtc.org, terelius@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/bcdad0f3a8e9d71991a540ac5... ==========
Message was sent while issue was closed.
Description was changed from ========== Generate random rtp packets with RtpPacketToSend instead of RtpSender in rtc event log unittest R=stefan@webrtc.org, terelius@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/bcdad0f3a8e9d71991a540ac5... ========== to ========== Generate random rtp packets with RtpPacketToSend instead of RtpSender in rtc event log unittest R=stefan@webrtc.org, terelius@webrtc.org Committed: https://crrev.com/bcdad0f3a8e9d71991a540ac5c7bd0a2bea90d1f Cr-Commit-Position: refs/heads/master@{#13705} ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as bcdad0f3a8e9d71991a540ac5c7bd0a2bea90d1f (presubmit successful).
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/bcdad0f3a8e9d71991a540ac5c7bd0a2bea90d1f Cr-Commit-Position: refs/heads/master@{#13705} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
