Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1433393002:
Add separate send-side UMA stats for screenshare and video. (Closed)
|
Created:
5 years, 1 month ago by sprang_webrtc Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), stefan-webrtc, tterriberry_mozilla.com, andresp, the sun, perkj_webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd separate send-side UMA stats for screenshare and video.
This CL duplicates all the histograms in SendStatisticsProxy. Might be
overkill, but we don't know which stats will be interesting and it makes
the change easier.
BUG=
Committed: https://crrev.com/b4a1ae5299fd57be66c7cbb7a982179bb1ecfb90
Cr-Commit-Position: refs/heads/master@{#10885}
Patch Set 1 #Patch Set 2 : Reset stats and update histograms on content type change #
Total comments: 12
Patch Set 3 : Addressed comments #
Total comments: 1
Patch Set 4 : Recreate VideoSendStream on content type change #Patch Set 5 : Fixed tests #Patch Set 6 : Fixed use-after-free in test #
Total comments: 2
Patch Set 7 : Reset stats in stats_proxy, donstream #
Total comments: 4
Patch Set 8 : Addressed comment #Patch Set 9 : Actually run UpdateHistograms... #
Total comments: 5
Patch Set 10 : Addressed comments #
Total comments: 1
Patch Set 11 : Fixed compile error #
Messages
Total messages: 51 (11 generated)
sprang@webrtc.org changed reviewers: + asapersson@webrtc.org, stefan@webrtc.org
sprang@webrtc.org changed reviewers: + pbos@webrtc.org - stefan@webrtc.org
-stefan since he's OOO I've changed this to reset the stats and update the histograms when content type is changed. This so we don't skew stats for a type that's not currently active.
https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.cc:67: int input_fps = This one should probably also have kMinRequiredSamples? https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy.h (right): https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.h:131: struct StatsSet { StatsSet sounds too generic, can you think of a name that also says what it contains, or put a comment above with why we need it separate (that it resets between screenshare/realtime)? https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.h:132: explicit StatsSet(const char* prefix) Put constructor in .cc file. https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.h:139: const std::string kPrefix; stats_prefix_, "const" doesn't make it a "kConstant". https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.h:163: rtc::scoped_ptr<StatsSet> stats_set_ GUARDED_BY(crit_); Since this doesn't rely on a destructor being called, skip the scoped_ptr and just copy-replace it.
https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.cc:67: int input_fps = On 2015/11/12 10:38:19, pbos-webrtc wrote: > This one should probably also have kMinRequiredSamples? Done. https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy.h (right): https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.h:131: struct StatsSet { On 2015/11/12 10:38:19, pbos-webrtc wrote: > StatsSet sounds too generic, can you think of a name that also says what it > contains, or put a comment above with why we need it separate (that it resets > between screenshare/realtime)? Done. https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.h:132: explicit StatsSet(const char* prefix) On 2015/11/12 10:38:19, pbos-webrtc wrote: > Put constructor in .cc file. Done. https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.h:139: const std::string kPrefix; On 2015/11/12 10:38:19, pbos-webrtc wrote: > stats_prefix_, "const" doesn't make it a "kConstant". Done. https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.h:163: rtc::scoped_ptr<StatsSet> stats_set_ GUARDED_BY(crit_); On 2015/11/12 10:38:19, pbos-webrtc wrote: > Since this doesn't rely on a destructor being called, skip the scoped_ptr and > just copy-replace it. Ugh, that would mean I have to add copy assignment operators to most of the classes it contains. Do I have to? :)
https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy.h (right): https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.h:163: rtc::scoped_ptr<StatsSet> stats_set_ GUARDED_BY(crit_); On 2015/11/12 11:01:25, språng wrote: > On 2015/11/12 10:38:19, pbos-webrtc wrote: > > Since this doesn't rely on a destructor being called, skip the scoped_ptr and > > just copy-replace it. > > Ugh, that would mean I have to add copy assignment operators to most of the > classes it contains. Do I have to? :) They're not copy-constructible by default?
https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy.h (right): https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.h:163: rtc::scoped_ptr<StatsSet> stats_set_ GUARDED_BY(crit_); On 2015/11/12 11:09:40, pbos-webrtc wrote: > On 2015/11/12 11:01:25, språng wrote: > > On 2015/11/12 10:38:19, pbos-webrtc wrote: > > > Since this doesn't rely on a destructor being called, skip the scoped_ptr > and > > > just copy-replace it. > > > > Ugh, that would mean I have to add copy assignment operators to most of the > > classes it contains. Do I have to? :) > > They're not copy-constructible by default? Nope. :(
On 2015/11/12 13:32:06, språng wrote: > https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... > File webrtc/video/send_statistics_proxy.h (right): > > https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... > webrtc/video/send_statistics_proxy.h:163: rtc::scoped_ptr<StatsSet> stats_set_ > GUARDED_BY(crit_); > On 2015/11/12 11:09:40, pbos-webrtc wrote: > > On 2015/11/12 11:01:25, språng wrote: > > > On 2015/11/12 10:38:19, pbos-webrtc wrote: > > > > Since this doesn't rely on a destructor being called, skip the scoped_ptr > > and > > > > just copy-replace it. > > > > > > Ugh, that would mean I have to add copy assignment operators to most of the > > > classes it contains. Do I have to? :) > > > > They're not copy-constructible by default? > > Nope. :( For some magic reason or just RTC_DISALLOW_COPY...?
On 2015/11/12 13:37:20, pbos-webrtc wrote: > On 2015/11/12 13:32:06, språng wrote: > > > https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... > > File webrtc/video/send_statistics_proxy.h (right): > > > > > https://codereview.webrtc.org/1433393002/diff/20001/webrtc/video/send_statist... > > webrtc/video/send_statistics_proxy.h:163: rtc::scoped_ptr<StatsSet> stats_set_ > > GUARDED_BY(crit_); > > On 2015/11/12 11:09:40, pbos-webrtc wrote: > > > On 2015/11/12 11:01:25, språng wrote: > > > > On 2015/11/12 10:38:19, pbos-webrtc wrote: > > > > > Since this doesn't rely on a destructor being called, skip the > scoped_ptr > > > and > > > > > just copy-replace it. > > > > > > > > Ugh, that would mean I have to add copy assignment operators to most of > the > > > > classes it contains. Do I have to? :) > > > > > > They're not copy-constructible by default? > > > > Nope. :( > > For some magic reason or just RTC_DISALLOW_COPY...? Default one implicitly deleted because of const members and no explicit either.
Do you think we should reset the VideoSendStream in WebRtcVideoEngine2.cc when we switch between screenshare/realtime instead? https://codereview.webrtc.org/1433393002/diff/40001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/1433393002/diff/40001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.cc:407: if (codec_mode_ != mode) { This is racy since pending frames might not have been reported before we switch mode, so we get some initial samples from old reporting in the wrong bucket. I don't think that matters too much, but please put a TODO here.
As per discussion, now resetting entire send stream on content type change. Feels like there should be a test for this, any opinion on how that's most easily accomplished?
The CQ bit was checked by sprang@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1433393002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1433393002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/10802) mac_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_rel/builds/10462)
On 2015/11/13 13:06:55, språng wrote:
> As per discussion, now resetting entire send stream on content type change.
> Feels like there should be a test for this, any opinion on how that's most
> easily accomplished?
Something like:
EXPECT_EQ(
1, test::NumHistogramSamples("WebRTC.Call.VideoBitrateReceivedInKbps"));
but within webrtcvideoengine2_unittests.cc? I think there's some tests in there
that input a screenshare frame etc.?
On 2015/11/17 14:49:10, pbos-webrtc wrote:
> On 2015/11/13 13:06:55, språng wrote:
> > As per discussion, now resetting entire send stream on content type change.
> > Feels like there should be a test for this, any opinion on how that's most
> > easily accomplished?
>
> Something like:
>
> EXPECT_EQ(
> 1, test::NumHistogramSamples("WebRTC.Call.VideoBitrateReceivedInKbps"));
>
> but within webrtcvideoengine2_unittests.cc? I think there's some tests in
there
> that input a screenshare frame etc.?
And also, obviously for the separate types of stats that you wanna test for.
On 2015/11/17 14:50:08, pbos-webrtc wrote:
> On 2015/11/17 14:49:10, pbos-webrtc wrote:
> > On 2015/11/13 13:06:55, språng wrote:
> > > As per discussion, now resetting entire send stream on content type
change.
> > > Feels like there should be a test for this, any opinion on how that's most
> > > easily accomplished?
> >
> > Something like:
> >
> > EXPECT_EQ(
> > 1,
test::NumHistogramSamples("WebRTC.Call.VideoBitrateReceivedInKbps"));
> >
> > but within webrtcvideoengine2_unittests.cc? I think there's some tests in
> there
> > that input a screenshare frame etc.?
>
> And also, obviously for the separate types of stats that you wanna test for.
Seems most tests there actually use a FakeVideoStream, so calls don't actually
get to a send stats proxy. I've added tests to EndToEnd instead and altered
the webrtcvideoengine2_unittests slightly. We can at least detect a reset based
on number of captured frames being reset. Good enough?
On 2015/11/20 09:34:49, språng wrote:
> On 2015/11/17 14:50:08, pbos-webrtc wrote:
> > On 2015/11/17 14:49:10, pbos-webrtc wrote:
> > > On 2015/11/13 13:06:55, språng wrote:
> > > > As per discussion, now resetting entire send stream on content type
> change.
> > > > Feels like there should be a test for this, any opinion on how that's
most
> > > > easily accomplished?
> > >
> > > Something like:
> > >
> > > EXPECT_EQ(
>
> > > 1,
> test::NumHistogramSamples("WebRTC.Call.VideoBitrateReceivedInKbps"));
> > >
> > > but within webrtcvideoengine2_unittests.cc? I think there's some tests in
> > there
> > > that input a screenshare frame etc.?
> >
> > And also, obviously for the separate types of stats that you wanna test for.
>
> Seems most tests there actually use a FakeVideoStream, so calls don't actually
> get to a send stats proxy. I've added tests to EndToEnd instead and altered
> the webrtcvideoengine2_unittests slightly. We can at least detect a reset
based
> on number of captured frames being reset. Good enough?
You can then verify that a FakeVideoStream is recreated in that test, I think
each stream contains number of incoming frames to that stream, or you can add
that.
On 2015/11/20 09:43:14, pbos-webrtc wrote:
> On 2015/11/20 09:34:49, språng wrote:
> > On 2015/11/17 14:50:08, pbos-webrtc wrote:
> > > On 2015/11/17 14:49:10, pbos-webrtc wrote:
> > > > On 2015/11/13 13:06:55, språng wrote:
> > > > > As per discussion, now resetting entire send stream on content type
> > change.
> > > > > Feels like there should be a test for this, any opinion on how that's
> most
> > > > > easily accomplished?
> > > >
> > > > Something like:
> > > >
> > > > EXPECT_EQ(
>
> >
> > > > 1,
> > test::NumHistogramSamples("WebRTC.Call.VideoBitrateReceivedInKbps"));
> > > >
> > > > but within webrtcvideoengine2_unittests.cc? I think there's some tests
in
> > > there
> > > > that input a screenshare frame etc.?
> > >
> > > And also, obviously for the separate types of stats that you wanna test
for.
> >
> > Seems most tests there actually use a FakeVideoStream, so calls don't
actually
> > get to a send stats proxy. I've added tests to EndToEnd instead and altered
> > the webrtcvideoengine2_unittests slightly. We can at least detect a reset
> based
> > on number of captured frames being reset. Good enough?
>
> You can then verify that a FakeVideoStream is recreated in that test, I think
> each stream contains number of incoming frames to that stream, or you can add
> that.
That's basically what I've got. This verifies UMA stats separately from
verifying streams get recreated when switching content type though. Question is
if it is worth the effort to actually try this with non-fake streams and insert
enough frames into a real pipeline to see that UMA is populated when stream is
removed. I say it's overkill, WDYT?
ping?
On 2015/11/13 13:06:55, språng wrote: > As per discussion, now resetting entire send stream on content type change. > Feels like there should be a test for this, any opinion on how that's most > easily accomplished? Can you give a recap of the discussion and why this was chosen?
On 2015/11/23 14:28:09, mflodman wrote: > On 2015/11/13 13:06:55, språng wrote: > > As per discussion, now resetting entire send stream on content type change. > > Feels like there should be a test for this, any opinion on how that's most > > easily accomplished? > > Can you give a recap of the discussion and why this was chosen? First approach was to try to determine content type in the rtp module, which was extremely cumbersome and so was abandoned. Instead I wrapped all the uma stats in send_stats_proxy in container class and used one for screenshare and one for realtime video. This would however have resulted in scewed stats since results aren't reported until the stream is destroyed (and having a "inactive" set of stats would have seemed like 0 fps input etc during that time). In order to solve that, we would have to detect content type change in VideoSendStream and then signal the stats proxy to create a new set of uma stats and report the old one. This seemed annoyingly complex so we decided to just reset the entire stream when changing content type, resulting in a very simple change in webrtcvideoengine2.cc
PING
Magnus do you think this is an OK approach or should we enforce it in VideoSendStream since the API permits reconfiguring content type here?
Ping mflodman
sprang@webrtc.org changed reviewers: + mflodman@webrtc.org
+mflodman
https://codereview.webrtc.org/1433393002/diff/100001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1433393002/diff/100001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvideoengine2.cc:2156: RecreateWebRtcStream(); As I read the code, this will generate a new sequence number and timestamp base for the same ssrc and srtp instance and won't work well.
https://codereview.webrtc.org/1433393002/diff/100001/talk/media/webrtc/webrtc... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1433393002/diff/100001/talk/media/webrtc/webrtc... talk/media/webrtc/webrtcvideoengine2.cc:2156: RecreateWebRtcStream(); On 2015/12/02 08:59:13, mflodman wrote: > As I read the code, this will generate a new sequence number and timestamp base > for the same ssrc and srtp instance and won't work well. Now I agree this seems to work, given the magic in VideoSendStream. I'd still like to discuss this though.
As per offline discussion, changed to reset stats internally in SendStatsProxy instead of resetting entire VideoSendStream on change of content type.
Thanks, I think this looks much better without resetting the entire video stream! I'd like to change ont thing and there is a question for Åsa, then LGTM. https://codereview.webrtc.org/1433393002/diff/120001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/1433393002/diff/120001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:153: uma_container_->UpdateHistograms(); Åsa, What is the complexity of doing this mid-call with the current implementation of RTC_HISTOGRAM_COUNTS_* ? https://codereview.webrtc.org/1433393002/diff/120001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.h (right): https://codereview.webrtc.org/1433393002/diff/120001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.h:43: const VideoEncoderConfig& encoder_config); I'd prefer VideoEncoderConfig::ContentType instead if VideoEncoderConfig, that is what's being used in SetContentType.
https://codereview.webrtc.org/1433393002/diff/120001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/1433393002/diff/120001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:153: uma_container_->UpdateHistograms(); On 2015/12/02 14:07:54, mflodman wrote: > Åsa, > What is the complexity of doing this mid-call with the current implementation of > RTC_HISTOGRAM_COUNTS_* ? I don't think there will be an issue with doing this mid-call.
Peter, Åsa: any additional comments, or should I push this? https://codereview.webrtc.org/1433393002/diff/120001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.h (right): https://codereview.webrtc.org/1433393002/diff/120001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.h:43: const VideoEncoderConfig& encoder_config); On 2015/12/02 14:07:54, mflodman wrote: > I'd prefer VideoEncoderConfig::ContentType instead if VideoEncoderConfig, that > is what's being used in SetContentType. Done.
lgtm with nits, but please remove UpdateHistograms to the uma dtor https://codereview.webrtc.org/1433393002/diff/160001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/1433393002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:41: RTC_NOTREACHED() << "Unknown content type."; Remove the default case here so we get compile errors instead when ContentTypes are added. https://codereview.webrtc.org/1433393002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:81: uma_container_->UpdateHistograms(); Maybe move this to the uma container dtor instead?
https://codereview.webrtc.org/1433393002/diff/160001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/1433393002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:41: RTC_NOTREACHED() << "Unknown content type."; On 2015/12/03 13:22:33, pbos-webrtc wrote: > Remove the default case here so we get compile errors instead when ContentTypes > are added. Done. https://codereview.webrtc.org/1433393002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:81: uma_container_->UpdateHistograms(); On 2015/12/03 13:22:33, pbos-webrtc wrote: > Maybe move this to the uma container dtor instead? Done.
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from mflodman@webrtc.org, pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1433393002/#ps180001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1433393002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1433393002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/11283)
https://codereview.webrtc.org/1433393002/diff/180001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/1433393002/diff/180001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:41: } Put RTC_NOTREACHED down here. It'll never happen still.
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org, mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/1433393002/#ps200001 (title: "Fixed compile error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1433393002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1433393002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add separate send-side UMA stats for screenshare and video. This CL duplicates all the histograms in SendStatisticsProxy. Might be overkill, but we don't know which stats will be interesting and it makes the change easier. BUG= ========== to ========== Add separate send-side UMA stats for screenshare and video. This CL duplicates all the histograms in SendStatisticsProxy. Might be overkill, but we don't know which stats will be interesting and it makes the change easier. BUG= Committed: https://crrev.com/b4a1ae5299fd57be66c7cbb7a982179bb1ecfb90 Cr-Commit-Position: refs/heads/master@{#10885} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/b4a1ae5299fd57be66c7cbb7a982179bb1ecfb90 Cr-Commit-Position: refs/heads/master@{#10885}
Message was sent while issue was closed.
https://codereview.webrtc.org/1433393002/diff/160001/webrtc/video/send_statis... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/1433393002/diff/160001/webrtc/video/send_statis... webrtc/video/send_statistics_proxy.cc:81: uma_container_->UpdateHistograms(); On 2015/12/03 13:32:55, språng wrote: > On 2015/12/03 13:22:33, pbos-webrtc wrote: > > Maybe move this to the uma container dtor instead? > > Done. I don't agree about this. We'll soon start calling UpdateHistograms multiple times during a call and I'd prefer to have this explicit in stats proxy class than implicit in uma container class.
Message was sent while issue was closed.
On 2015/12/04 08:30:52, mflodman wrote: > https://codereview.webrtc.org/1433393002/diff/160001/webrtc/video/send_statis... > File webrtc/video/send_statistics_proxy.cc (right): > > https://codereview.webrtc.org/1433393002/diff/160001/webrtc/video/send_statis... > webrtc/video/send_statistics_proxy.cc:81: uma_container_->UpdateHistograms(); > On 2015/12/03 13:32:55, språng wrote: > > On 2015/12/03 13:22:33, pbos-webrtc wrote: > > > Maybe move this to the uma container dtor instead? > > > > Done. > > I don't agree about this. > > We'll soon start calling UpdateHistograms multiple times during a call and I'd > prefer to have this explicit in stats proxy class than implicit in uma container > class. And completely remove it from the destructor? Otherwise I'd like to have it in both ~UmaContainer and as a public method. WDYT? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
