Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1669323002:
TMMBRSet become vector<rtcp::TmmbItem> (Closed)
|
Created:
4 years, 10 months ago by danilchap Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionTMMBRSet become vector<rtcp::TmmbItem>
this is a slice of https://codereview.webrtc.org/1474693002/
All TMMBRSet functions intentionally left unchanged. Goal to make them obsolete, not to clear.
BUG=webrtc:5565
Committed: https://crrev.com/a4f31bd03a7bf52470ae7143fa25cddef2cbf02c
Cr-Commit-Position: refs/heads/master@{#11813}
Patch Set 1 #
Total comments: 16
Patch Set 2 : at(i)->(*this)[i]; assert -> RTC_DCHECK #Patch Set 3 : makes std::vector<rtcp::TmmbItem> compatible with TMMBRSet #
Messages
Total messages: 22 (9 generated)
Description was changed from ========== TMMBRSet becomes vector<rtcp::TmmbItem> this is a slice of https://codereview.webrtc.org/1474693002/ All TMMBRSet functions intentionally left unchanged. Goal to make them obsolete, not to clear. BUG= ========== to ========== TMMBRSet become vector<rtcp::TmmbItem> this is a slice of https://codereview.webrtc.org/1474693002/ All TMMBRSet functions intentionally left unchanged. Goal to make them obsolete, not to clear. BUG= ==========
Description was changed from ========== TMMBRSet become vector<rtcp::TmmbItem> this is a slice of https://codereview.webrtc.org/1474693002/ All TMMBRSet functions intentionally left unchanged. Goal to make them obsolete, not to clear. BUG= ========== to ========== TMMBRSet become vector<rtcp::TmmbItem> this is a slice of https://codereview.webrtc.org/1474693002/ All TMMBRSet functions intentionally left unchanged. Goal to make them obsolete, not to clear. BUG=webrtc:5565 ==========
danilchap@webrtc.org changed reviewers: + philipel@webrtc.org
Don't you think it is possible to replace this class completely with a vector? https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/tmmbr_help.cc (right): https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:42: at(i).set_bitrate_bps(0); replace at() with operator[] since at() does bound checking even in release and throws an exception(!) if it is out of bounds. https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:62: assert(i < size()); Use RTC_DCHECK instead of assert. https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:63: at(i).set_bitrate_bps(tmmbrSet * 1000); Again, replace at() with operator[]. https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:74: assert(_lengthOfSet < size()); Use RTC_DCHECK. https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:79: assert(sourceIdx < _lengthOfSet); RTC_DCHECK :) https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:87: swap(at(i), at(j)); Remove line 86 and just go for std::swap instead. Also replace at() with operator[]. https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/tmmbr_help.h (right): https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.h:31: uint32_t sizeOfSet() const { return size(); } Maybe remove this function since you inherit the public api of std::vector? https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.h:36: return at(i).bitrate_bps() / 1000; Replace at() with operator[] since at() does bound checking.
It is sure possible to replace it with plain vector (and that is one of the goals, see master CL in description), but it is not that trivial. https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/tmmbr_help.cc (right): https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:42: at(i).set_bitrate_bps(0); On 2016/02/23 13:46:39, philipel wrote: > replace at() with operator[] since at() does bound checking even in release and > throws an exception(!) if it is out of bounds. exception is not a problem, still better avoid unnecessary range check. Done. https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:62: assert(i < size()); On 2016/02/23 13:46:39, philipel wrote: > Use RTC_DCHECK instead of assert. Done. https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:63: at(i).set_bitrate_bps(tmmbrSet * 1000); On 2016/02/23 13:46:39, philipel wrote: > Again, replace at() with operator[]. Done. https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:74: assert(_lengthOfSet < size()); On 2016/02/23 13:46:38, philipel wrote: > Use RTC_DCHECK. Done. https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:79: assert(sourceIdx < _lengthOfSet); On 2016/02/23 13:46:38, philipel wrote: > RTC_DCHECK :) Done. https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:87: swap(at(i), at(j)); On 2016/02/23 13:46:38, philipel wrote: > Remove line 86 and just go for std::swap instead. But that is not recommended way to use swap function. Though I know swap function is not overloaded for TmmbItem, still, TmmnItem is not trivial type, would rather use general style for swap. https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/tmmbr_help.h (right): https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.h:31: uint32_t sizeOfSet() const { return size(); } On 2016/02/23 13:46:39, philipel wrote: > Maybe remove this function since you inherit the public api of std::vector? Would rather do it in other CL: removing this function would require updating all users of this class. This CL concentrated on altering TMMBRSet implementation without touching interface. https://codereview.webrtc.org/1669323002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/tmmbr_help.h:36: return at(i).bitrate_bps() / 1000; On 2016/02/23 13:46:39, philipel wrote: > Replace at() with operator[] since at() does bound checking. Done.
LGTM
Took a deeper thought about question 'can it be replaced by vector' and realized current way is not helpful: TMBBRSet relies on length field that can't be set if one use only std::vector functions to fill TMBBRSet. This patchset syncs two different ways to get useful size of the TMMBRSet. Because elements between size and capacity are not cleared now, code that rely on it fixed to iterate until size instead of capacity.
On 2016/02/24 10:35:05, danilchap wrote: > Took a deeper thought about question 'can it be replaced by vector' and realized > current way is not helpful: TMBBRSet relies on length field that can't be set if > one use only std::vector functions to fill TMBBRSet. > > This patchset syncs two different ways to get useful size of the TMMBRSet. > Because elements between size and capacity are not cleared now, code that rely > on it fixed to iterate until size instead of capacity. LGTM
danilchap@webrtc.org changed reviewers: + stefan@webrtc.org
stefan, PTAL for an owner approval
lgtm
The CQ bit was checked by danilchap@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1669323002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1669323002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by danilchap@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1669323002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1669323002/40001
Message was sent while issue was closed.
Description was changed from ========== TMMBRSet become vector<rtcp::TmmbItem> this is a slice of https://codereview.webrtc.org/1474693002/ All TMMBRSet functions intentionally left unchanged. Goal to make them obsolete, not to clear. BUG=webrtc:5565 ========== to ========== TMMBRSet become vector<rtcp::TmmbItem> this is a slice of https://codereview.webrtc.org/1474693002/ All TMMBRSet functions intentionally left unchanged. Goal to make them obsolete, not to clear. BUG=webrtc:5565 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== TMMBRSet become vector<rtcp::TmmbItem> this is a slice of https://codereview.webrtc.org/1474693002/ All TMMBRSet functions intentionally left unchanged. Goal to make them obsolete, not to clear. BUG=webrtc:5565 ========== to ========== TMMBRSet become vector<rtcp::TmmbItem> this is a slice of https://codereview.webrtc.org/1474693002/ All TMMBRSet functions intentionally left unchanged. Goal to make them obsolete, not to clear. BUG=webrtc:5565 Committed: https://crrev.com/a4f31bd03a7bf52470ae7143fa25cddef2cbf02c Cr-Commit-Position: refs/heads/master@{#11813} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a4f31bd03a7bf52470ae7143fa25cddef2cbf02c Cr-Commit-Position: refs/heads/master@{#11813} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
