Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 5 months ago by danilchap Modified:
3 years, 4 months ago Reviewers:
eladalon CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRefactor composing report blocks for rtcp Sender/Receiver reports.
Compose them while creating sr/rr instead of presaving in temporary
member variable
BUG=webrtc:5565, webrtc:8016
Review-Url: https://codereview.webrtc.org/2979413002
Cr-Commit-Position: refs/heads/master@{#19138}
Committed: https://chromium.googlesource.com/external/webrtc/+/96b69bdbee32062c199c31859f833f2761b89567
Patch Set 1 #
Total comments: 22
Patch Set 2 : Addressing feedback #Patch Set 3 : Move changes to rtcp_packet into own CL #
Total comments: 2
Created: 3 years, 5 months ago
Messages
Total messages: 22 (14 generated)
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/...
Description was changed from ========== Refactor composing report blocks in rtcp Sender/Receiver reports compose them while creating sr/rr instead of presaving in temporary member variabe BUG=webrtc:5565, webrtc:8016 ========== to ========== Refactor composing report blocks in rtcp Sender/Receiver reports. Compose them while creating sr/rr instead of presaving in temporary member variable BUG=webrtc:5565, webrtc:8016 ==========
danilchap@webrtc.org changed reviewers: + eladalon@webrtc.org
Small comments for the easily reviewed part; I'll look at the rest more in depth separately. https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report.cc (right): https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report.cc:98: bool ReceiverReport::SetReportBlocks(std::vector<ReportBlock> blocks) { Not sure about this - what do you think of accepting (std::vector<ReportBlock>&& blocks) instead? https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report.cc:100: LOG(LS_WARNING) << "Max report blocks reached."; Maybe log blocks.size() https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc (right): https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:130: TEST(RtcpPacketReceiverReportTest, SetReportBlocksOverwritesOldBlocks) { nit: Checking by size only does not guarantee correctness completely. Suppose we start with [1], and try to set([5, 6, 7]), I can imagine code that would accidentally end up with [1, 6, 7], which would still pass the test. IMHO, it wouldn't be overkill to check the members, too. Or is it difficult to do comparison of the specific elements there? https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:145: EXPECT_TRUE(rr.SetReportBlocks(blocks)); nit: Two tests here: 1. Max number of blocks accepted. 2. One more than max - rejected. https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc (right): https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc:127: bool SenderReport::SetReportBlocks(std::vector<ReportBlock> blocks) { Similar comments for this file and the next one as for the Receiver case.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:839: for (auto& statistician : receive_statistics_->GetActiveStatisticians()) { Maybe: auto name_me = receive_statistics_->GetActiveStatisticians(); result.reserve(name_me.size()); for (auto& statistician : name_me) { ? https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:842: if (!statistician.second->GetStatistics(&stats, true)) nits: 1. Inside of ReceiveStatisticsImpl::GetActiveStatisticians(), I see the following lines: rtc::CritScope cs(&receive_statistics_lock_); StatisticianMap active_statisticians; We can probably reverse their order. 2. While doing my due dilligence and making sure that statistician.second is guaranteed non-null, I saw this: if (/* some conditional*/) { impl = unrelated; } else { impl = new StreamStatisticianImpl(clock_, this, this); statisticians_[header.ssrc] = impl; } ... impl->IncomingPacket(header, packet_length, retransmitted); So if |new| fails and we have a nullptr there, we're guatanteed to crash very soon after. However, we won't know for sure where it's from the |new| or from |unrelated|. Perhaps an RTC_CHECK is in order after the |new|, but not here? Wdyt? https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:849: continue; When I read this, I wonder if we should |continue| or |break|. The reason to not break is probably that you still want to reset the other statisticians? If so, perhaps we should comment about it? (Another thing I ask myself is if we want to produce multiple warning logs when we continue. If we don't change to |break|, we can still probably have a boolean local to the function that makes sure we only log once? Or would that be overkill?) https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:861: result.push_back(block); Brainstorming - what do you think about constructing the new ReportBlock immediately in the vector, then having |block| be a reference to that new object, so that we wouldn't have to perform copying here, but would rather be performing all of the previous calls to Set*() on the relevant object from the get-go?
https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report.cc (right): https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report.cc:98: bool ReceiverReport::SetReportBlocks(std::vector<ReportBlock> blocks) { On 2017/07/21 12:53:19, eladalon wrote: > Not sure about this - what do you think of accepting (std::vector<ReportBlock>&& > blocks) instead? that is banned by style guide. https://google.github.io/styleguide/cppguide.html#Rvalue_references T&& in a function signature means 'may be move' here it move parameter almost always so prefer a simpler signature. https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report.cc:100: LOG(LS_WARNING) << "Max report blocks reached."; On 2017/07/21 12:53:19, eladalon wrote: > Maybe log blocks.size() Done. https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc (right): https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:130: TEST(RtcpPacketReceiverReportTest, SetReportBlocksOverwritesOldBlocks) { On 2017/07/21 12:53:19, eladalon wrote: > nit: Checking by size only does not guarantee correctness completely. Suppose we > start with [1], and try to set([5, 6, 7]), I can imagine code that would > accidentally end up with [1, 6, 7], which would still pass the test. IMHO, it > wouldn't be overkill to check the members, too. Or is it difficult to do > comparison of the specific elements there? I lack imagination to see that code, but it's not too hard to check some values in report blocks. Done. https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/receiver_report_unittest.cc:145: EXPECT_TRUE(rr.SetReportBlocks(blocks)); On 2017/07/21 12:53:19, eladalon wrote: > nit: Two tests here: > 1. Max number of blocks accepted. > 2. One more than max - rejected. renamed to document this test is about showing the limit. https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc (right): https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/sender_report.cc:127: bool SenderReport::SetReportBlocks(std::vector<ReportBlock> blocks) { On 2017/07/21 12:53:19, eladalon wrote: > Similar comments for this file and the next one as for the Receiver case. Done. https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:839: for (auto& statistician : receive_statistics_->GetActiveStatisticians()) { On 2017/07/21 13:34:45, eladalon wrote: > Maybe: > auto name_me = receive_statistics_->GetActiveStatisticians(); > result.reserve(name_me.size()); > for (auto& statistician : name_me) { > ? Done, that is an optimization and closer to the original code. https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:842: if (!statistician.second->GetStatistics(&stats, true)) This CL is about refactoring rtcp_sender, would prefer to review ReceiveStatistics in later CL. (this version does as little checks as previous version). On 2017/07/21 13:34:45, eladalon wrote: > nits: > > 1. > Inside of ReceiveStatisticsImpl::GetActiveStatisticians(), I see the following > lines: > rtc::CritScope cs(&receive_statistics_lock_); > StatisticianMap active_statisticians; > We can probably reverse their order. yep, it won't hurt. > 2. > While doing my due dilligence and making sure that statistician.second is > guaranteed non-null, I saw this: > if (/* some conditional*/) { > impl = unrelated; > } else { > impl = new StreamStatisticianImpl(clock_, this, this); > statisticians_[header.ssrc] = impl; > } > ... > impl->IncomingPacket(header, packet_length, retransmitted); > So if |new| fails and we have a nullptr there, we're guatanteed to crash very > soon after. However, we won't know for sure where it's from the |new| or from > |unrelated|. Perhaps an RTC_CHECK is in order after the |new|, but not here? > Wdyt? I assume new can't return nullptr (I expect out of memory scenario will terminate the program) https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:849: continue; On 2017/07/21 13:34:45, eladalon wrote: > When I read this, I wonder if we should |continue| or |break|. The reason to not > break is probably that you still want to reset the other statisticians? If so, > perhaps we should comment about it? > > (Another thing I ask myself is if we want to produce multiple warning logs when > we continue. If we don't change to |break|, we can still probably have a boolean > local to the function that makes sure we only log once? Or would that be > overkill?) I initially put more reasonable break, but then switch to continue just to keep old behaviour. for future I put a TODO hinting that the way statistics used now, it shouldn't be possible to go over the limit, so for now I care little what will happen with too many report blocks. e.g. Do not mind to switch back to break if you think it would be nicer. https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:861: result.push_back(block); On 2017/07/21 13:34:45, eladalon wrote: > Brainstorming - what do you think about constructing the new ReportBlock > immediately in the vector, then having |block| be a reference to that new > object, so that we wouldn't have to perform copying here, but would rather be > performing all of the previous calls to Set*() on the relevant object from the > get-go? Done.
Description was changed from ========== Refactor composing report blocks in rtcp Sender/Receiver reports. Compose them while creating sr/rr instead of presaving in temporary member variable BUG=webrtc:5565, webrtc:8016 ========== to ========== Refactor composing report blocks for rtcp Sender/Receiver reports. Compose them while creating sr/rr instead of presaving in temporary member variable BUG=webrtc:5565, webrtc:8016 ==========
lgtm https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:842: if (!statistician.second->GetStatistics(&stats, true)) 1. Yes, I didn't intend for this to be done in this CL; just something I came across because I was reviewing this CL, so I mentioned it here. 2. Let me push a CL for that, then. 3. Do we always crash when OOM? Do all builds of WebRTC do that? https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:849: continue; On 2017/07/21 17:11:14, danilchap wrote: > On 2017/07/21 13:34:45, eladalon wrote: > > When I read this, I wonder if we should |continue| or |break|. The reason to > not > > break is probably that you still want to reset the other statisticians? If so, > > perhaps we should comment about it? > > > > (Another thing I ask myself is if we want to produce multiple warning logs > when > > we continue. If we don't change to |break|, we can still probably have a > boolean > > local to the function that makes sure we only log once? Or would that be > > overkill?) > > I initially put more reasonable break, but then switch to continue just to keep > old behaviour. > > for future I put a TODO hinting that the way statistics used now, it shouldn't > be possible to go over the limit, so for now I care little what will happen with > too many report blocks. > e.g. Do not mind to switch back to break if you think it would be nicer. Due to my lack of context, I'm okay with either breaking/continuing, but I would suggest a comment (in addition to the TODO), attached to that specific line (not the if-statement), that would show the reader that the behavior (resetting the other statisticians or avoiding doing so) was intentional, and preferably also explain the rationale (I would personally like to know which one is the right behavior). I wonder if, should this happen, we'd find ourselves (almost) always failing to send report blocks for the statisticians that appear later in the list, by the way? But maybe this is all theoretical, because it's not ever supposed to happen anyway? https://codereview.webrtc.org/2979413002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/2979413002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:854: rtcp::ReportBlock& block = result.back(); It would probably be overkill to make sure the emplace_back didn't fail due to OOM, right?
https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:842: if (!statistician.second->GetStatistics(&stats, true)) On 2017/07/25 12:19:20, eladalon wrote: > Do we always crash when OOM? Do all builds of WebRTC do that? That is my assumption since I never seen code in webrtc that tries to recover from oom. https://codereview.webrtc.org/2979413002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:849: continue; On 2017/07/25 12:19:20, eladalon wrote: > On 2017/07/21 17:11:14, danilchap wrote: > > On 2017/07/21 13:34:45, eladalon wrote: > > > When I read this, I wonder if we should |continue| or |break|. The reason to > > not > > > break is probably that you still want to reset the other statisticians? If > so, > > > perhaps we should comment about it? > > > > > > (Another thing I ask myself is if we want to produce multiple warning logs > > when > > > we continue. If we don't change to |break|, we can still probably have a > > boolean > > > local to the function that makes sure we only log once? Or would that be > > > overkill?) > > > > I initially put more reasonable break, but then switch to continue just to > keep > > old behaviour. > > > > for future I put a TODO hinting that the way statistics used now, it shouldn't > > be possible to go over the limit, so for now I care little what will happen > with > > too many report blocks. > > e.g. Do not mind to switch back to break if you think it would be nicer. > > Due to my lack of context, I'm okay with either breaking/continuing, but I would > suggest a comment (in addition to the TODO), attached to that specific line (not > the if-statement), that would show the reader that the behavior (resetting the > other statisticians or avoiding doing so) was intentional, and preferably also > explain the rationale (I would personally like to know which one is the right > behavior). > > I wonder if, should this happen, we'd find ourselves (almost) always failing to > send report blocks for the statisticians that appear later in the list, by the > way? But maybe this is all theoretical, because it's not ever supposed to happen > anyway? Next CL (early draft is https://codereview.webrtc.org/2981163003/) I plan to move half of this function inside ReceiveStatistics itself. Then more expensive 'continue' will be simpler to implement than cheaper 'break'. In practice it shouldn't matter for now. yes, if this should happen, we'll fail to send report blocks to unlucky statisticians with higher ssrc. Thus todo to propertly support it (spec do describe several ways how to handle more than 31 report blocks) https://codereview.webrtc.org/2979413002/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/2979413002/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:854: rtcp::ReportBlock& block = result.back(); On 2017/07/25 12:19:20, eladalon wrote: > It would probably be overkill to make sure the emplace_back didn't fail due to > OOM, right? yep, specially that emplace_back has no way to report an error (other than with an exception that shouldn't be catched anyway)
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: This issue passed the CQ dry run.
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/...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1500999146821030,
"parent_rev": "7fb11d737661f5b8c00baeab278e9f08c64a1be1", "commit_rev":
"96b69bdbee32062c199c31859f833f2761b89567"}
Message was sent while issue was closed.
Description was changed from ========== Refactor composing report blocks for rtcp Sender/Receiver reports. Compose them while creating sr/rr instead of presaving in temporary member variable BUG=webrtc:5565, webrtc:8016 ========== to ========== Refactor composing report blocks for rtcp Sender/Receiver reports. Compose them while creating sr/rr instead of presaving in temporary member variable BUG=webrtc:5565, webrtc:8016 Review-Url: https://codereview.webrtc.org/2979413002 Cr-Commit-Position: refs/heads/master@{#19138} Committed: https://chromium.googlesource.com/external/webrtc/+/96b69bdbee32062c199c31859... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/96b69bdbee32062c199c31859... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
