Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1989363006:
TMMBRHelp::FindBoundingSet function cleaned (Closed)
|
Created:
4 years, 7 months ago by danilchap Modified:
4 years, 4 months ago Reviewers:
philipel CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionTMMBRHelp::FindBoundingSet function cleaned
it become static to clearly state input, output and temporary variables
style updated with minor improvments to actual algorithm.
Committed: https://crrev.com/262e4861284e2deac01f1137c40230f4f4283634
Cr-Commit-Position: refs/heads/master@{#13652}
Patch Set 1 : change function signature and remove now unused functions #
Total comments: 1
Patch Set 2 : Fix variable names style, comments, types. #Patch Set 3 : git cl format --full #Patch Set 4 : indexes replaced with iterators #Patch Set 5 : optimize step2 #Patch Set 6 : removed num_bounding_set variable as dublicate of bounding_set->size() #Patch Set 7 : packet_rate calculated in bytes/s instead of bits/s removing unnecesary multiplications #Patch Set 8 : rebase (remove critical section) #
Total comments: 21
Patch Set 9 : Feedback #
Total comments: 2
Patch Set 10 : Removed default values #
Messages
Total messages: 23 (15 generated)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #6 (id:200001) has been deleted
Patchset #7 (id:240001) has been deleted
Patchset #6 (id:220001) has been deleted
to make CL more reviewable, it is splited into several steps with small description. https://codereview.webrtc.org/1989363006/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/tmmbr_help.cc (right): https://codereview.webrtc.org/1989363006/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:289: boundingSet->back().bitrate_bps() * 1000 / overlooked removing *1000 here, removed in patchset#7
Description was changed from ========== TMMBRHelp::FindBoundingSet function cleaned it become static to clearly state input, output and temporary variables style updated with minor improvments to actual algorithm. ========== to ========== TMMBRHelp::FindBoundingSet function cleaned it become static to clearly state input, output and temporary variables style updated with minor improvments to actual algorithm. ==========
danilchap@webrtc.org changed reviewers: + asapersson@webrtc.org
danilchap@webrtc.org changed reviewers: + philipel@webrtc.org - asapersson@webrtc.org
https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/tmmbr_help.cc (right): https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:69: for (uint32_t i = 0; i < _candidateSet.size(); i++) { size_t https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:96: void TMMBRHelp::FindBoundingSet(std::vector<rtcp::TmmbItem> candidates, If you change line 104 to copy that single element instead of moving the vector you can change to const std::vector<rtcp::TmmbItem>&, which makes the interface clearer. https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:113: // 2. For tuples with same overhead, keep the one with the lowest bitrate. Add empty line between 112-113. https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:133: // 3. Select and remove tuple with lowest tmmbr. Add empty line between 132-133 https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:136: for (auto it = candidates.begin(); it != candidates.end(); ++it) { Combine this loop with the loop on line 143 or change line 143 to: for (auto it = min_bitrate_it; ... https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:209: if (packet_rate <= intersection[bounding_set->size() - 1]) { Reading form uninitialized memory? Use std::vector instead of std::unique_ptr<float[]>, or change to: std::unique_ptr<float[]> intersection(new float[num_candidates]()); to zero initialize. https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:217: if (packet_rate < max_packet_rate[bounding_set->size() - 1]) { Ditto. https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:239: for (uint32_t i = 0; (i < length) && (i < _boundingSet.size()); ++i) { size_t https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/tmmbr_help.h (right): https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.h:19: class TMMBRSet : public std::vector<rtcp::TmmbItem> { Is there a reason the implementation is in the .h file? If not, move it to the .cc file. https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.h:57: TMMBRSet _candidateSet; If you are refactoring/cleaning this class you might want to change |_candidateSet| to |candidateSet_|. Same for all other private members of course.
https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/tmmbr_help.cc (right): https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:69: for (uint32_t i = 0; i < _candidateSet.size(); i++) { On 2016/08/04 12:39:48, philipel wrote: > size_t Done. https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:96: void TMMBRHelp::FindBoundingSet(std::vector<rtcp::TmmbItem> candidates, On 2016/08/04 12:39:48, philipel wrote: > If you change line 104 to copy that single element instead of moving the vector > you can change to const std::vector<rtcp::TmmbItem>&, which makes the interface > clearer. Disagree, candidates variables mutated more often than that: in Step1 candidates are sorted (in place) in Step2 and later 'inactive' candidates marked with set_bitrate_bps(0) (mirroring what TmmbrSet was doing with ClearEntry function) The idea here is that I know most of the time caller wouldn't need the list of candidates after calling this function, so I ask not a copy, but the original list to avoid making an extra copy. https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:113: // 2. For tuples with same overhead, keep the one with the lowest bitrate. On 2016/08/04 12:39:48, philipel wrote: > Add empty line between 112-113. Done. https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:133: // 3. Select and remove tuple with lowest tmmbr. On 2016/08/04 12:39:48, philipel wrote: > Add empty line between 132-133 Done. https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:136: for (auto it = candidates.begin(); it != candidates.end(); ++it) { On 2016/08/04 12:39:48, philipel wrote: > Combine this loop with the loop on line 143 or change line 143 to: > for (auto it = min_bitrate_it; ... Done. https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:209: if (packet_rate <= intersection[bounding_set->size() - 1]) { On 2016/08/04 12:39:48, philipel wrote: > Reading form uninitialized memory? > > Use std::vector instead of std::unique_ptr<float[]>, or change to: > std::unique_ptr<float[]> intersection(new float[num_candidates]()); > to zero initialize. Done. There is some logic that first bounding_set->size() entries of intersection/packet_rate are always initialized, but std::vector<float> still make much more sense than unique_ptr<float[]> https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:217: if (packet_rate < max_packet_rate[bounding_set->size() - 1]) { On 2016/08/04 12:39:48, philipel wrote: > Ditto. Done. https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:239: for (uint32_t i = 0; (i < length) && (i < _boundingSet.size()); ++i) { On 2016/08/04 12:39:48, philipel wrote: > size_t Done. https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/tmmbr_help.h (right): https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.h:19: class TMMBRSet : public std::vector<rtcp::TmmbItem> { On 2016/08/04 12:39:49, philipel wrote: > Is there a reason the implementation is in the .h file? If not, move it to the > .cc file. Goal is to remove this class entirely and use std::vector<rtcp::TmmbItem> instead. Refactoring FindBoundingSet is an important step towards that goal. Implemtation is kept in the header file to make it easier to review replacements of legacy accessors. https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.h:57: TMMBRSet _candidateSet; On 2016/08/04 12:39:48, philipel wrote: > If you are refactoring/cleaning this class you might want to change > |_candidateSet| to |candidateSet_|. > > Same for all other private members of course. this CL concentrates on light refactoring of FindBoundingSet function. follow up CL will cleanup this class. goal is to remove this class completely leaving only FindBoundingSet function: https://codereview.webrtc.org/1474693002/
lgtm with nit, but I guess this class will be gone soon anyway. https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/tmmbr_help.cc (right): https://codereview.webrtc.org/1989363006/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:96: void TMMBRHelp::FindBoundingSet(std::vector<rtcp::TmmbItem> candidates, On 2016/08/04 13:55:35, danilchap wrote: > On 2016/08/04 12:39:48, philipel wrote: > > If you change line 104 to copy that single element instead of moving the > vector > > you can change to const std::vector<rtcp::TmmbItem>&, which makes the > interface > > clearer. > > Disagree, candidates variables mutated more often than that: > in Step1 candidates are sorted (in place) > in Step2 and later 'inactive' candidates marked with set_bitrate_bps(0) > (mirroring what TmmbrSet was doing with ClearEntry function) True. > The idea here is that I know most of the time caller wouldn't need the list of > candidates after calling this function, so I ask not a copy, but the original > list to avoid making an extra copy. Makes sense. https://codereview.webrtc.org/1989363006/diff/320001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/tmmbr_help.cc (right): https://codereview.webrtc.org/1989363006/diff/320001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:154: std::vector<float> intersection(num_candidates, 0.0); Default value 0.0 not really necessary.
https://codereview.webrtc.org/1989363006/diff/320001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/tmmbr_help.cc (right): https://codereview.webrtc.org/1989363006/diff/320001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:154: std::vector<float> intersection(num_candidates, 0.0); On 2016/08/04 14:37:20, philipel wrote: > Default value 0.0 not really necessary. They are also misleading because code doesn't rely on default values. Removed, thank you.
The CQ bit was checked by danilchap@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org Link to the patchset: https://codereview.webrtc.org/1989363006/#ps340001 (title: "Removed default values")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== TMMBRHelp::FindBoundingSet function cleaned it become static to clearly state input, output and temporary variables style updated with minor improvments to actual algorithm. ========== to ========== TMMBRHelp::FindBoundingSet function cleaned it become static to clearly state input, output and temporary variables style updated with minor improvments to actual algorithm. ==========
Message was sent while issue was closed.
Committed patchset #10 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== TMMBRHelp::FindBoundingSet function cleaned it become static to clearly state input, output and temporary variables style updated with minor improvments to actual algorithm. ========== to ========== TMMBRHelp::FindBoundingSet function cleaned it become static to clearly state input, output and temporary variables style updated with minor improvments to actual algorithm. Committed: https://crrev.com/262e4861284e2deac01f1137c40230f4f4283634 Cr-Commit-Position: refs/heads/master@{#13652} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/262e4861284e2deac01f1137c40230f4f4283634 Cr-Commit-Position: refs/heads/master@{#13652} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
