Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2235223002:
Add ability to handle data from multiple streams in RateAccCounter. (Closed)
|
Created:
4 years, 4 months ago by åsapersson Modified:
4 years, 1 month ago Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd ability to handle data from multiple streams in RateAccCounter.
BUG=webrtc:5283
Committed: https://crrev.com/fe647f4ab218c570a0d2575f7e14325932afe7d1
Cr-Commit-Position: refs/heads/master@{#14864}
Patch Set 1 #Patch Set 2 #Patch Set 3 #Patch Set 4 #Patch Set 5 : rebase #Patch Set 6 #
Total comments: 5
Patch Set 7 #
Total comments: 6
Patch Set 8 : rebase #Patch Set 9 : address comments #Patch Set 10 : int64->bool #Patch Set 11 : rebase #
Total comments: 4
Patch Set 12 : rename ssrc to stream_id #Patch Set 13 : int -> uint32_t #
Messages
Total messages: 34 (22 generated)
Description was changed from ========== ssf ssf ssf BUG= ========== to ========== Add support for multiple ssrc in StatsCounter class... BUG= ==========
Description was changed from ========== Add support for multiple ssrc in StatsCounter class... BUG= ========== to ========== Add ability to handle data from multiple streams in RateAccCounter. BUG= ==========
asapersson@webrtc.org changed reviewers: + mflodman@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/2235223002/diff/100001/webrtc/video/stats_count... File webrtc/video/stats_counter.cc (right): https://codereview.webrtc.org/2235223002/diff/100001/webrtc/video/stats_count... webrtc/video/stats_counter.cc:165: samples_[0].Add(sample); Is zero some default ssrc? I think it's also a valid ssrc, so I don't think using it as default is safe? https://codereview.webrtc.org/2235223002/diff/100001/webrtc/video/stats_count... File webrtc/video/stats_counter.h (right): https://codereview.webrtc.org/2235223002/diff/100001/webrtc/video/stats_count... webrtc/video/stats_counter.h:79: class StatsCounter { Would it be cleaner to have an StreamsStatsCounter which contains a map from ssrc or stream_id to StatsCounter? That way you still have the simple StatsCounter and don't have to use ssrc 0 for that.
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:160001) has been deleted
https://codereview.webrtc.org/2235223002/diff/100001/webrtc/video/stats_count... File webrtc/video/stats_counter.cc (right): https://codereview.webrtc.org/2235223002/diff/100001/webrtc/video/stats_count... webrtc/video/stats_counter.cc:165: samples_[0].Add(sample); On 2016/08/19 14:36:56, stefan-webrtc (holmer) wrote: > Is zero some default ssrc? I think it's also a valid ssrc, so I don't think > using it as default is safe? The ssrc can only be set for RateAccCounter (and not for the other counters) so just set to something here (i.e. 0). https://codereview.webrtc.org/2235223002/diff/100001/webrtc/video/stats_count... File webrtc/video/stats_counter.h (right): https://codereview.webrtc.org/2235223002/diff/100001/webrtc/video/stats_count... webrtc/video/stats_counter.h:79: class StatsCounter { On 2016/08/19 14:36:56, stefan-webrtc (holmer) wrote: > Would it be cleaner to have an StreamsStatsCounter which contains a map from > ssrc or stream_id to StatsCounter? That way you still have the simple > StatsCounter and don't have to use ssrc 0 for that. Updated the code a bit, ptal. GetMetric is returning a metric from one or more streams.
Patchset #7 (id:180001) has been deleted
Description was changed from ========== Add ability to handle data from multiple streams in RateAccCounter. BUG= ========== to ========== Add ability to handle data from multiple streams in RateAccCounter. BUG=webrtc:5283 ==========
https://codereview.webrtc.org/2235223002/diff/100001/webrtc/video/stats_count... File webrtc/video/stats_counter.cc (right): https://codereview.webrtc.org/2235223002/diff/100001/webrtc/video/stats_count... webrtc/video/stats_counter.cc:165: samples_[0].Add(sample); On 2016/08/23 09:06:02, åsapersson wrote: > On 2016/08/19 14:36:56, stefan-webrtc (holmer) wrote: > > Is zero some default ssrc? I think it's also a valid ssrc, so I don't think > > using it as default is safe? > > The ssrc can only be set for RateAccCounter (and not for the other counters) so > just set to something here (i.e. 0). Acknowledged. https://codereview.webrtc.org/2235223002/diff/200001/webrtc/video/stats_count... File webrtc/video/stats_counter.cc (right): https://codereview.webrtc.org/2235223002/diff/200001/webrtc/video/stats_count... webrtc/video/stats_counter.cc:71: int64_t Num() const { Should we rename Num and num_ Count() and count_? https://codereview.webrtc.org/2235223002/diff/200001/webrtc/video/stats_count... webrtc/video/stats_counter.cc:218: return false; This is a fairly expensive way of checking for no samples since you loop through the samples and add them up. What's the expected size of the map? Maybe it's better to keep a variable around in Samples which keeps track of the count?
https://codereview.webrtc.org/2235223002/diff/200001/webrtc/video/stats_count... File webrtc/video/stats_counter.cc (right): https://codereview.webrtc.org/2235223002/diff/200001/webrtc/video/stats_count... webrtc/video/stats_counter.cc:186: void StatsCounter::Add(int sample) { Do you think it would be better to just expose the SSRC here to keep the class more general and have the user use ssrc 0 whenever it doesn't have ssrcs?
Patchset #14 (id:340001) has been deleted
Patchset #10 (id:260001) has been deleted
Patchset #10 (id:280001) has been deleted
Patchset #10 (id:300001) has been deleted
Patchset #10 (id:320001) has been deleted
Patchset #12 (id:400001) has been deleted
Patchset #10 (id:360001) has been deleted
Patchset #10 (id:380001) has been deleted
Patchset #9 (id:240001) has been deleted
https://codereview.webrtc.org/2235223002/diff/200001/webrtc/video/stats_count... File webrtc/video/stats_counter.cc (right): https://codereview.webrtc.org/2235223002/diff/200001/webrtc/video/stats_count... webrtc/video/stats_counter.cc:71: int64_t Num() const { On 2016/09/08 07:13:34, stefan-webrtc (holmer) wrote: > Should we rename Num and num_ Count() and count_? Done. https://codereview.webrtc.org/2235223002/diff/200001/webrtc/video/stats_count... webrtc/video/stats_counter.cc:186: void StatsCounter::Add(int sample) { On 2016/09/08 07:15:13, stefan-webrtc (holmer) wrote: > Do you think it would be better to just expose the SSRC here to keep the class > more general and have the user use ssrc 0 whenever it doesn't have ssrcs? It is only one subclass that have use for setting the ssrc, RateAccCounter, so in most cases it will not be needed (for all Add). Maybe keep it as it is? https://codereview.webrtc.org/2235223002/diff/200001/webrtc/video/stats_count... webrtc/video/stats_counter.cc:218: return false; On 2016/09/08 07:13:34, stefan-webrtc (holmer) wrote: > This is a fairly expensive way of checking for no samples since you loop through > the samples and add them up. What's the expected size of the map? Maybe it's > better to keep a variable around in Samples which keeps track of the count? The expected size will be one for all sub-classes except the RateAccCounter. Added a total_count_.
lgtm
Patchset #11 (id:460001) has been deleted
https://codereview.webrtc.org/2235223002/diff/480001/webrtc/video/stats_count... File webrtc/video/stats_counter.cc (right): https://codereview.webrtc.org/2235223002/diff/480001/webrtc/video/stats_count... webrtc/video/stats_counter.cc:130: ++count_; Shouldn't we set max_ here as well? https://codereview.webrtc.org/2235223002/diff/480001/webrtc/video/stats_count... File webrtc/video/stats_counter.h (right): https://codereview.webrtc.org/2235223002/diff/480001/webrtc/video/stats_count... webrtc/video/stats_counter.h:108: void Set(int sample, uint32_t ssrc); Here and in other places in this class, wdyt of changing ssrc to stream_id? Just to avoid adding more "dependencies" on RTP in parts of the code not directly depending on RTP.
https://codereview.webrtc.org/2235223002/diff/480001/webrtc/video/stats_count... File webrtc/video/stats_counter.cc (right): https://codereview.webrtc.org/2235223002/diff/480001/webrtc/video/stats_count... webrtc/video/stats_counter.cc:130: ++count_; On 2016/10/12 00:28:36, mflodman wrote: > Shouldn't we set max_ here as well? MaxCounter which uses max_ only calls the Add method above. Either Add or Set is called for a counter (so will not be used for the counters that calls Set). https://codereview.webrtc.org/2235223002/diff/480001/webrtc/video/stats_count... File webrtc/video/stats_counter.h (right): https://codereview.webrtc.org/2235223002/diff/480001/webrtc/video/stats_count... webrtc/video/stats_counter.h:108: void Set(int sample, uint32_t ssrc); On 2016/10/12 00:28:36, mflodman wrote: > Here and in other places in this class, wdyt of changing ssrc to stream_id? Just > to avoid adding more "dependencies" on RTP in parts of the code not directly > depending on RTP. Done. Renamed to stream_id.
The CQ bit was checked by asapersson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2235223002/#ps520001 (title: "int -> uint32_t")
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 ========== Add ability to handle data from multiple streams in RateAccCounter. BUG=webrtc:5283 ========== to ========== Add ability to handle data from multiple streams in RateAccCounter. BUG=webrtc:5283 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:520001)
Message was sent while issue was closed.
Description was changed from ========== Add ability to handle data from multiple streams in RateAccCounter. BUG=webrtc:5283 ========== to ========== Add ability to handle data from multiple streams in RateAccCounter. BUG=webrtc:5283 Committed: https://crrev.com/fe647f4ab218c570a0d2575f7e14325932afe7d1 Cr-Commit-Position: refs/heads/master@{#14864} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/fe647f4ab218c570a0d2575f7e14325932afe7d1 Cr-Commit-Position: refs/heads/master@{#14864} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
