Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2766323006:
Correcting the amount of padding when send side bwe includes RTP overhead.
|
Created:
3 years, 9 months ago by minyue-webrtc Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionCorrecting the amount of padding when send side bwe includes RTP overhead.
BUG=webrtc:7418
Patch Set 1 #Patch Set 2 : another find #
Total comments: 12
Patch Set 3 : update #
Total comments: 6
Patch Set 4 : fixing #
Total comments: 6
Patch Set 5 : Fix and unittests #
Total comments: 10
Patch Set 6 : on comments` #
Total comments: 4
Patch Set 7 : fixing #
Total comments: 3
Patch Set 8 : rebasing #
Messages
Total messages: 30 (5 generated)
minyue@webrtc.org changed reviewers: + michaelt@webrtc.org, philipel@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc (right): https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc:203: // TODO(minyue): it looks like that the fitting expects |size| to include Philip and Stefan, would you take a look at this?
https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:468: bytes_left -= packet->headers_size(); packet was moved, so you should not use it. save the correct size before sending: size_t bytes_used = send_side_bwe_with_overhead_ ? packet->size() : packet->payload_size(); if (!PrepareAndSend(std::move(packet)) break; bytes_left -= bytes_used; https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:588: bytes_sent += padding_packet.headers_size(); may be if else is better: if (with_overhead_) { bytes_sent += padding_packet.size(); } else { bytes_sent += padding_bytes_in_packet or padding_packet.padding_size(); }
minyue@webrtc.org changed reviewers: + danilchap@webrtc.org
Hi Danil, Thanks for the comments! If you know the system well, I'd add you to review the basic idea. I am not 100% sure myself that current solution is complete and totally correct. https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:468: bytes_left -= packet->headers_size(); On 2017/03/24 08:47:20, danilchap wrote: > packet was moved, so you should not use it. > save the correct size before sending: > size_t bytes_used = send_side_bwe_with_overhead_ ? packet->size() : > packet->payload_size(); > if (!PrepareAndSend(std::move(packet)) > break; > bytes_left -= bytes_used; Thanks! of course https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:588: bytes_sent += padding_packet.headers_size(); On 2017/03/24 08:47:20, danilchap wrote: > may be if else is better: > if (with_overhead_) { > bytes_sent += padding_packet.size(); > } else { > bytes_sent += padding_bytes_in_packet or padding_packet.padding_size(); > } I like that too.
https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:574: padding_packet.SetPadding(padding_bytes_in_packet, &random_); Is this really correct? When you call SendPadData with the experiment on you expect to send |bytes| number of bytes, including the header. So I think you need to subtract the size of the header from |padding_bytes_in_packet| to not send |bytes| + <header size> number of bytes.
https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:574: padding_packet.SetPadding(padding_bytes_in_packet, &random_); On 2017/03/27 10:55:38, philipel wrote: > Is this really correct? When you call SendPadData with the experiment on you > expect to send |bytes| number of bytes, including the header. So I think you > need to subtract the size of the header from |padding_bytes_in_packet| to not > send |bytes| + <header size> number of bytes. I think this is correct: here you set number of padding bytes per packet, which is normally maximum padding size. you want to send total size 'bytes', thus bytes_sent need to be updated correctly to terminate "while (bytes_sent < bytes)" loop earlier.
https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:574: padding_packet.SetPadding(padding_bytes_in_packet, &random_); On 2017/03/27 11:20:23, danilchap wrote: > On 2017/03/27 10:55:38, philipel wrote: > > Is this really correct? When you call SendPadData with the experiment on you > > expect to send |bytes| number of bytes, including the header. So I think you > > need to subtract the size of the header from |padding_bytes_in_packet| to not > > send |bytes| + <header size> number of bytes. > > I think this is correct: > here you set number of padding bytes per packet, which is normally maximum > padding size. > you want to send total size 'bytes', thus bytes_sent need to be updated > correctly to terminate "while (bytes_sent < bytes)" loop earlier. I agree that we want to terminate the loop as early as possible by maximizing the size of the padding packets. But in the case of sending say 100 bytes? Then we would create a packet of 100 bytes padding + <header size>, which exceeds the number of bytes we want to send. I think this might be important in the case of audio BWE since the bitrates we work with are so low (which is why the overhead of headers are important to fix in the first place I guess?).
https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:574: padding_packet.SetPadding(padding_bytes_in_packet, &random_); On 2017/03/27 11:38:49, philipel wrote: > On 2017/03/27 11:20:23, danilchap wrote: > > On 2017/03/27 10:55:38, philipel wrote: > > > Is this really correct? When you call SendPadData with the experiment on you > > > expect to send |bytes| number of bytes, including the header. So I think you > > > need to subtract the size of the header from |padding_bytes_in_packet| to > not > > > send |bytes| + <header size> number of bytes. > > > > I think this is correct: > > here you set number of padding bytes per packet, which is normally maximum > > padding size. > > you want to send total size 'bytes', thus bytes_sent need to be updated > > correctly to terminate "while (bytes_sent < bytes)" loop earlier. > > I agree that we want to terminate the loop as early as possible by maximizing > the size of the padding packets. But in the case of sending say 100 bytes? Then > we would create a packet of 100 bytes padding + <header size>, which exceeds the > number of bytes we want to send. > > I think this might be important in the case of audio BWE since the bitrates we > work with are so low (which is why the overhead of headers are important to fix > in the first place I guess?). sorry, I forgout about audio case where we want smaller packets. Yes, it is incorrect for audio then. padding_bytes_in_packet should be calculated with header size in mind, likely here instead of top of this function.
https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc (right): https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc:203: // TODO(minyue): it looks like that the fitting expects |size| to include Looks wrong to me as well. https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:479: padding_bytes_in_packet = Shouldn't we calculate padding_bytes_in_packet inside the loop ? https://codereview.webrtc.org/2766323006/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:574: padding_packet.SetPadding(padding_bytes_in_packet, &random_); On 2017/03/27 10:55:38, philipel wrote: > Is this really correct? When you call SendPadData with the experiment on you > expect to send |bytes| number of bytes, including the header. So I think you > need to subtract the size of the header from |padding_bytes_in_packet| to not > send |bytes| + <header size> number of bytes. Acknowledged.
I have updated this. I may continue to add a test in RtpSender but I am not sure how to do it nicely. So please take a look at what I have got so far.
https://codereview.webrtc.org/2766323006/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc (right): https://codereview.webrtc.org/2766323006/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc:230: MOCK_CONST_METHOD0(size, size_t()); I'm not sure it is good idea to make cheap accessor virtual, specially just for tests. (it also breaks invariant size() = header_size() + payload_size() + padding_size()) May be rather create helper method that use extra knowledge about RtpPacket to create packet of given size: std::unique_ptr<RtpPacketToSend> CreatePacket(size_t size) { std::unique_ptr<RtpPacketToSend> packet(new RtpPacketToSend(nullptr, size)); packet->AllocatePayload(size - packet->header_size()); EXPECT_EQ(packet->size(), size); return packet; } This will only limit to header_size of 12 bytes (minimum header size), but that should be enough for test below. It is possible to extend this function a bit to have custome header_size if you prefer to be explicit (as long as they are >= 12 and multiple of 4) https://codereview.webrtc.org/2766323006/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc:270: hist_.GetBestFittingPacket(kMinPacketRequestBytes, true); since last parameter of the GetBestFittingPacket matter for this test, put a comment what it means, with an extra variable: const bool kIncludeHeader = true; or with a comment: , true /* include_header */) https://codereview.webrtc.org/2766323006/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:467: : packet->payload_size() - packet->headers_size(); prefer |packet->size() - packet.header_size()| (or |packet->payload_size() + packet->padding_size()| or, assuming padding size is 0, just |packet->payload_size()|) https://codereview.webrtc.org/2766323006/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:593: bytes_sent += send_side_bwe_with_overhead_ ? padding_packet.padding_size() other way around: .size() when overhead is included and padding_size() when not.
Thanks for the comments! They are addressed so far. Regarding unittests, I would like to land https://codereview.webrtc.org/2783743003/ first. Then the rebasing of this CL on that one will trigger updates on unittests. https://codereview.webrtc.org/2766323006/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc (right): https://codereview.webrtc.org/2766323006/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc:230: MOCK_CONST_METHOD0(size, size_t()); On 2017/03/28 15:10:58, danilchap wrote: > I'm not sure it is good idea to make cheap accessor virtual, specially just for > tests. (it also breaks invariant > size() = header_size() + payload_size() + padding_size()) > > May be rather create helper method that use extra knowledge about RtpPacket to > create packet of given size: > > std::unique_ptr<RtpPacketToSend> CreatePacket(size_t size) { > std::unique_ptr<RtpPacketToSend> packet(new RtpPacketToSend(nullptr, size)); > packet->AllocatePayload(size - packet->header_size()); > EXPECT_EQ(packet->size(), size); > return packet; > } > > This will only limit to header_size of 12 bytes (minimum header size), but that > should be enough for test below. > It is possible to extend this function a bit to have custome header_size if you > prefer to be explicit (as long as they are >= 12 and multiple of 4) Fully agreed. I think const header size if enough for the test. So it will be easy. Just one comment, using the real RTP packet is a bit painful since it is not easy to know the implementations there, e.g., only AllocatePayload can work, even SetPayloadSize(), which sounds better, does not.
https://codereview.webrtc.org/2766323006/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc (right): https://codereview.webrtc.org/2766323006/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc:230: MOCK_CONST_METHOD0(size, size_t()); On 2017/03/29 11:39:44, minyue-webrtc wrote: > On 2017/03/28 15:10:58, danilchap wrote: > > I'm not sure it is good idea to make cheap accessor virtual, specially just > for > > tests. (it also breaks invariant > > size() = header_size() + payload_size() + padding_size()) > > > > May be rather create helper method that use extra knowledge about RtpPacket to > > create packet of given size: > > > > std::unique_ptr<RtpPacketToSend> CreatePacket(size_t size) { > > std::unique_ptr<RtpPacketToSend> packet(new RtpPacketToSend(nullptr, size)); > > packet->AllocatePayload(size - packet->header_size()); > > EXPECT_EQ(packet->size(), size); > > return packet; > > } > > > > This will only limit to header_size of 12 bytes (minimum header size), but > that > > should be enough for test below. > > It is possible to extend this function a bit to have custome header_size if > you > > prefer to be explicit (as long as they are >= 12 and multiple of 4) > > Fully agreed. I think const header size if enough for the test. So it will be > easy. > > Just one comment, using the real RTP packet is a bit painful since it is not > easy to know the implementations there, e.g., only AllocatePayload can work, > even SetPayloadSize(), which sounds better, does not. I'm sorry for making interface of the rtp packet confusing. AllocatePayload can increase payload size SetPayloadSize limited to only reduce payload size. Probably I should merge these two function into one to make it less confusing.
https://codereview.webrtc.org/2766323006/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:467: : packet->payload_size() - packet->headers_size(); Why "payload_size - headers_size", i don't understand this part. https://codereview.webrtc.org/2766323006/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:497: while (bytes_sent < bytes) { if we define packet size before the loop. Will not the last packet be to big ?
I was totally stupid yesterday. See the new patch (PS5). and unittests are added (updated). Note that the patch set is rebased on https://codereview.webrtc.org/2783743003/. Now it is ready to go % a concern micheal brings up. https://codereview.webrtc.org/2766323006/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:467: : packet->payload_size() - packet->headers_size(); On 2017/03/29 14:45:21, michaelt wrote: > Why "payload_size - headers_size", i don't understand this part. neither do I understand it :) will fix. Thanks. https://codereview.webrtc.org/2766323006/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:497: while (bytes_sent < bytes) { On 2017/03/29 14:45:21, michaelt wrote: > if we define packet size before the loop. Will not the last packet be to big ? I have not looked into this. will do
https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:54: virtual size_t headers_size() const; remove virtual https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:64: virtual size_t size() const; remove virtual https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet_history.h (right): https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet_history.h:52: RTC_DEPRECATED std::unique_ptr<RtpPacketToSend> GetBestFittingPacket( Since the long term goal is to include all headers in the BWE I think this function should not be deprecated, but the overloaded one instead.
https://codereview.webrtc.org/2766323006/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:497: while (bytes_sent < bytes) { On 2017/03/29 18:52:33, minyue-webrtc wrote: > On 2017/03/29 14:45:21, michaelt wrote: > > if we define packet size before the loop. Will not the last packet be to big ? > > > I have not looked into this. will do Might not be important (for same reason padding might overshoot in video case): it is better to send one big packet than two small, so it is ok to overshoot a bit - next call to padding with request less of it. https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet_history.h (right): https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet_history.h:57: std::unique_ptr<RtpPacketToSend> GetBestFittingPacket( can you add a comment what |include_header| mean in this case. (while it is obvious in context of this CL, it doesn't look obvious without it) https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc (right): https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc:232: packet->AllocatePayload(kMinPacketRequestBytes - header_size); SetPayloadSize should be usable if you rebase. As you mention, that name make more sense. https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:819: kMaxPaddingSize + rtp_header_len * 2 can you add a comment why * 2?
https://codereview.webrtc.org/2766323006/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:497: while (bytes_sent < bytes) { On 2017/03/30 11:58:08, danilchap wrote: > On 2017/03/29 18:52:33, minyue-webrtc wrote: > > On 2017/03/29 14:45:21, michaelt wrote: > > > if we define packet size before the loop. Will not the last packet be to big > ? > > > > > > I have not looked into this. will do > > Might not be important (for same reason padding might overshoot in video case): > it is better to send one big packet than two small, so it is ok to overshoot a > bit - next call to padding with request less of it. This is exactly why we do it this way. We want to avoid sending many small packets and prefer temporary overshoots to ensure we have a lower packet rate.
Thanks for the comments! please take a look at PS6. https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:54: virtual size_t headers_size() const; On 2017/03/30 11:52:11, philipel wrote: > remove virtual oh, I forgot https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:64: virtual size_t size() const; On 2017/03/30 11:52:11, philipel wrote: > remove virtual Done. https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet_history.h (right): https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet_history.h:52: RTC_DEPRECATED std::unique_ptr<RtpPacketToSend> GetBestFittingPacket( On 2017/03/30 11:52:11, philipel wrote: > Since the long term goal is to include all headers in the BWE I think this > function should not be deprecated, but the overloaded one instead. good point. But we cannot use RTC_DEPRECATED, I added a TODO instead. https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/2766323006/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:819: kMaxPaddingSize + rtp_header_len * 2 On 2017/03/30 11:58:08, danilchap wrote: > can you add a comment why * 2? because two RTP packets are created.
lgtm I do feel the SendPadData function could be cleaned up a bit, but if we do that then lets do it in another CL.
lgtm
lgtm
Can you make it clear what overhead means in the CL description and add a bug? Does overhead mean RTP headers or is it all from IP to RTP? https://codereview.webrtc.org/2766323006/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:593: : padding_packet.padding_size(); Maybe for consistency use: packet->size() - packet->headers_size()?
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
Description was changed from ========== Correcting the amount of padding when send side bwe includes overhead. BUG= ========== to ========== Correcting the amount of padding when send side bwe includes RTP overhead. BUG=webrtc:7418 ==========
After running video_loopback which I had to fix, I found two additional places that needs with_overhead treatment, plus a bug (int - int -> size_t can wrap around). I would call for another review. https://codereview.webrtc.org/2766323006/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:593: : padding_packet.padding_size(); On 2017/04/04 08:13:38, stefan-webrtc wrote: > Maybe for consistency use: packet->size() - packet->headers_size()? I would rather do padding_size + headers_size I hope to avoid using size(), everywhere if possible, since it is not clear by its name. https://codereview.webrtc.org/2766323006/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:189: return rtp_header_extension_map_.RegisterByType(id, type) ? 0 : -1; sorry, this is due to rebasing https://codereview.webrtc.org/2766323006/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:478: ? std::max<int>(0, bytes - RtpHeaderLength()) This was wrong, since bytes - RtpHeaderLength() can be negative https://codereview.webrtc.org/2766323006/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1266: const size_t packet_size = this is a simple refactoring
still lgtm https://codereview.webrtc.org/2766323006/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:593: : padding_packet.padding_size(); On 2017/04/11 09:35:14, minyue-webrtc wrote: > On 2017/04/04 08:13:38, stefan-webrtc wrote: > > Maybe for consistency use: packet->size() - packet->headers_size()? > > I would rather do padding_size + headers_size > > I hope to avoid using size(), everywhere if possible, since it is not clear by > its name. personally prefer size() function too: size_t packet_size = packet->size(); But may be because I remember what that function do. [full size of the packet RTPSender aware of, i.e. include rtp overhead, exclude udp/crypto overheads].
https://codereview.webrtc.org/2766323006/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2766323006/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:593: : padding_packet.padding_size(); On 2017/04/11 14:25:18, danilchap wrote: > On 2017/04/11 09:35:14, minyue-webrtc wrote: > > On 2017/04/04 08:13:38, stefan-webrtc wrote: > > > Maybe for consistency use: packet->size() - packet->headers_size()? > > > > I would rather do padding_size + headers_size > > > > I hope to avoid using size(), everywhere if possible, since it is not clear by > > its name. > > personally prefer size() function too: > size_t packet_size = packet->size(); > > But may be because I remember what that function do. [full size of the packet > RTPSender aware of, i.e. include rtp overhead, exclude udp/crypto overheads]. You are right here. I realized that we should include transport overhead here as well. Per offline discussion with Stefan, I may do it in a follow up CL, since current change is already an improvement, |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
