Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2867713003:
Remove hardcoded kValueSizeBytes values from variable-length header extensions. (Closed)
|
Created:
3 years, 7 months ago by erikvarga1 Modified:
3 years, 6 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman, nisse-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemove hardcoded kValueSizeBytes values from variable-length header extensions.
Since the RtpStreamId and RepairedRtpStreamId extensions can have variable
length, it makes no sense for them to have a constant valueSize field.
The header length calculation in RtpHeaderExtensionMap needed to be changed
for this because it previously worked with the assumption that all header
types have a constant size. Now it's the caller's job to specify the length
of the extensions that it might use.
BUG=webrtc:7433
Review-Url: https://codereview.webrtc.org/2867713003
Cr-Commit-Position: refs/heads/master@{#18179}
Committed: https://chromium.googlesource.com/external/webrtc/+/2788373528ec05ccb3e3107ee67fb12819011f01
Patch Set 1 #
Total comments: 16
Patch Set 2 : Patch 1 #
Total comments: 15
Patch Set 3 : Patch 2 #
Total comments: 10
Patch Set 4 : Patch 3 #
Total comments: 5
Patch Set 5 : Patch 4 #
Total comments: 6
Patch Set 6 : Patch 5 #
Total comments: 7
Patch Set 7 : Patch 6 #Patch Set 8 : Patch 7 #
Total comments: 1
Patch Set 9 : use RaceChecker instead of ThreadChecker in a few places. #
Messages
Total messages: 52 (27 generated)
Description was changed from ========== Remove hardcoded kValueSizeBytes values from variable-length header extensions. Since the RtpStreamId and RepairedRtpStreamId extensions can have variable length, it makes no sense for them to have a constant valueSize field. The header length calculation in RtpHeaderExtensionMap needed to be changed for this because it previously worked with the assumption that all header types have a constant size. Now it's the caller's job to specify the length of the extensions that it might use. BUG= ========== to ========== Remove hardcoded kValueSizeBytes values from variable-length header extensions. Since the RtpStreamId and RepairedRtpStreamId extensions can have variable length, it makes no sense for them to have a constant valueSize field. The header length calculation in RtpHeaderExtensionMap needed to be changed for this because it previously worked with the assumption that all header types have a constant size. Now it's the caller's job to specify the length of the extensions that it might use. BUG= ==========
erikvarga@google.com changed reviewers: + danilchap@webrtc.org
erikvarga@google.com changed reviewers: + erikvarga@google.com
Description was changed from ========== Remove hardcoded kValueSizeBytes values from variable-length header extensions. Since the RtpStreamId and RepairedRtpStreamId extensions can have variable length, it makes no sense for them to have a constant valueSize field. The header length calculation in RtpHeaderExtensionMap needed to be changed for this because it previously worked with the assumption that all header types have a constant size. Now it's the caller's job to specify the length of the extensions that it might use. BUG= ========== to ========== Remove hardcoded kValueSizeBytes values from variable-length header extensions. Since the RtpStreamId and RepairedRtpStreamId extensions can have variable length, it makes no sense for them to have a constant valueSize field. The header length calculation in RtpHeaderExtensionMap needed to be changed for this because it previously worked with the assumption that all header types have a constant size. Now it's the caller's job to specify the length of the extensions that it might use. BUG=webrtc:7433 ==========
Nice solution! https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:62: // Returns the overhead, per packet, for FlexFEC. add description for the parameter. Can it have different values? should it? https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:131: struct RTPExtensionSizeInfo { may be RtpExtensionSize or RtpHeaderExtensionSize: RTP -> Rtp to follow style guide (even if lots of types around do not) drop Info because it doesn't seems to add descriptiveness and does look like help avoiding name conflict. https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:137: constexpr RTPExtensionSizeInfo CreateExtensionSizeInfo() { planning to use this function outside of the rtp_rtcp module? https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:48: constexpr RTPExtensionSizeInfo kExtensionSizes[] = { can you add a comment or change variable name to explain why only these extensions are selected. (e.g. why PlayoutDelay is included, but AudioLevel and VideoOrientation are not). https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:36: constexpr RTPExtensionSizeInfo kFecExtensionSizes[] = { how this list is different from kExtensionSizes in rtp_sender.cc ? may be add an accessor to RTPSender: static rtc::ArrayView<> ???ExtensionSizes();
https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:62: // Returns the overhead, per packet, for FlexFEC. On 2017/05/08 16:25:05, danilchap wrote: > add description for the parameter. > Can it have different values? should it? Done. https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:131: struct RTPExtensionSizeInfo { On 2017/05/08 16:25:06, danilchap wrote: > may be RtpExtensionSize or RtpHeaderExtensionSize: > RTP -> Rtp to follow style guide (even if lots of types around do not) > drop Info because it doesn't seems to add descriptiveness and does look like > help avoiding name conflict. Done. I saw RTP in a few places so I thought that was the norm. https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:137: constexpr RTPExtensionSizeInfo CreateExtensionSizeInfo() { On 2017/05/08 16:25:06, danilchap wrote: > planning to use this function outside of the rtp_rtcp module? I don't think so. This is used solely for the RTP extension overhead calculation, so it shouldn't really be needed in other modules. https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:48: constexpr RTPExtensionSizeInfo kExtensionSizes[] = { On 2017/05/08 16:25:06, danilchap wrote: > can you add a comment or change variable name to explain why only these > extensions are selected. (e.g. why PlayoutDelay is included, but AudioLevel and > VideoOrientation are not). Done. I've renamed this to show that it can be used for padding or FEC packets. https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:36: constexpr RTPExtensionSizeInfo kFecExtensionSizes[] = { On 2017/05/08 16:25:06, danilchap wrote: > how this list is different from kExtensionSizes in rtp_sender.cc ? > > may be add an accessor to RTPSender: > static rtc::ArrayView<> ???ExtensionSizes(); Done, I've added a getter for the FEC packets. Also, since RtpHeaderLength() might be used for other packets later on, I figured it'd make sense for it to have an |extension_sizes| arg instead of using kExtensionSizes directly.
https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:131: struct RTPExtensionSizeInfo { On 2017/05/09 11:40:03, erikvarga1 wrote: > On 2017/05/08 16:25:06, danilchap wrote: > > may be RtpExtensionSize or RtpHeaderExtensionSize: > > RTP -> Rtp to follow style guide (even if lots of types around do not) > > drop Info because it doesn't seems to add descriptiveness and does look like > > help avoiding name conflict. > > Done. I saw RTP in a few places so I thought that was the norm. May be hide this struct in less public header and move it closer to where it is used. e.g. to rtp_header_extension.h https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:137: constexpr RTPExtensionSizeInfo CreateExtensionSizeInfo() { On 2017/05/09 11:40:03, erikvarga1 wrote: > On 2017/05/08 16:25:06, danilchap wrote: > > planning to use this function outside of the rtp_rtcp module? > > I don't think so. This is used solely for the RTP extension overhead > calculation, so it shouldn't really be needed in other modules. Then do not declare it here: it seems you use it in rtp_sender.cc and in a test. Declare it just in rtp_sender.cc where you use it and in the test feel free to inline the body. https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:36: constexpr RTPExtensionSizeInfo kFecExtensionSizes[] = { On 2017/05/09 11:40:03, erikvarga1 wrote: > On 2017/05/08 16:25:06, danilchap wrote: > > how this list is different from kExtensionSizes in rtp_sender.cc ? > > > > may be add an accessor to RTPSender: > > static rtc::ArrayView<> ???ExtensionSizes(); > > Done, I've added a getter for the FEC packets. > > Also, since RtpHeaderLength() might be used for other packets later on, I > figured it'd make sense for it to have an |extension_sizes| arg instead of using > kExtensionSizes directly. Do you know such packets? It is generally better to make code simpler now and add complexity only when you about to use it. https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right): https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:133: virtual size_t MaxPayloadSize( This function look unused [and only slightly tested] - may be better delete it rather than complicate it. https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc (right): https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc:93: size_t total = 0; may be values_size better describes this variable https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc:95: if (ids_[extension.type] != kInvalidId) if (IsRegistred(extension.type)) looks more readable. https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc:96: total += extension.value_size + 1; add a comment or constant for + 1 https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:292: size_t RTPSender::MaxPayloadSize( if you agree to remove MaxPayloadSize from rtp_rtcp interface, then this function can be removed too and it's body incorporated into SendPadData function. It will be easier to reason about extension_sizes argument then. https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.h:20: #include "webrtc/base/constructormagic.h" #include "webrtc/base/array_view.h" https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.h:67: static rtc::ArrayView<const RtpExtensionSize> FecExtensionSizes(); may be declare it it near AllocatePacket function, since this return sizes for same extensions AllocatePacket aware about.
https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... File webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h (right): https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:131: struct RTPExtensionSizeInfo { On 2017/05/09 12:33:48, danilchap wrote: > On 2017/05/09 11:40:03, erikvarga1 wrote: > > On 2017/05/08 16:25:06, danilchap wrote: > > > may be RtpExtensionSize or RtpHeaderExtensionSize: > > > RTP -> Rtp to follow style guide (even if lots of types around do not) > > > drop Info because it doesn't seems to add descriptiveness and does look like > > > help avoiding name conflict. > > > > Done. I saw RTP in a few places so I thought that was the norm. > > May be hide this struct in less public header and move it closer to where it is > used. > e.g. to rtp_header_extension.h Done. https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/inclu... webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h:137: constexpr RTPExtensionSizeInfo CreateExtensionSizeInfo() { On 2017/05/09 12:33:48, danilchap wrote: > On 2017/05/09 11:40:03, erikvarga1 wrote: > > On 2017/05/08 16:25:06, danilchap wrote: > > > planning to use this function outside of the rtp_rtcp module? > > > > I don't think so. This is used solely for the RTP extension overhead > > calculation, so it shouldn't really be needed in other modules. > > Then do not declare it here: > it seems you use it in rtp_sender.cc and in a test. > Declare it just in rtp_sender.cc where you use it and in the test feel free to > inline the body. Done. https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2867713003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:36: constexpr RTPExtensionSizeInfo kFecExtensionSizes[] = { On 2017/05/09 12:33:49, danilchap wrote: > On 2017/05/09 11:40:03, erikvarga1 wrote: > > On 2017/05/08 16:25:06, danilchap wrote: > > > how this list is different from kExtensionSizes in rtp_sender.cc ? > > > > > > may be add an accessor to RTPSender: > > > static rtc::ArrayView<> ???ExtensionSizes(); > > > > Done, I've added a getter for the FEC packets. > > > > Also, since RtpHeaderLength() might be used for other packets later on, I > > figured it'd make sense for it to have an |extension_sizes| arg instead of > using > > kExtensionSizes directly. > > Do you know such packets? > It is generally better to make code simpler now and add complexity only when you > about to use it. Alright, since it's not used elsewhere, I guess it makes more sense to have the function work for the padding/fec packets only. I removed the extra arg and just added a comment instead. https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right): https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:133: virtual size_t MaxPayloadSize( On 2017/05/09 12:33:49, danilchap wrote: > This function look unused [and only slightly tested] - may be better delete it > rather than complicate it. Alright, I've removed it. https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc (right): https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc:93: size_t total = 0; On 2017/05/09 12:33:49, danilchap wrote: > may be values_size better describes this variable Done. https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc:95: if (ids_[extension.type] != kInvalidId) On 2017/05/09 12:33:49, danilchap wrote: > if (IsRegistred(extension.type)) > looks more readable. Done. https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc:96: total += extension.value_size + 1; On 2017/05/09 12:33:49, danilchap wrote: > add a comment or constant for + 1 Done. https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:292: size_t RTPSender::MaxPayloadSize( On 2017/05/09 12:33:49, danilchap wrote: > if you agree to remove MaxPayloadSize from rtp_rtcp interface, then this > function can be removed too and it's body incorporated into SendPadData > function. > It will be easier to reason about extension_sizes argument then. Done. https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.h:20: #include "webrtc/base/constructormagic.h" On 2017/05/09 12:33:49, danilchap wrote: > #include "webrtc/base/array_view.h" Oops, I forgot that. Done now. https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.h:67: static rtc::ArrayView<const RtpExtensionSize> FecExtensionSizes(); On 2017/05/09 12:33:49, danilchap wrote: > may be declare it it near AllocatePacket function, since this return sizes for > same extensions AllocatePacket aware about. Done. I thought for some reason that static functions needed to be placed at the top, but now that I checked, I don't see anything like that in the style guide.
lgtm % nits https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/2867713003/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.h:67: static rtc::ArrayView<const RtpExtensionSize> FecExtensionSizes(); On 2017/05/09 13:30:31, erikvarga1 wrote: > On 2017/05/09 12:33:49, danilchap wrote: > > may be declare it it near AllocatePacket function, since this return sizes for > > same extensions AllocatePacket aware about. > > Done. I thought for some reason that static functions needed to be placed at the > top, but now that I checked, I don't see anything like that in the style guide. Factory methods should be placed at the top (and factory methods are common reason to use static functions) https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/m... File webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h (right): https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/m... webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h:59: MOCK_CONST_METHOD1(MaxPayloadSize, remove https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc (right): https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc:93: static constexpr size_t kExtensionSizeOverhead = 1; may be kExtensionHeaderLength = 1 May be with so many headers involved add comment both for this one and for the constant above: // Header size of the extension block, See RFC3550 Section 5.3.1 // Header size of each individual extension. See RFC5285 Section 4.2 https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc:94: size_t value_size = 0; values_size (since it represent size of several values) https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:466: size_t max_payload_size; to stress this variable is not used after if-else block below, declare it inside each of the branches. if (audio) { size_t max_payload_size = ... } else { size_t max_payload_size = ... ... } https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:482: - video_->FecPacketOverhead() // FEC/ULP/RED overhead. next CL it would make sense to to remove this and next line: padding is not protected by fec, so this overhead is irrelevant... This CL keep old behaviour to be safer.
https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/m... File webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h (right): https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/m... webrtc/modules/rtp_rtcp/mocks/mock_rtp_rtcp.h:59: MOCK_CONST_METHOD1(MaxPayloadSize, On 2017/05/09 14:45:22, danilchap wrote: > remove Done. https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc (right): https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc:93: static constexpr size_t kExtensionSizeOverhead = 1; On 2017/05/09 14:45:22, danilchap wrote: > may be kExtensionHeaderLength = 1 > May be with so many headers involved add comment both for this one and for the > constant above: > // Header size of the extension block, See RFC3550 Section 5.3.1 > // Header size of each individual extension. See RFC5285 Section 4.2 Oh, right, that one byte is not only the size but also the ID. In that case ExtensionHeader does make more sense. Done. https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_header_extension.cc:94: size_t value_size = 0; On 2017/05/09 14:45:22, danilchap wrote: > values_size (since it represent size of several values) Ah, I missed that 's' in the previous comment. Done now. https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:466: size_t max_payload_size; On 2017/05/09 14:45:22, danilchap wrote: > to stress this variable is not used after if-else block below, declare it inside > each of the branches. > if (audio) { > size_t max_payload_size = > ... > } else { > size_t max_payload_size = ... > ... > } Done. https://codereview.webrtc.org/2867713003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:482: - video_->FecPacketOverhead() // FEC/ULP/RED overhead. On 2017/05/09 14:45:22, danilchap wrote: > next CL it would make sense to to remove this and next line: > padding is not protected by fec, so this overhead is irrelevant... > > This CL keep old behaviour to be safer. Acknowledged. I'll add a fix once this CL's submitted.
erikvarga@webrtc.org changed reviewers: - erikvarga@google.com
The CQ bit was checked by danilchap@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/23939)
brandtr@webrtc.org changed reviewers: + brandtr@webrtc.org
https://codereview.webrtc.org/2867713003/diff/60001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (left): https://codereview.webrtc.org/2867713003/diff/60001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:131: virtual size_t MaxPayloadSize() const = 0; I find the different "payload size"/MTU methods in the RTP module to be confusing, so it's good to see that they are getting fewer :) https://codereview.webrtc.org/2867713003/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_sender.cc (right): https://codereview.webrtc.org/2867713003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_sender.cc:152: return rtp_header_extension_map_.GetTotalLengthInBytes(extension_sizes) + I'm not familiar with the RID extension, but in this CL it looks like we don't have to add it to the FlexFEC packets? Currently, we only register BWE header extensions for FlexFEC: https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour... Since this method is always called with a fixed input argument (from rtp_sender_video.cc), can we move the call to rtp_header_extension_map_.GetTotalLengthInBytes(extension_sizes) to the ctor, and have this method return a const member variable + kFlexfecMaxHeaderSize?
https://codereview.webrtc.org/2867713003/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_sender.cc (right): https://codereview.webrtc.org/2867713003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_sender.cc:152: return rtp_header_extension_map_.GetTotalLengthInBytes(extension_sizes) + On 2017/05/10 11:12:37, brandtr wrote: > I'm not familiar with the RID extension, but in this CL it looks like we don't > have to add it to the FlexFEC packets? Currently, we only register BWE header > extensions for FlexFEC: > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour... > > Since this method is always called with a fixed input argument (from > rtp_sender_video.cc), can we move the call to > rtp_header_extension_map_.GetTotalLengthInBytes(extension_sizes) to the ctor, > and have this method return a const member variable + kFlexfecMaxHeaderSize? Yeah, stream IDs aren't used here (or in padding packets), so Flexfec always gets the same extension sizes. I've moved the call into the constructor and also moved the FecExtensionSizes() call to video_send_stream.cc (where the FlexfecSender is created). https://codereview.webrtc.org/2867713003/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2867713003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:174: return kExtensionSizes; This didn't compile for android, so I had to change it to use the (data, size) constructor instead.
https://codereview.webrtc.org/2867713003/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2867713003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:175: sizeof(kExtensionSizes)); sizeof would return size in bytes, but array view need number of elements you might want to use arraysize macro from "webrtc/base/arraysize.h" Before that add a test to make it fail.
https://codereview.webrtc.org/2867713003/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/flexfec_sender.cc (right): https://codereview.webrtc.org/2867713003/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/flexfec_sender.cc:152: return rtp_header_extension_map_.GetTotalLengthInBytes(extension_sizes) + On 2017/05/10 13:14:21, erikvarga1 wrote: > On 2017/05/10 11:12:37, brandtr wrote: > > I'm not familiar with the RID extension, but in this CL it looks like we don't > > have to add it to the FlexFEC packets? Currently, we only register BWE header > > extensions for FlexFEC: > > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour... > > > > Since this method is always called with a fixed input argument (from > > rtp_sender_video.cc), can we move the call to > > rtp_header_extension_map_.GetTotalLengthInBytes(extension_sizes) to the ctor, > > and have this method return a const member variable + kFlexfecMaxHeaderSize? > > Yeah, stream IDs aren't used here (or in padding packets), so Flexfec always > gets the same extension sizes. > I've moved the call into the constructor and also moved the FecExtensionSizes() > call to video_send_stream.cc (where the FlexfecSender is created). Thanks! https://codereview.webrtc.org/2867713003/diff/80001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.webrtc.org/2867713003/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:65: // to calculate the RTP header extension overhead and it should have an entry I think this comment is redundant now? https://codereview.webrtc.org/2867713003/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:87: size_t header_extensions_size_; Can you make this const?
https://codereview.webrtc.org/2867713003/diff/80001/webrtc/modules/rtp_rtcp/i... File webrtc/modules/rtp_rtcp/include/flexfec_sender.h (right): https://codereview.webrtc.org/2867713003/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:65: // to calculate the RTP header extension overhead and it should have an entry On 2017/05/10 14:36:27, brandtr wrote: > I think this comment is redundant now? Oops, I forgot to remove that. Removed now. https://codereview.webrtc.org/2867713003/diff/80001/webrtc/modules/rtp_rtcp/i... webrtc/modules/rtp_rtcp/include/flexfec_sender.h:87: size_t header_extensions_size_; On 2017/05/10 14:36:27, brandtr wrote: > Can you make this const? Done. https://codereview.webrtc.org/2867713003/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2867713003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:175: sizeof(kExtensionSizes)); On 2017/05/10 13:27:13, danilchap wrote: > sizeof would return size in bytes, but array view need number of elements > you might want to use arraysize macro from "webrtc/base/arraysize.h" > > Before that add a test to make it fail. Oh right, my bad. Fixed now. I've added a test to see how MaxPacketOverhead works when there are header extensions (and it fails when sizeof is used here).
https://codereview.webrtc.org/2867713003/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc (right): https://codereview.webrtc.org/2867713003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc:287: kRtpOneByteHeaderLength + 3*kExtensionHeaderLength); git cl format to add spaces round * https://codereview.webrtc.org/2867713003/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2867713003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:174: return rtc::ArrayView<const RtpExtensionSize>(kExtensionSizes, or (if it works) return rtc::MakeArrayView(kExtensionSizes, arraysize(kExtensionSizes); https://codereview.webrtc.org/2867713003/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/2867713003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.h:20: #include "webrtc/base/arraysize.h" include it where you use it (i.e. in .cc instead of .h)
lgtm. Thanks for making the changes to FlexfecSender! Also added a small suggestion. https://codereview.webrtc.org/2867713003/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc (right): https://codereview.webrtc.org/2867713003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc:287: kRtpOneByteHeaderLength + 3*kExtensionHeaderLength); On 2017/05/10 18:15:10, danilchap wrote: > git cl format > to add spaces round * Maybe put these constants in the order that they would appear in the actual header: kRtpOneByteHeaderLength + kExtensionHeaderLength + AbsoluteSendTime::kValueSizeBytes + kExtensionHeaderLength + TransmissionOffset::kValueSizeBytes + kExtensionHeaderLength + TransportSequenceNumber::kValueSizeBytes
https://codereview.webrtc.org/2867713003/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc (right): https://codereview.webrtc.org/2867713003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/flexfec_sender_unittest.cc:287: kRtpOneByteHeaderLength + 3*kExtensionHeaderLength); On 2017/05/10 19:23:06, brandtr wrote: > On 2017/05/10 18:15:10, danilchap wrote: > > git cl format > > to add spaces round * > > Maybe put these constants in the order that they would appear in the actual > header: > > kRtpOneByteHeaderLength + > kExtensionHeaderLength + AbsoluteSendTime::kValueSizeBytes + > kExtensionHeaderLength + TransmissionOffset::kValueSizeBytes + > kExtensionHeaderLength + TransportSequenceNumber::kValueSizeBytes Done. https://codereview.webrtc.org/2867713003/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2867713003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:174: return rtc::ArrayView<const RtpExtensionSize>(kExtensionSizes, On 2017/05/10 18:15:10, danilchap wrote: > or (if it works) > return rtc::MakeArrayView(kExtensionSizes, arraysize(kExtensionSizes); Done, it seems to work (when compiling locally for android). https://codereview.webrtc.org/2867713003/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.h (right): https://codereview.webrtc.org/2867713003/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.h:20: #include "webrtc/base/arraysize.h" On 2017/05/10 18:15:10, danilchap wrote: > include it where you use it (i.e. in .cc instead of .h) Done.
The CQ bit was checked by erikvarga@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by erikvarga@webrtc.org
The CQ bit was checked by erikvarga@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
danilchap@webrtc.org changed reviewers: + stefan@webrtc.org
Stefan, can you take a quick look at webrtc/test/fuzzers
https://codereview.webrtc.org/2867713003/diff/140001/webrtc/test/fuzzers/flex... File webrtc/test/fuzzers/flexfec_sender_fuzzer.cc (right): https://codereview.webrtc.org/2867713003/diff/140001/webrtc/test/fuzzers/flex... webrtc/test/fuzzers/flexfec_sender_fuzzer.cc:39: kNoRtpHeaderExtensions, kNoRtpHeaderExtensionSizes, I've added the missing new param to the FlexfecSender constructor here. The fuzzer tests should hopefully be OK after this.
On 2017/05/11 12:55:28, danilchap wrote: > Stefan, can you take a quick look at webrtc/test/fuzzers Stefan is OOO until Monday.
danilchap@webrtc.org changed reviewers: + mflodman@webrtc.org - stefan@webrtc.org
Magnus, do you mind approving small change in test/fuzzers ? (side note: may be time to extend fuzzers/OWNERS)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
danilchap@webrtc.org changed reviewers: + stefan@webrtc.org - mflodman@webrtc.org
holmer@google.com changed reviewers: + holmer@google.com
lgtm for webrtc/fuzzers
The CQ bit was checked by erikvarga@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org, brandtr@webrtc.org Link to the patchset: https://codereview.webrtc.org/2867713003/#ps140001 (title: "Patch 7")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17120)
lgtm
The CQ bit was checked by erikvarga@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495022169541070,
"parent_rev": "b30843a2faae8975b2d9f6706f9013327b169948", "commit_rev":
"2788373528ec05ccb3e3107ee67fb12819011f01"}
Message was sent while issue was closed.
Description was changed from ========== Remove hardcoded kValueSizeBytes values from variable-length header extensions. Since the RtpStreamId and RepairedRtpStreamId extensions can have variable length, it makes no sense for them to have a constant valueSize field. The header length calculation in RtpHeaderExtensionMap needed to be changed for this because it previously worked with the assumption that all header types have a constant size. Now it's the caller's job to specify the length of the extensions that it might use. BUG=webrtc:7433 ========== to ========== Remove hardcoded kValueSizeBytes values from variable-length header extensions. Since the RtpStreamId and RepairedRtpStreamId extensions can have variable length, it makes no sense for them to have a constant valueSize field. The header length calculation in RtpHeaderExtensionMap needed to be changed for this because it previously worked with the assumption that all header types have a constant size. Now it's the caller's job to specify the length of the extensions that it might use. BUG=webrtc:7433 Review-Url: https://codereview.webrtc.org/2867713003 Cr-Commit-Position: refs/heads/master@{#18179} Committed: https://chromium.googlesource.com/external/webrtc/+/2788373528ec05ccb3e3107ee... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/external/webrtc/+/2788373528ec05ccb3e3107ee... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
