Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2467873005:
RTCMediaStream[Track]Stats added. (Closed)
DescriptionRTCMediaStream[Track]Stats added.
Not all members are collected by RTCStatsCollector and detached tracks
are not visible in the returned stats. This needs to be addressed before
closing crbug.com/660827 and crbug.com/659137
BUG=chromium:660827, chromium:659137, chromium:627816
Committed: https://crrev.com/09bc128603909680a3d9d09100d9be7d3f008036
Cr-Commit-Position: refs/heads/master@{#14978}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Addressed some of hta's comments #
Total comments: 8
Patch Set 3 : Rebase with master #Patch Set 4 : Addressed comments #
Messages
Total messages: 21 (7 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
hbos@webrtc.org changed reviewers: + deadbeef@webrtc.org, hta@webrtc.org
Please take a look, deadbeef and hta. https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:307: video_track_source_stats.input_height); I wonder if this is the right value. What if the source is a HD video but the output is downscaled, which value should the track have? https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/stats/rtcstats... File webrtc/api/stats/rtcstats_objects.h (right): https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/stats/rtcstats... webrtc/api/stats/rtcstats_objects.h:219: RTCStatsMember<std::vector<std::string>> ssrc_ids; I don't know how to get the SSRCs for the tracks. https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/stats/rtcstats... webrtc/api/stats/rtcstats_objects.h:282: RTCStatsMember<std::string> media_track_id; How do I know which track's ID to use? A track that has the same SSRC as this stream? Can multiple tracks have the same SSRCs?
https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:307: video_track_source_stats.input_height); On 2016/11/02 11:44:55, hbos wrote: > I wonder if this is the right value. What if the source is a HD video but the > output is downscaled, which value should the track have? And there are similar stats to the spec in Video[Sender/Receiver]Info but that is currently not accessible from the stream/track?
https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:245: // crbug.com/659137 Is it not the case that a stream can be detached as well? It has no "detached" state and the comment about detached tracks stats continuing to appear are not made for streams in the spec.
There were a few comments.... and at least two spec bugs. https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:245: // crbug.com/659137 On 2016/11/02 11:54:44, hbos wrote: > Is it not the case that a stream can be detached as well? It has no "detached" > state and the comment about detached tracks stats continuing to appear are not > made for streams in the spec. The spec has strongly deemphasized streams. In fact, we don't have a means of detaching streams that come in using AddTrack - we're just assuming that they drop off when the tracks are removed - and when AddTrack lands, the correct iteration is to go through all tracks and take note of the streams in passing, no tthe other way around. But AddTrack isn't implemented yet. I think the current code is fine for now. https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:264: stream_stats->track_ids->push_back(audio_track_stats->id()); Consider what happens here if a track is a member of two streams. https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:275: audio_track_stats->audio_level = signal_level / 32767.0; Check this one with Niklas. Audio levels are awful stuff. (and I find the description in the stats doc to be clear as mud, given that it references a document that uses 0..127 on a logarithmic scale, while we want 0..1 on a linear one....) https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:307: video_track_source_stats.input_height); On 2016/11/02 11:44:55, hbos wrote: > I wonder if this is the right value. What if the source is a HD video but the > output is downscaled, which value should the track have? It should be the downscaled value. I have a TODO to fix width and height to reflect the downscaled value for getSettings(), but haven't gotten around to it yet. https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:307: video_track_source_stats.input_height); On 2016/11/02 11:48:41, hbos wrote: > On 2016/11/02 11:44:55, hbos wrote: > > I wonder if this is the right value. What if the source is a HD video but the > > output is downscaled, which value should the track have? > > And there are similar stats to the spec in Video[Sender/Receiver]Info but that > is currently not accessible from the stream/track? Yes. More work needed both for getSettings and for these stats. https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/stats/rtcstats... File webrtc/api/stats/rtcstats_objects.h (right): https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/stats/rtcstats... webrtc/api/stats/rtcstats_objects.h:219: RTCStatsMember<std::vector<std::string>> ssrc_ids; On 2016/11/02 11:44:56, hbos wrote: > I don't know how to get the SSRCs for the tracks. You may have to reverse-map them from the part that goes through all the SSRCs. https://codereview.webrtc.org/2467873005/diff/40001/webrtc/stats/rtcstats_obj... File webrtc/stats/rtcstats_objects.cc (right): https://codereview.webrtc.org/2467873005/diff/40001/webrtc/stats/rtcstats_obj... webrtc/stats/rtcstats_objects.cc:272: // "stream" instead. crbug.com/660827, crbug.com/659137 Spec bug. Will fix. https://codereview.webrtc.org/2467873005/diff/40001/webrtc/stats/rtcstats_obj... webrtc/stats/rtcstats_objects.cc:301: // crbug.com/659137, crbug.com/660827 https://github.com/w3c/webrtc-stats/issues/80
hbos@webrtc.org changed reviewers: + niklas.enbom@webrtc.org
Addressed some of the comments. +niklase for question about audio level https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:245: // crbug.com/659137 On 2016/11/03 14:34:21, hta-webrtc wrote: > On 2016/11/02 11:54:44, hbos wrote: > > Is it not the case that a stream can be detached as well? It has no "detached" > > state and the comment about detached tracks stats continuing to appear are not > > made for streams in the spec. > > The spec has strongly deemphasized streams. In fact, we don't have a means of > detaching streams that come in using AddTrack - we're just assuming that they > drop off when the tracks are removed - and when AddTrack lands, the correct > iteration is to go through all tracks and take note of the streams in passing, > no tthe other way around. > > But AddTrack isn't implemented yet. > > I think the current code is fine for now. Acknowledged. https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:264: stream_stats->track_ids->push_back(audio_track_stats->id()); On 2016/11/03 14:34:21, hta-webrtc wrote: > Consider what happens here if a track is a member of two streams. Done. Skipping if already exists. https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:275: audio_track_stats->audio_level = signal_level / 32767.0; On 2016/11/03 14:34:21, hta-webrtc wrote: > Check this one with Niklas. Audio levels are awful stuff. > (and I find the description in the stats doc to be clear as mud, given that it > references a document that uses 0..127 on a logarithmic scale, while we want > 0..1 on a linear one....) +niklase: Is this the correct way to get the audio level? https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:307: video_track_source_stats.input_height); On 2016/11/03 14:34:21, hta-webrtc wrote: > On 2016/11/02 11:44:55, hbos wrote: > > I wonder if this is the right value. What if the source is a HD video but the > > output is downscaled, which value should the track have? > > It should be the downscaled value. I have a TODO to fix width and height to > reflect the downscaled value for getSettings(), but haven't gotten around to it > yet. What are you referring to when you say "getSettings"? https://codereview.webrtc.org/2467873005/diff/40001/webrtc/stats/rtcstats_obj... File webrtc/stats/rtcstats_objects.cc (right): https://codereview.webrtc.org/2467873005/diff/40001/webrtc/stats/rtcstats_obj... webrtc/stats/rtcstats_objects.cc:272: // "stream" instead. crbug.com/660827, crbug.com/659137 On 2016/11/03 14:34:22, hta-webrtc wrote: > Spec bug. Will fix. Acknowledged. https://codereview.webrtc.org/2467873005/diff/40001/webrtc/stats/rtcstats_obj... webrtc/stats/rtcstats_objects.cc:301: // crbug.com/659137, crbug.com/660827 On 2016/11/03 14:34:22, hta-webrtc wrote: > https://github.com/w3c/webrtc-stats/issues/80 Acknowledged.
Addressed some of the comments. +niklase for question about audio level
(replying to a single question) https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2467873005/diff/40001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:307: video_track_source_stats.input_height); On 2016/11/03 18:10:30, hbos wrote: > On 2016/11/03 14:34:21, hta-webrtc wrote: > > On 2016/11/02 11:44:55, hbos wrote: > > > I wonder if this is the right value. What if the source is a HD video but > the > > > output is downscaled, which value should the track have? > > > > It should be the downscaled value. I have a TODO to fix width and height to > > reflect the downscaled value for getSettings(), but haven't gotten around to > it > > yet. > > What are you referring to when you say "getSettings"? https://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediastreamtrac...
I don't know about the audio level or resolution, but everything else lgtm. https://codereview.webrtc.org/2467873005/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2467873005/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:163: class FakeAudioProcessorForStats : public AudioProcessorInterface { nit: Since these fake objects only exist to return specific values, it seems like it would be appropriate to use mocks instead. But probably not worth changing everything at this point. https://codereview.webrtc.org/2467873005/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:169: } nit: The Create methods here seem redundant. Couldn't FakeAudioProcessorForStats inherit from rtc::RefCountedObject<AudioProcessorInterface>, and the unit test could just use the constructor?
lgtm, most important comment is about a comment. https://codereview.webrtc.org/2467873005/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2467873005/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:243: // detachment. crbug.com/659137 Since data can be passed between the time of the last stats collection and the detaching event, we have to perform stats collection upon detachment. Please update comment. https://codereview.webrtc.org/2467873005/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2467873005/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:1302: expected_remote_video_track.track_identifier = "RemoteVideoTrackID"; Would it be clearer to use remote_video_track->id here? I'm always nervous when we're writing tests where multiple strings have to be kept in sync.
Landing this and resolving audio level question over email instead. Based on [1] both scales are linear, if not I'll add a TODO. [1] https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/webrtc_aud... https://codereview.webrtc.org/2467873005/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector.cc (right): https://codereview.webrtc.org/2467873005/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector.cc:243: // detachment. crbug.com/659137 On 2016/11/08 09:50:22, hta-webrtc wrote: > Since data can be passed between the time of the last stats collection and the > detaching event, we have to perform stats collection upon detachment. Please > update comment. Done. https://codereview.webrtc.org/2467873005/diff/60001/webrtc/api/rtcstatscollec... File webrtc/api/rtcstatscollector_unittest.cc (right): https://codereview.webrtc.org/2467873005/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:163: class FakeAudioProcessorForStats : public AudioProcessorInterface { On 2016/11/04 18:19:14, Taylor Brandstetter wrote: > nit: Since these fake objects only exist to return specific values, it seems > like it would be appropriate to use mocks instead. But probably not worth > changing everything at this point. Acknowledged. Leaving as-is. https://codereview.webrtc.org/2467873005/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:169: } On 2016/11/04 18:19:14, Taylor Brandstetter wrote: > nit: The Create methods here seem redundant. Couldn't FakeAudioProcessorForStats > inherit from rtc::RefCountedObject<AudioProcessorInterface>, and the unit test > could just use the constructor? Done. Did it for FakeVideoTrackSourceForStats too. https://codereview.webrtc.org/2467873005/diff/60001/webrtc/api/rtcstatscollec... webrtc/api/rtcstatscollector_unittest.cc:1302: expected_remote_video_track.track_identifier = "RemoteVideoTrackID"; On 2016/11/08 09:50:22, hta-webrtc wrote: > Would it be clearer to use remote_video_track->id here? > I'm always nervous when we're writing tests where multiple strings have to be > kept in sync. Done, updated all "blah_identifiers" (but not RTCStats IDs).
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org, deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2467873005/#ps100001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2016/11/08 13:43:37, hbos wrote: > Landing this and resolving audio level question over email instead. Based on [1] > both scales are linear, if not I'll add a TODO. Or both are logarithmic and scaled to that range*
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== RTCMediaStream[Track]Stats added. Not all members are collected by RTCStatsCollector and detached tracks are not visible in the returned stats. This needs to be addressed before closing crbug.com/660827 and crbug.com/659137 BUG=chromium:660827, chromium:659137, chromium:627816 ========== to ========== RTCMediaStream[Track]Stats added. Not all members are collected by RTCStatsCollector and detached tracks are not visible in the returned stats. This needs to be addressed before closing crbug.com/660827 and crbug.com/659137 BUG=chromium:660827, chromium:659137, chromium:627816 Committed: https://crrev.com/09bc128603909680a3d9d09100d9be7d3f008036 Cr-Commit-Position: refs/heads/master@{#14978} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/09bc128603909680a3d9d09100d9be7d3f008036 Cr-Commit-Position: refs/heads/master@{#14978} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
