Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1546)

Issue 3008983002: Change reporting of timing frames conditions in GetStats on receive side (Closed)

Created:
3 years, 3 months ago by ilnik
Modified:
3 years, 3 months ago
Reviewers:
tommi, sprang_webrtc
CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, kwiberg-webrtc, the sun, mflodman
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Change reporting of timing frames conditions in GetStats on receive side Instead of the longest frame since the last GetStats call, the longest frame received during last 10 seconds should be returned in GetStats(). Previous way is not a good idea because there are maybe several consumers of GetStats calls. If not all of them parse timing frame reports, some of them may be lost. Also, streamline reporting of TimingFrames via GetStats (remove separate methods and use VideoReceiveStream::Stats struct instead). BUG=webrtc:7594 Review-Url: https://codereview.webrtc.org/3008983002 Cr-Commit-Position: refs/heads/master@{#19650} Committed: https://chromium.googlesource.com/external/webrtc/+/75204c5ccdda561ccffbfa3a8fa94feac8e19e30

Patch Set 1 #

Patch Set 2 : Fix timing frames ToString implementation #

Total comments: 4

Patch Set 3 : Implement Sprang@ comments #

Total comments: 6

Patch Set 4 : Implement Tommi@ comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -56 lines) Patch
M webrtc/api/video/video_timing.h View 1 2 3 1 chunk +4 lines, -0 lines 0 comments Download
M webrtc/api/video/video_timing.cc View 1 2 3 2 chunks +11 lines, -3 lines 0 comments Download
M webrtc/call/video_receive_stream.h View 2 chunks +4 lines, -2 lines 0 comments Download
M webrtc/media/engine/fakewebrtccall.h View 1 chunk +0 lines, -2 lines 0 comments Download
M webrtc/media/engine/fakewebrtccall.cc View 1 chunk +0 lines, -5 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine.cc View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/video/end_to_end_tests.cc View 3 chunks +3 lines, -9 lines 0 comments Download
M webrtc/video/receive_statistics_proxy.h View 1 2 2 chunks +4 lines, -3 lines 0 comments Download
M webrtc/video/receive_statistics_proxy.cc View 3 chunks +5 lines, -18 lines 0 comments Download
M webrtc/video/receive_statistics_proxy_unittest.cc View 1 2 2 chunks +10 lines, -6 lines 0 comments Download
M webrtc/video/video_receive_stream.h View 1 chunk +0 lines, -2 lines 0 comments Download
M webrtc/video/video_receive_stream.cc View 1 chunk +0 lines, -5 lines 0 comments Download

Messages

Total messages: 28 (17 generated)
ilnik
3 years, 3 months ago (2017-08-31 09:34:12 UTC) #4
sprang_webrtc
https://codereview.webrtc.org/3008983002/diff/20001/webrtc/video/receive_statistics_proxy.h File webrtc/video/receive_statistics_proxy.h (right): https://codereview.webrtc.org/3008983002/diff/20001/webrtc/video/receive_statistics_proxy.h#newcode169 webrtc/video/receive_statistics_proxy.h:169: GUARDED_BY(&crit_); uhg. mutable :( If it's needed, can you ...
3 years, 3 months ago (2017-08-31 13:24:43 UTC) #5
ilnik
https://codereview.webrtc.org/3008983002/diff/20001/webrtc/video/receive_statistics_proxy.h File webrtc/video/receive_statistics_proxy.h (right): https://codereview.webrtc.org/3008983002/diff/20001/webrtc/video/receive_statistics_proxy.h#newcode169 webrtc/video/receive_statistics_proxy.h:169: GUARDED_BY(&crit_); On 2017/08/31 13:24:43, sprang_webrtc wrote: > uhg. mutable ...
3 years, 3 months ago (2017-08-31 13:32:59 UTC) #6
sprang_webrtc
lgtm
3 years, 3 months ago (2017-09-01 09:15:40 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/3008983002/40001
3 years, 3 months ago (2017-09-01 09:16:38 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/21083)
3 years, 3 months ago (2017-09-01 09:21:55 UTC) #11
ilnik
On 2017/09/01 09:21:55, commit-bot: I haz the power wrote: > Try jobs failed on following ...
3 years, 3 months ago (2017-09-01 09:41:07 UTC) #12
tommi
lgtm https://codereview.webrtc.org/3008983002/diff/40001/webrtc/api/video/video_timing.cc File webrtc/api/video/video_timing.cc (right): https://codereview.webrtc.org/3008983002/diff/40001/webrtc/api/video/video_timing.cc#newcode49 webrtc/api/video/video_timing.cc:49: nit: fix whitespace https://codereview.webrtc.org/3008983002/diff/40001/webrtc/api/video/video_timing.h File webrtc/api/video/video_timing.h (right): https://codereview.webrtc.org/3008983002/diff/40001/webrtc/api/video/video_timing.h#newcode92 ...
3 years, 3 months ago (2017-09-04 09:09:21 UTC) #21
ilnik
https://codereview.webrtc.org/3008983002/diff/40001/webrtc/api/video/video_timing.cc File webrtc/api/video/video_timing.cc (right): https://codereview.webrtc.org/3008983002/diff/40001/webrtc/api/video/video_timing.cc#newcode49 webrtc/api/video/video_timing.cc:49: On 2017/09/04 09:09:21, tommi wrote: > nit: fix whitespace ...
3 years, 3 months ago (2017-09-04 09:18:55 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/3008983002/60001
3 years, 3 months ago (2017-09-04 09:20:07 UTC) #25
commit-bot: I haz the power
3 years, 3 months ago (2017-09-04 10:35:46 UTC) #28
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/75204c5ccdda561ccffbfa3a8...

Powered by Google App Engine
This is Rietveld 408576698