Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2011473002:
Cleanup BWE SendTimeHistory class (Closed)
|
Created:
4 years, 7 months ago by danilchap Modified:
4 years, 4 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. |
DescriptionCleanup BWE SendTimeHistory class
Committed: https://crrev.com/f734c135a452ef34cc12b8c7f22e70cee71ab6b5
Cr-Commit-Position: refs/heads/master@{#13590}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : #Patch Set 4 : rebase #
Total comments: 15
Patch Set 5 : feedback #Patch Set 6 : rebase #
Total comments: 4
Patch Set 7 : feedback #Patch Set 8 : feedback2 #
Total comments: 1
Patch Set 9 : use SequenceNumberUnwrapper instead of Comparator #
Messages
Total messages: 20 (6 generated)
find(0)+branch+upper_bound(0) where single begin would do provoke me to clean-up this class. While at it renamed parameters to better represent what they are, cleaned up includes, replaced asserts with DCHECKs, replaced key (oldest_sequence_number) with iterator (oldest_packet_info) since key was only used to search for it or next iterator. inlined EraseOld into AddAndRemoveOld (the only place it was used) because it became too short.
danilchap@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/include/send_time_history.h (right): https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:30: // Cleanup overaged entries, then add new packet info with given parameters. Cleanup old entries... https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:35: // Updates send_time_ms field of packet info identified by |sequence_number|. Use |send_time_ms| or remove || from |sequence_number|, prefer to use ||. https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:45: using PacketInfoHistory = std::map<uint16_t, PacketInfo>; Include "webrtc/modules/video_coding/sequence_number_util.h" and define PacketInfoHistory as std::map<uint16_t, PacketInfo, DescendingSeqNumComp<uint16_t>>. As long as you only have at most half the interval of packets in the PacketInfoHistory they will be sorted correctly even if the sequence numbers wrap. Also, you don't need to define PacketInfoHistory if you do this, just let it be a map. https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:51: PacketInfoHistory::iterator oldest_packet_info_; Use custom comparator instead and remove oldest_packet_info_. https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/send_time_history.cc (right): https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/send_time_history.cc:36: while (!history_.empty() && Is the packet with the oldest sequence number also the oldest packet time wise? If so, you can just iterate from 'history_.begin()' to find the range of packets that should be erased. (Assuming you use the custom comparator). https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/send_time_history.cc:40: EraseOldestPacketInfo(); Remove 'EraseOldestPacketInfo' and just do 'history_.erase()' instead.
https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/include/send_time_history.h (right): https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:30: // Cleanup overaged entries, then add new packet info with given parameters. On 2016/06/21 09:48:29, philipel wrote: > Cleanup old entries... Done. https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:35: // Updates send_time_ms field of packet info identified by |sequence_number|. On 2016/06/21 09:48:29, philipel wrote: > Use |send_time_ms| or remove || from |sequence_number|, prefer to use ||. || refer to function parameter. In current phrase send_time_ms doesn't (it refer to internal field instead). Rephrased so that send_time_ms refers to parameter now which makes more sense for an interface description. https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:45: using PacketInfoHistory = std::map<uint16_t, PacketInfo>; On 2016/06/21 09:48:29, philipel wrote: > Include "webrtc/modules/video_coding/sequence_number_util.h" and define > PacketInfoHistory as std::map<uint16_t, PacketInfo, > DescendingSeqNumComp<uint16_t>>. > > As long as you only have at most half the interval of packets in the > PacketInfoHistory they will be sorted correctly even if the sequence numbers > wrap. > > Also, you don't need to define PacketInfoHistory if you do this, just let it be > a map. This is good idea, but unfortunately it would create rbe -> video_coding dependency that generally doesn't make sense. (including header without linking dependency is still a dependency) reimplementing comparer structure instead. (btw, you mean AscendingSeqNumComp, not DescendingSeqNumComp?) https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:51: PacketInfoHistory::iterator oldest_packet_info_; On 2016/06/21 09:48:29, philipel wrote: > Use custom comparator instead and remove oldest_packet_info_. Done. https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/send_time_history.cc (right): https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/send_time_history.cc:36: while (!history_.empty() && On 2016/06/21 09:48:30, philipel wrote: > Is the packet with the oldest sequence number also the oldest packet time wise? > If so, you can just iterate from 'history_.begin()' to find the range of packets > that should be erased. (Assuming you use the custom comparator). might not be the case when packets arrive out of order. searching range to delete might be more efficient, but less readable. (since search is done by some custom predicate instead of by key) This function is called for every new packet, so with about same packet rate it should delete 0 - 2 packets per call. https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/send_time_history.cc:40: EraseOldestPacketInfo(); On 2016/06/21 09:48:30, philipel wrote: > Remove 'EraseOldestPacketInfo' and just do 'history_.erase()' instead. Done.
lgtm https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/include/send_time_history.h (right): https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:35: // Updates send_time_ms field of packet info identified by |sequence_number|. On 2016/06/21 11:45:30, danilchap wrote: > On 2016/06/21 09:48:29, philipel wrote: > > Use |send_time_ms| or remove || from |sequence_number|, prefer to use ||. > > || refer to function parameter. In current phrase send_time_ms doesn't (it refer > to internal field instead). > Rephrased so that send_time_ms refers to parameter now which makes more sense > for an interface description. Acknowledged. https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:45: using PacketInfoHistory = std::map<uint16_t, PacketInfo>; On 2016/06/21 11:45:30, danilchap wrote: > On 2016/06/21 09:48:29, philipel wrote: > > Include "webrtc/modules/video_coding/sequence_number_util.h" and define > > PacketInfoHistory as std::map<uint16_t, PacketInfo, > > DescendingSeqNumComp<uint16_t>>. > > > > As long as you only have at most half the interval of packets in the > > PacketInfoHistory they will be sorted correctly even if the sequence numbers > > wrap. > > > > Also, you don't need to define PacketInfoHistory if you do this, just let it > be > > a map. > > This is good idea, but unfortunately it would create rbe -> video_coding > dependency that generally doesn't make sense. > (including header without linking dependency is still a dependency) > reimplementing comparer structure instead. > (btw, you mean AscendingSeqNumComp, not DescendingSeqNumComp?) I guess that makes sense. I think the right thing to do is to move "sequence_number_util.h" to some better place where it can be used for cases such as this, but that is for another CL of course. https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/send_time_history.cc (right): https://codereview.webrtc.org/2011473002/diff/60001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/send_time_history.cc:36: while (!history_.empty() && On 2016/06/21 11:45:30, danilchap wrote: > On 2016/06/21 09:48:30, philipel wrote: > > Is the packet with the oldest sequence number also the oldest packet time > wise? > > If so, you can just iterate from 'history_.begin()' to find the range of > packets > > that should be erased. (Assuming you use the custom comparator). > > might not be the case when packets arrive out of order. > > searching range to delete might be more efficient, but less readable. (since > search is done by some custom predicate instead of by key) > This function is called for every new packet, so with about same packet rate it > should delete 0 - 2 packets per call. > Acknowledged.
stefan@webrtc.org changed reviewers: + sprang@google.com
lgtm, but I'd be more comfortable if sprang@ also verified that I haven't missed some corner case that the old, way more complicated, code was supposed to cover. :) https://codereview.webrtc.org/2011473002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/include/send_time_history.h (right): https://codereview.webrtc.org/2011473002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:45: struct SeqNoComparer { SeqNumComparator? https://codereview.webrtc.org/2011473002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/send_time_history.cc (right): https://codereview.webrtc.org/2011473002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/send_time_history.cc:43: constexpr int64_t kNoArrivalTimeMs = 0; // Arrival time is ignored. Would -1 be a better number for an ignored arrival time?
https://codereview.webrtc.org/2011473002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/include/send_time_history.h (right): https://codereview.webrtc.org/2011473002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/include/send_time_history.h:45: struct SeqNoComparer { On 2016/08/01 11:22:49, stefan-webrtc (holmer) wrote: > SeqNumComparator? Done. https://codereview.webrtc.org/2011473002/diff/100001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/send_time_history.cc (right): https://codereview.webrtc.org/2011473002/diff/100001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/send_time_history.cc:43: constexpr int64_t kNoArrivalTimeMs = 0; // Arrival time is ignored. On 2016/08/01 11:22:49, stefan-webrtc (holmer) wrote: > Would -1 be a better number for an ignored arrival time? Used same value that was before. Since it is ignored, the actual value (0 or -1) doesn't matter: user never can see it. Having ignored as 0 and unset as -1 distracts with a unimportant question 'why?'. Made both -1 for consistency.
sprang@webrtc.org changed reviewers: + sprang@webrtc.org
I have one concern, and that is that we need to check that the comparator
doesn't violate the transitive property. For example, if the map contains {x =
0, y = 32000} and we add an item z = 48000, x < y < z won't hold as z will be
seen as a backwards wrap from x, but a forwards move from y. This can cause the
map to be in a bad state where an inserted item cannot be looked up.
I also think the old implementation had a bug and didn't handle backwards wraps
correctly. :)
https://codereview.webrtc.org/2011473002/diff/140001/webrtc/modules/remote_bi...
File webrtc/modules/remote_bitrate_estimator/send_time_history.cc (right):
https://codereview.webrtc.org/2011473002/diff/140001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/send_time_history.cc:77: uint16_t rhs)
const {
Nit: Indentation
On 2016/08/01 12:41:21, språng wrote:
Comparator does violate the transitive property. Normally sequence number range
of under aged
packets should be small and it shouldn't be an issue,
but those numbers come from the network, so this non-transitive property can be
abused in
some unexpected way.
Replaced it with unwrapper, so map now has clearly transitive keys, yet
backwards wraps are still handled.
ptal
> I have one concern, and that is that we need to check that the comparator
> doesn't violate the transitive property. For example, if the map contains {x =
> 0, y = 32000} and we add an item z = 48000, x < y < z won't hold as z will be
> seen as a backwards wrap from x, but a forwards move from y. This can cause
the
> map to be in a bad state where an inserted item cannot be looked up.
>
> I also think the old implementation had a bug and didn't handle backwards
wraps
> correctly. :)
>
>
https://codereview.webrtc.org/2011473002/diff/140001/webrtc/modules/remote_bi...
> File webrtc/modules/remote_bitrate_estimator/send_time_history.cc (right):
>
>
lgtm Thanks!
lgtm
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/2011473002/#ps160001 (title: "use SequenceNumberUnwrapper instead of Comparator")
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.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Cleanup BWE SendTimeHistory class ========== to ========== Cleanup BWE SendTimeHistory class Committed: https://crrev.com/f734c135a452ef34cc12b8c7f22e70cee71ab6b5 Cr-Commit-Position: refs/heads/master@{#13590} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/f734c135a452ef34cc12b8c7f22e70cee71ab6b5 Cr-Commit-Position: refs/heads/master@{#13590} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
