Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 3 months ago by fippo1 Modified:
4 years, 10 months ago Reviewers:
tommi, hta - Chromium, tommi (sloooow) - chröme CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
Descriptionhttps://github.com/w3c/webrtc-stats/pull/10/files added mediaType to the tracks. The closest in the current stats is the ssrc type.
This is somewhat easier than looking up the media type by iterating pc.getLocalStreams / pc.getRemoteStreams and all tracks. Temporary until stats get revamped to conform to the spec obviously.
BUG=webrtc:4117
Committed: https://crrev.com/bec70ab0fd3613c532359a849bfbc67749da6d24
Cr-Commit-Position: refs/heads/master@{#11412}
Patch Set 1 #Patch Set 2 : add tests #
Total comments: 1
Patch Set 3 : rebase #
Created: 4 years, 11 months ago
Messages
Total messages: 32 (13 generated)
fippo@andyet.net changed reviewers: + hta@chromium.org
lgtm
hta@chromium.org changed reviewers: + tommi@chromium.org
Needs LGTM from an OWNER.
ping - Tommi, can you LGTM this? fippo, is it still up to date?
hta: still merges without conflicts
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
added a couple of comments. I have a proposal for doing this slightly differently and since strings have been a problem here, I'd like to avoid adding them (and I don't think we need it in this case): * Add a new enum, MediaType, to StatsReport. Presumably with three possible values, kAudio, kVideo, kData. * Change the StatsCollection::*New(...) methods to require the media type enum. * Add a private const member variable, media_type_, to StatsReport and a const getter. This will require matching changes in Chrome rtc_peer_connection_handler.cc and peer_connection_tracker.cc. This is a lot more involved of course than the current CL, but I'd rather not go back down the route of allocating strings when we don't really need to. I'm happy to make all of these changes btw. --- I'm leaving my comments below in, but they don't really apply if we go with the above proposal https://codereview.webrtc.org/1307633007/diff/20001/talk/app/webrtc/statscoll... File talk/app/webrtc/statscollector.cc (right): https://codereview.webrtc.org/1307633007/diff/20001/talk/app/webrtc/statscoll... talk/app/webrtc/statscollector.cc:178: report->AddString(StatsReport::kStatsValueNameMediaType, "audio"); Can you move this to the top of the function? If you could also add a TODO for me: // TODO(tommi): Look into setting the media type of a report when the report is first created. Same below. The reason is that now we're setting this in several places while it should be in only one place. Secondly we're setting it each time the report is updated but this value should never change.
On 2016/01/11 12:22:50, tommi-webrtc wrote: > added a couple of comments. I have a proposal for doing this slightly > differently and since strings have been a problem here, I'd like to avoid adding > them (and I don't think we need it in this case): > > * Add a new enum, MediaType, to StatsReport. Presumably with three possible > values, kAudio, kVideo, kData. > * Change the StatsCollection::*New(...) methods to require the media type enum. > * Add a private const member variable, media_type_, to StatsReport and a const > getter. > > This will require matching changes in Chrome rtc_peer_connection_handler.cc and > peer_connection_tracker.cc. > > This is a lot more involved of course than the current CL, but I'd rather not go > back down the route of allocating strings when we don't really need to. I'm > happy to make all of these changes btw. > > --- > I'm leaving my comments below in, but they don't really apply if we go with the > above proposal > > https://codereview.webrtc.org/1307633007/diff/20001/talk/app/webrtc/statscoll... > File talk/app/webrtc/statscollector.cc (right): > > https://codereview.webrtc.org/1307633007/diff/20001/talk/app/webrtc/statscoll... > talk/app/webrtc/statscollector.cc:178: > report->AddString(StatsReport::kStatsValueNameMediaType, "audio"); > Can you move this to the top of the function? > > If you could also add a TODO for me: > // TODO(tommi): Look into setting the media type of a report when the report is > first created. > > Same below. The reason is that now we're setting this in several places while > it should be in only one place. Secondly we're setting it each time the report > is updated but this value should never change. (replying in the interest of landing this one - I think users need it) enums look good to me - this will require adding an AddMediaType function. But I don't think they should be on StatsReport at ceate time, because there are many StatsReports (and will be more) that don't associate to media types. Adding them to the SSRC report is OK to me in the short run.
On 2016/01/21 11:21:57, hta - Chromium wrote: > On 2016/01/11 12:22:50, tommi-webrtc wrote: > > added a couple of comments. I have a proposal for doing this slightly > > differently and since strings have been a problem here, I'd like to avoid > adding > > them (and I don't think we need it in this case): > > > > * Add a new enum, MediaType, to StatsReport. Presumably with three possible > > values, kAudio, kVideo, kData. > > * Change the StatsCollection::*New(...) methods to require the media type > enum. > > * Add a private const member variable, media_type_, to StatsReport and a const > > getter. > > > > This will require matching changes in Chrome rtc_peer_connection_handler.cc > and > > peer_connection_tracker.cc. > > > > This is a lot more involved of course than the current CL, but I'd rather not > go > > back down the route of allocating strings when we don't really need to. I'm > > happy to make all of these changes btw. > > > > --- > > I'm leaving my comments below in, but they don't really apply if we go with > the > > above proposal > > > > > https://codereview.webrtc.org/1307633007/diff/20001/talk/app/webrtc/statscoll... > > File talk/app/webrtc/statscollector.cc (right): > > > > > https://codereview.webrtc.org/1307633007/diff/20001/talk/app/webrtc/statscoll... > > talk/app/webrtc/statscollector.cc:178: > > report->AddString(StatsReport::kStatsValueNameMediaType, "audio"); > > Can you move this to the top of the function? > > > > If you could also add a TODO for me: > > // TODO(tommi): Look into setting the media type of a report when the report > is > > first created. > > > > Same below. The reason is that now we're setting this in several places while > > it should be in only one place. Secondly we're setting it each time the report > > is updated but this value should never change. > > (replying in the interest of landing this one - I think users need it) > > enums look good to me - this will require adding an AddMediaType function. But I > don't think they should be on StatsReport at ceate time, because there are many > StatsReports (and will be more) that don't associate to media types. Adding them > to the SSRC report is OK to me in the short run. OK, cool. Sounds like this is a reasonable approach to begin with and I can follow up on it. Lgtm. landing.
The CQ bit was checked by tommi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1307633007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1307633007/20001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by tommi@webrtc.org
lgtm oops... from the right account this time.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1307633007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1307633007/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/4662) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/4658) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/6981) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/12149) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/329)
fippo - can you rebase?
On 2016/01/21 11:52:20, tommi-webrtc wrote: > fippo - can you rebase? done -- hopefully got it right. Minor merge conflict.
The CQ bit was checked by hta@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, hta@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1307633007/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1307633007/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1307633007/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
The CQ bit was checked by hta@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1307633007/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1307633007/40001
Message was sent while issue was closed.
Description was changed from ========== https://github.com/w3c/webrtc-stats/pull/10/files added mediaType to the tracks. The closest in the current stats is the ssrc type. This is somewhat easier than looking up the media type by iterating pc.getLocalStreams / pc.getRemoteStreams and all tracks. Temporary until stats get revamped to conform to the spec obviously. BUG=webrtc:4117 ========== to ========== https://github.com/w3c/webrtc-stats/pull/10/files added mediaType to the tracks. The closest in the current stats is the ssrc type. This is somewhat easier than looking up the media type by iterating pc.getLocalStreams / pc.getRemoteStreams and all tracks. Temporary until stats get revamped to conform to the spec obviously. BUG=webrtc:4117 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== https://github.com/w3c/webrtc-stats/pull/10/files added mediaType to the tracks. The closest in the current stats is the ssrc type. This is somewhat easier than looking up the media type by iterating pc.getLocalStreams / pc.getRemoteStreams and all tracks. Temporary until stats get revamped to conform to the spec obviously. BUG=webrtc:4117 ========== to ========== https://github.com/w3c/webrtc-stats/pull/10/files added mediaType to the tracks. The closest in the current stats is the ssrc type. This is somewhat easier than looking up the media type by iterating pc.getLocalStreams / pc.getRemoteStreams and all tracks. Temporary until stats get revamped to conform to the spec obviously. BUG=webrtc:4117 Committed: https://crrev.com/bec70ab0fd3613c532359a849bfbc67749da6d24 Cr-Commit-Position: refs/heads/master@{#11412} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/bec70ab0fd3613c532359a849bfbc67749da6d24 Cr-Commit-Position: refs/heads/master@{#11412} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
