Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2319543002:
WebRTCStats added for surfacing RTCStats from WebRTC to Blink. (Closed)
|
Created:
4 years, 3 months ago by hbos_chromium Modified:
4 years, 3 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, feature-media-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebRTCStats added for surfacing RTCStats from WebRTC to Blink.
webrtc::RTCStats lives in the WebRTC repo and can't be directly
accessed from Blink. In content we implement blink::WebRTCStats and
friends to bridge this gap.
Work split up in smaller CLs. In a follow-up CL we will gather stats
in content's rtc_peer_connection_handler.cc using
webrtc::RTCStatsCollector and return the resulting stats to Blink
using [Web]RTCStats, this will include unittests.
BUG=chromium:627816
Committed: https://crrev.com/24cc79a3796ac4d1c8a7a302d6bf9a1a4203bd84
Cr-Commit-Position: refs/heads/master@{#417553}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments #
Total comments: 6
Patch Set 3 : Addressed comments, added WebRTCStatsReport #
Total comments: 6
Patch Set 4 : Addressed comments #
Messages
Total messages: 40 (26 generated)
The CQ bit was checked by hbos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by hbos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== WebRTCStats added for surfacing RTCStats from WebRTC to Blink. BUG=chromium:627816 ========== to ========== WebRTCStats added for surfacing RTCStats from WebRTC to Blink. webrtc::RTCStats lives in the WebRTC repo and can't be directly accessed from Blink. In content we implement blink::WebRTCStats to bridge this gap. Work split up in smaller CLs. In a follow-up CL we will gather stats in content's rtc_peer_connection_handler.cc using webrtc::RTCStatsCollector and return the resulting stats to Blink using [Web]RTCStats. Test code will be added when we add a GetStats in Blink. BUG=chromium:627816 ==========
hbos@chromium.org changed reviewers: + esprehn@chromium.org, perkj@chromium.org
Description was changed from ========== WebRTCStats added for surfacing RTCStats from WebRTC to Blink. webrtc::RTCStats lives in the WebRTC repo and can't be directly accessed from Blink. In content we implement blink::WebRTCStats to bridge this gap. Work split up in smaller CLs. In a follow-up CL we will gather stats in content's rtc_peer_connection_handler.cc using webrtc::RTCStatsCollector and return the resulting stats to Blink using [Web]RTCStats. Test code will be added when we add a GetStats in Blink. BUG=chromium:627816 ========== to ========== WebRTCStats added for surfacing RTCStats from WebRTC to Blink. webrtc::RTCStats lives in the WebRTC repo and can't be directly accessed from Blink. In content we implement blink::WebRTCStats to bridge this gap. Work split up in smaller CLs. In a follow-up CL we will gather stats in content's rtc_peer_connection_handler.cc using webrtc::RTCStatsCollector and return the resulting stats to Blink using [Web]RTCStats. Test code will be added when we add a getStats in Blink. BUG=chromium:627816 ==========
Description was changed from ========== WebRTCStats added for surfacing RTCStats from WebRTC to Blink. webrtc::RTCStats lives in the WebRTC repo and can't be directly accessed from Blink. In content we implement blink::WebRTCStats to bridge this gap. Work split up in smaller CLs. In a follow-up CL we will gather stats in content's rtc_peer_connection_handler.cc using webrtc::RTCStatsCollector and return the resulting stats to Blink using [Web]RTCStats. Test code will be added when we add a getStats in Blink. BUG=chromium:627816 ========== to ========== WebRTCStats added for surfacing RTCStats from WebRTC to Blink. webrtc::RTCStats lives in the WebRTC repo and can't be directly accessed from Blink. In content we implement blink::WebRTCStats to bridge this gap. Work split up in smaller CLs. In a follow-up CL we will gather stats in content's rtc_peer_connection_handler.cc using webrtc::RTCStatsCollector and return the resulting stats to Blink using [Web]RTCStats, this will include unittests. BUG=chromium:627816 ==========
Please take a look, esprehn and perkj.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2319543002/diff/60001/content/renderer/BUILD.gn File content/renderer/BUILD.gn (right): https://codereview.chromium.org/2319543002/diff/60001/content/renderer/BUILD.... content/renderer/BUILD.gn:592: "media/rtc_stats.cc", these files depend on webrtc third_party? Then please move to media/webrtc https://codereview.chromium.org/2319543002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_stats.cc (right): https://codereview.chromium.org/2319543002/diff/60001/content/renderer/media/... content/renderer/media/rtc_stats.cc:14: double kMillisecondsPerMicrosecond = 0.001; I am sure this const exist in a number of places. use the one in chrome. Here is one in webrtc.. https://cs.chromium.org/chromium/src/third_party/webrtc/base/timeutils.h?rcl=... https://codereview.chromium.org/2319543002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_stats.h (right): https://codereview.chromium.org/2319543002/diff/60001/content/renderer/media/... content/renderer/media/rtc_stats.h:27: std::vector<const webrtc::RTCStatsMemberInterface*> members_; Can you add a comment about ownership of members? https://codereview.chromium.org/2319543002/diff/60001/content/renderer/media/... content/renderer/media/rtc_stats.h:53: const webrtc::RTCStatsMemberInterface* member_; comment on lifetime of member_ ? https://codereview.chromium.org/2319543002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebRTCStats.h (right): https://codereview.chromium.org/2319543002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebRTCStats.h:19: // Corresponds to |webrtc::RTCStatsMemberInterface::Type| in WebRTC. Can you really refer to webrtc:: in blink?
Patchset #2 (id:80001) has been deleted
Please take (another) look perkj, esprehn https://codereview.chromium.org/2319543002/diff/60001/content/renderer/BUILD.gn File content/renderer/BUILD.gn (right): https://codereview.chromium.org/2319543002/diff/60001/content/renderer/BUILD.... content/renderer/BUILD.gn:592: "media/rtc_stats.cc", On 2016/09/07 14:34:36, perkj_chrome wrote: > these files depend on webrtc third_party? Then please move to media/webrtc Done. https://codereview.chromium.org/2319543002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_stats.cc (right): https://codereview.chromium.org/2319543002/diff/60001/content/renderer/media/... content/renderer/media/rtc_stats.cc:14: double kMillisecondsPerMicrosecond = 0.001; On 2016/09/07 14:34:36, perkj_chrome wrote: > I am sure this const exist in a number of places. use the one in chrome. > > Here is one in webrtc.. > https://cs.chromium.org/chromium/src/third_party/webrtc/base/timeutils.h?rcl=... Done. https://codereview.chromium.org/2319543002/diff/60001/content/renderer/media/... File content/renderer/media/rtc_stats.h (right): https://codereview.chromium.org/2319543002/diff/60001/content/renderer/media/... content/renderer/media/rtc_stats.h:27: std::vector<const webrtc::RTCStatsMemberInterface*> members_; On 2016/09/07 14:34:36, perkj_chrome wrote: > Can you add a comment about ownership of members? Done. Added comment in WebRTCStats.h too. https://codereview.chromium.org/2319543002/diff/60001/content/renderer/media/... content/renderer/media/rtc_stats.h:53: const webrtc::RTCStatsMemberInterface* member_; On 2016/09/07 14:34:36, perkj_chrome wrote: > comment on lifetime of member_ ? Done. https://codereview.chromium.org/2319543002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/WebRTCStats.h (right): https://codereview.chromium.org/2319543002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/WebRTCStats.h:19: // Corresponds to |webrtc::RTCStatsMemberInterface::Type| in WebRTC. On 2016/09/07 14:34:36, perkj_chrome wrote: > Can you really refer to webrtc:: in blink? Dunno, removed comment.
What's the plan for moving the content/renderer/media code into blink? This patch is expanding the Web* api surface further. In general we're trying to make it smaller instead. https://codereview.chromium.org/2319543002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebRTCStats.h (right): https://codereview.chromium.org/2319543002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebRTCStats.h:47: // |WebRTCStatsMember| created by a |WebRTCStats| object that has been destroyed. This sounds pretty suspect, do you want a RefPtr instead ? https://codereview.chromium.org/2319543002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebRTCStats.h:65: virtual const std::vector<uint32_t>& valueSequenceUint32() const = 0; Blink doesn't use std::vector, this would need to be WebVector.
Hang on, I want to add a WebRTCStatsReport in this same CL instead of in a follow-up, I'll ping when I've updated it.
Patchset #3 (id:120001) has been deleted
Description was changed from ========== WebRTCStats added for surfacing RTCStats from WebRTC to Blink. webrtc::RTCStats lives in the WebRTC repo and can't be directly accessed from Blink. In content we implement blink::WebRTCStats to bridge this gap. Work split up in smaller CLs. In a follow-up CL we will gather stats in content's rtc_peer_connection_handler.cc using webrtc::RTCStatsCollector and return the resulting stats to Blink using [Web]RTCStats, this will include unittests. BUG=chromium:627816 ========== to ========== WebRTCStats added for surfacing RTCStats from WebRTC to Blink. webrtc::RTCStats lives in the WebRTC repo and can't be directly accessed from Blink. In content we implement blink::WebRTCStats and friends to bridge this gap. Work split up in smaller CLs. In a follow-up CL we will gather stats in content's rtc_peer_connection_handler.cc using webrtc::RTCStatsCollector and return the resulting stats to Blink using [Web]RTCStats, this will include unittests. BUG=chromium:627816 ==========
The CQ bit was checked by hbos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/08 09:19:48, esprehn wrote: > What's the plan for moving the content/renderer/media code into blink? This > patch is expanding the Web* api surface further. In general we're trying to make > it smaller instead. I don't know. Are there plans for moving content/renderer/media into blink? Seems like a massive amount of work. It would be nice if blink could include directly from webrtc, that would probably allow getting rid of many bridges like this. Not sure if there is a good reason for not being allowed to do that now that WebKit is not a separate repo from chromium. https://codereview.chromium.org/2319543002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebRTCStats.h (right): https://codereview.chromium.org/2319543002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebRTCStats.h:47: // |WebRTCStatsMember| created by a |WebRTCStats| object that has been destroyed. On 2016/09/08 09:19:47, esprehn wrote: > This sounds pretty suspect, do you want a RefPtr instead ? I'm able to avoid this problem by letting the implementations reference the thing that is owning the data is is pointing to. But speaking of ref counting, I don't think WTF's RefPtr and RefCounted is allowed in WebKit/public/platform for inclusion outside of Blink? And base/memory/ref_counted.h is not allowed in Blink. I'm under the impression that there isn't anything ref counted for bridging Blink and content, do any of you know? https://codereview.chromium.org/2319543002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebRTCStats.h:65: virtual const std::vector<uint32_t>& valueSequenceUint32() const = 0; On 2016/09/08 09:19:47, esprehn wrote: > Blink doesn't use std::vector, this would need to be WebVector. Done. Also changed const char* and std::string to WebString.
On 2016/09/08 12:17:09, hbos_chromium wrote: > On 2016/09/08 09:19:48, esprehn wrote: > > What's the plan for moving the content/renderer/media code into blink? This > > patch is expanding the Web* api surface further. In general we're trying to > make > > it smaller instead. > > I don't know. > Are there plans for moving content/renderer/media into blink? Seems like a > massive amount of work. > It would be nice if blink could include directly from webrtc, that would > probably allow getting rid of many bridges like this. Not sure if there is a > good reason for not being allowed to do that now that WebKit is not a separate > repo from chromium. > > https://codereview.chromium.org/2319543002/diff/100001/third_party/WebKit/pub... > File third_party/WebKit/public/platform/WebRTCStats.h (right): > > https://codereview.chromium.org/2319543002/diff/100001/third_party/WebKit/pub... > third_party/WebKit/public/platform/WebRTCStats.h:47: // |WebRTCStatsMember| > created by a |WebRTCStats| object that has been destroyed. > On 2016/09/08 09:19:47, esprehn wrote: > > This sounds pretty suspect, do you want a RefPtr instead ? > > I'm able to avoid this problem by letting the implementations reference the > thing that is owning the data is is pointing to. > > But speaking of ref counting, I don't think WTF's RefPtr and RefCounted is > allowed in WebKit/public/platform for inclusion outside of Blink? And > base/memory/ref_counted.h is not allowed in Blink. I'm under the impression that > there isn't anything ref counted for bridging Blink and content, do any of you > know? > > https://codereview.chromium.org/2319543002/diff/100001/third_party/WebKit/pub... > third_party/WebKit/public/platform/WebRTCStats.h:65: virtual const > std::vector<uint32_t>& valueSequenceUint32() const = 0; > On 2016/09/08 09:19:47, esprehn wrote: > > Blink doesn't use std::vector, this would need to be WebVector. > > Done. Also changed const char* and std::string to WebString. PTAL perkj, esprehn.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This lgtm, but media team does need to come up with a plan to move content/renderer/media into blink. content/renderer is going away, we've already moved tons of stuff out. See Onion Soup. :) https://codereview.chromium.org/2319543002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebRTCStats.h (right): https://codereview.chromium.org/2319543002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebRTCStats.h:47: // |WebRTCStatsMember| created by a |WebRTCStats| object that has been destroyed. On 2016/09/08 at 12:17:09, hbos_chromium wrote: > On 2016/09/08 09:19:47, esprehn wrote: > > This sounds pretty suspect, do you want a RefPtr instead ? > > I'm able to avoid this problem by letting the implementations reference the thing that is owning the data is is pointing to. > > But speaking of ref counting, I don't think WTF's RefPtr and RefCounted is allowed in WebKit/public/platform for inclusion outside of Blink? And base/memory/ref_counted.h is not allowed in Blink. I'm under the impression that there isn't anything ref counted for bridging Blink and content, do any of you know? That's what WebPrivatePtr does, if your type is RefCounted it'll handle the refs for you.
PTAL perkj https://codereview.chromium.org/2319543002/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebRTCStats.h (right): https://codereview.chromium.org/2319543002/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebRTCStats.h:47: // |WebRTCStatsMember| created by a |WebRTCStats| object that has been destroyed. On 2016/09/08 22:41:39, esprehn wrote: > On 2016/09/08 at 12:17:09, hbos_chromium wrote: > > On 2016/09/08 09:19:47, esprehn wrote: > > > This sounds pretty suspect, do you want a RefPtr instead ? > > > > I'm able to avoid this problem by letting the implementations reference the > thing that is owning the data is is pointing to. > > > > But speaking of ref counting, I don't think WTF's RefPtr and RefCounted is > allowed in WebKit/public/platform for inclusion outside of Blink? And > base/memory/ref_counted.h is not allowed in Blink. I'm under the impression that > there isn't anything ref counted for bridging Blink and content, do any of you > know? > > That's what WebPrivatePtr does, if your type is RefCounted it'll handle the refs > for you. Good to know!
lgtm if the comments are fixed. https://codereview.chromium.org/2319543002/diff/140001/content/renderer/media... File content/renderer/media/webrtc/rtc_stats.h (right): https://codereview.chromium.org/2319543002/diff/140001/content/renderer/media... content/renderer/media/webrtc/rtc_stats.h:43: // Referencing the owning report protects |stats_| from destruction. owning report protects ? https://codereview.chromium.org/2319543002/diff/140001/content/renderer/media... content/renderer/media/webrtc/rtc_stats.h:47: const std::vector<const webrtc::RTCStatsMemberInterface*> stats_members_; and a comment about stats_members_? https://codereview.chromium.org/2319543002/diff/140001/content/renderer/media... content/renderer/media/webrtc/rtc_stats.h:73: // Referencing the owning report protects |member_| from destruction. dito
https://codereview.chromium.org/2319543002/diff/140001/content/renderer/media... File content/renderer/media/webrtc/rtc_stats.h (right): https://codereview.chromium.org/2319543002/diff/140001/content/renderer/media... content/renderer/media/webrtc/rtc_stats.h:43: // Referencing the owning report protects |stats_| from destruction. On 2016/09/09 07:10:40, perkj_chrome wrote: > owning report protects ? Done. https://codereview.chromium.org/2319543002/diff/140001/content/renderer/media... content/renderer/media/webrtc/rtc_stats.h:47: const std::vector<const webrtc::RTCStatsMemberInterface*> stats_members_; On 2016/09/09 07:10:40, perkj_chrome wrote: > and a comment about stats_members_? Done. https://codereview.chromium.org/2319543002/diff/140001/content/renderer/media... content/renderer/media/webrtc/rtc_stats.h:73: // Referencing the owning report protects |member_| from destruction. On 2016/09/09 07:10:40, perkj_chrome wrote: > dito Done.
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/2319543002/#ps160001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== WebRTCStats added for surfacing RTCStats from WebRTC to Blink. webrtc::RTCStats lives in the WebRTC repo and can't be directly accessed from Blink. In content we implement blink::WebRTCStats and friends to bridge this gap. Work split up in smaller CLs. In a follow-up CL we will gather stats in content's rtc_peer_connection_handler.cc using webrtc::RTCStatsCollector and return the resulting stats to Blink using [Web]RTCStats, this will include unittests. BUG=chromium:627816 ========== to ========== WebRTCStats added for surfacing RTCStats from WebRTC to Blink. webrtc::RTCStats lives in the WebRTC repo and can't be directly accessed from Blink. In content we implement blink::WebRTCStats and friends to bridge this gap. Work split up in smaller CLs. In a follow-up CL we will gather stats in content's rtc_peer_connection_handler.cc using webrtc::RTCStatsCollector and return the resulting stats to Blink using [Web]RTCStats, this will include unittests. BUG=chromium:627816 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== WebRTCStats added for surfacing RTCStats from WebRTC to Blink. webrtc::RTCStats lives in the WebRTC repo and can't be directly accessed from Blink. In content we implement blink::WebRTCStats and friends to bridge this gap. Work split up in smaller CLs. In a follow-up CL we will gather stats in content's rtc_peer_connection_handler.cc using webrtc::RTCStatsCollector and return the resulting stats to Blink using [Web]RTCStats, this will include unittests. BUG=chromium:627816 ========== to ========== WebRTCStats added for surfacing RTCStats from WebRTC to Blink. webrtc::RTCStats lives in the WebRTC repo and can't be directly accessed from Blink. In content we implement blink::WebRTCStats and friends to bridge this gap. Work split up in smaller CLs. In a follow-up CL we will gather stats in content's rtc_peer_connection_handler.cc using webrtc::RTCStatsCollector and return the resulting stats to Blink using [Web]RTCStats, this will include unittests. BUG=chromium:627816 Committed: https://crrev.com/24cc79a3796ac4d1c8a7a302d6bf9a1a4203bd84 Cr-Commit-Position: refs/heads/master@{#417553} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/24cc79a3796ac4d1c8a7a302d6bf9a1a4203bd84 Cr-Commit-Position: refs/heads/master@{#417553} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
