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

Issue 3009793002: Implement googContentType GetStats metric reported on receive side. (Closed)

Created:
3 years, 3 months ago by ilnik
Modified:
3 years, 3 months ago
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

Implement googContentType GetStats metric reported on receive side. Reported per video stream as a string. BUG=webrtc:8174 Review-Url: https://codereview.webrtc.org/3009793002 Cr-Commit-Position: refs/heads/master@{#19667} Committed: https://chromium.googlesource.com/external/webrtc/+/2e1b40bdf63743b07388628707c4b8e349495807

Patch Set 1 #

Total comments: 2

Patch Set 2 : fix identation #

Patch Set 3 : rebase #

Patch Set 4 : rebase #

Patch Set 5 : Fix ce #

Total comments: 16

Patch Set 6 : Implement Tommi@ comments #

Patch Set 7 : Change content_type to be enum until the latest possible point #

Patch Set 8 : Fix uninitialized variable error #

Patch Set 9 : Fix uninitialized value error pass 2 #

Total comments: 2

Patch Set 10 : Implement Tommi@ comments #

Patch Set 11 : Rebase #

Patch Set 12 : Fix broken tests on ASAN #

Unified diffs Side-by-side diffs Delta from patch set Stats (+39 lines, -1 line) Patch
M webrtc/api/statstypes.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +2 lines, -1 line 0 comments Download
M webrtc/api/statstypes.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/api/video/video_content_type.h View 1 2 3 4 5 6 7 8 9 2 chunks +4 lines, -0 lines 0 comments Download
M webrtc/api/video/video_content_type.cc View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M webrtc/call/video_receive_stream.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/media/base/mediachannel.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -0 lines 0 comments Download
M webrtc/media/engine/webrtcvideoengine.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/pc/statscollector.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -0 lines 0 comments Download
M webrtc/video/receive_statistics_proxy.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/video/receive_statistics_proxy_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +15 lines, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 64 (39 generated)
ilnik
Hello, please review it in Wednesday morning, as we really want this metric landed before ...
3 years, 3 months ago (2017-08-29 16:32:12 UTC) #2
sprang_webrtc
lgtm https://codereview.webrtc.org/3009793002/diff/1/webrtc/pc/statscollector.cc File webrtc/pc/statscollector.cc (right): https://codereview.webrtc.org/3009793002/diff/1/webrtc/pc/statscollector.cc#newcode274 webrtc/pc/statscollector.cc:274: info.content_type); nit: indentation
3 years, 3 months ago (2017-08-29 16:38:13 UTC) #3
ilnik
https://codereview.webrtc.org/3009793002/diff/1/webrtc/pc/statscollector.cc File webrtc/pc/statscollector.cc (right): https://codereview.webrtc.org/3009793002/diff/1/webrtc/pc/statscollector.cc#newcode274 webrtc/pc/statscollector.cc:274: info.content_type); On 2017/08/29 16:38:13, sprang_webrtc wrote: > nit: indentation ...
3 years, 3 months ago (2017-08-29 16:44:09 UTC) #4
ilnik
3 years, 3 months ago (2017-08-29 16:44:10 UTC) #5
Taylor Brandstetter
lgtm
3 years, 3 months ago (2017-08-29 16:53:47 UTC) #6
ilnik
On 2017/08/29 16:53:47, Taylor Brandstetter wrote: > lgtm Thank you for very fast review!
3 years, 3 months ago (2017-08-29 16:54:56 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/3009793002/60001
3 years, 3 months ago (2017-08-30 10:33:11 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/builds/7393) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, ...
3 years, 3 months ago (2017-08-30 10:36:20 UTC) #15
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/3009793002/80001
3 years, 3 months ago (2017-08-30 10:40:38 UTC) #18
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/20965)
3 years, 3 months ago (2017-08-30 10:45:29 UTC) #20
tommi
https://codereview.webrtc.org/3009793002/diff/80001/webrtc/api/statstypes.cc File webrtc/api/statstypes.cc (right): https://codereview.webrtc.org/3009793002/diff/80001/webrtc/api/statstypes.cc#newcode523 webrtc/api/statstypes.cc:523: case kStatsValueNameContentType: see for example where kStatsValueNameCurrentDelayMs is, in ...
3 years, 3 months ago (2017-08-30 15:03:28 UTC) #28
ilnik
https://codereview.webrtc.org/3009793002/diff/80001/webrtc/api/statstypes.cc File webrtc/api/statstypes.cc (right): https://codereview.webrtc.org/3009793002/diff/80001/webrtc/api/statstypes.cc#newcode523 webrtc/api/statstypes.cc:523: case kStatsValueNameContentType: On 2017/08/30 15:03:27, tommi wrote: > see ...
3 years, 3 months ago (2017-08-30 15:30:25 UTC) #29
tommi
https://codereview.webrtc.org/3009793002/diff/80001/webrtc/call/video_receive_stream.h File webrtc/call/video_receive_stream.h (right): https://codereview.webrtc.org/3009793002/diff/80001/webrtc/call/video_receive_stream.h#newcode90 webrtc/call/video_receive_stream.h:90: // "screen" vs "realtime". On 2017/08/30 15:30:24, ilnik wrote: ...
3 years, 3 months ago (2017-08-30 17:54:43 UTC) #30
ilnik
https://codereview.webrtc.org/3009793002/diff/80001/webrtc/call/video_receive_stream.h File webrtc/call/video_receive_stream.h (right): https://codereview.webrtc.org/3009793002/diff/80001/webrtc/call/video_receive_stream.h#newcode90 webrtc/call/video_receive_stream.h:90: // "screen" vs "realtime". On 2017/08/30 17:54:42, tommi wrote: ...
3 years, 3 months ago (2017-08-30 18:12:18 UTC) #31
tommi
https://codereview.webrtc.org/3009793002/diff/80001/webrtc/call/video_receive_stream.h File webrtc/call/video_receive_stream.h (right): https://codereview.webrtc.org/3009793002/diff/80001/webrtc/call/video_receive_stream.h#newcode90 webrtc/call/video_receive_stream.h:90: // "screen" vs "realtime". On 2017/08/30 18:12:18, ilnik wrote: ...
3 years, 3 months ago (2017-08-31 07:28:48 UTC) #32
ilnik
https://codereview.webrtc.org/3009793002/diff/80001/webrtc/call/video_receive_stream.h File webrtc/call/video_receive_stream.h (right): https://codereview.webrtc.org/3009793002/diff/80001/webrtc/call/video_receive_stream.h#newcode90 webrtc/call/video_receive_stream.h:90: // "screen" vs "realtime". On 2017/08/31 07:28:48, tommi wrote: ...
3 years, 3 months ago (2017-08-31 07:51:11 UTC) #33
ilnik
tommi, PTAL, it's done as you suggested now. New stat is propagated in GetStats as ...
3 years, 3 months ago (2017-09-04 07:38:22 UTC) #42
tommi
sorry for the delay - one last request https://codereview.webrtc.org/3009793002/diff/160001/webrtc/pc/statscollector.cc File webrtc/pc/statscollector.cc (right): https://codereview.webrtc.org/3009793002/diff/160001/webrtc/pc/statscollector.cc#newcode276 webrtc/pc/statscollector.cc:276: webrtc::videocontenttypehelpers::ToString(info.content_type)); ...
3 years, 3 months ago (2017-09-04 09:27:43 UTC) #47
ilnik
https://codereview.webrtc.org/3009793002/diff/160001/webrtc/pc/statscollector.cc File webrtc/pc/statscollector.cc (right): https://codereview.webrtc.org/3009793002/diff/160001/webrtc/pc/statscollector.cc#newcode276 webrtc/pc/statscollector.cc:276: webrtc::videocontenttypehelpers::ToString(info.content_type)); On 2017/09/04 09:27:42, tommi wrote: > since AddString ...
3 years, 3 months ago (2017-09-04 09:41:07 UTC) #48
tommi
lgtm
3 years, 3 months ago (2017-09-04 10:57:51 UTC) #49
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/3009793002/180001
3 years, 3 months ago (2017-09-04 10:58:24 UTC) #52
commit-bot: I haz the power
Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/24680)
3 years, 3 months ago (2017-09-04 11:02:30 UTC) #54
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/3009793002/200001
3 years, 3 months ago (2017-09-04 11:17:40 UTC) #57
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/3009793002/220001
3 years, 3 months ago (2017-09-04 12:39:21 UTC) #61
commit-bot: I haz the power
3 years, 3 months ago (2017-09-04 14:57:29 UTC) #64
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/external/webrtc/+/2e1b40bdf63743b0738862870...

Powered by Google App Engine
This is Rietveld 408576698