Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1827023002:
Get VideoCapturer stats via VideoTrackSourceInterface in StatsCollector, (Closed)
|
Created:
4 years, 9 months ago by nisse-webrtc Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionGet VideoCapturer stats via VideoTrackSourceInterface in StatsCollector,
without involving the VideoMediaChannel.
BUG=webrtc:5426
Committed: https://crrev.com/fcc640f8f6c844211fb91dea49cb3f727dc07a00
Cr-Commit-Position: refs/heads/master@{#12193}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Move StatsReport logic to VideoRtpSender. #
Total comments: 16
Patch Set 3 : Address nits. #Patch Set 4 : Spacing fix, forgotten in previous upload. #Patch Set 5 : Move logic back to StatsCollector, with an explicit cast to VideoTrackInterface* #
Total comments: 15
Patch Set 6 : Address comments from Peter and Taylor. #
Total comments: 7
Patch Set 7 : Replace VideoCapturer GetStats by GetInputSize, eliminating a copy. #Patch Set 8 : Rebase. #
Total comments: 1
Patch Set 9 : Added TODO comments regarding SSRC == 0. #
Total comments: 4
Patch Set 10 : Comment update. #Patch Set 11 : Rebase. #
Messages
Total messages: 49 (14 generated)
Hi, this cl moves the capturer->GetStats out of the videoengine. Instead, adding
a GetInfo method on the VideoTrackSourceInterface. Not yet working
(investigating crash in peerconnection_unittests).
I don't understand how the StatsReport things really work, and how related
values are grouped together. Ideally, the reports should look identical when
these stats are extracted in ExtractCapturerInfo, compared to the old version
where it came from ExtractVideoInfo.
StatsReport* report = reports_.FindOrAddNew(StatsReport::Id());
seems unlikely to be right, but what should it be?
It's also unclear it my iteration over the tracks really is the right approach.
As far as I understand, there are no test cases for these stats?
nisse@webrtc.org changed reviewers: + pbos@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org
On 2016/03/24 12:25:54, nisse-webrtc wrote: > Hi, this cl moves the capturer->GetStats out of the videoengine. Instead, adding > a GetInfo method on the VideoTrackSourceInterface. Not yet working > (investigating crash in peerconnection_unittests). The crash is because I use StatsReport::Id(), which returns NULL. Anyway, I imagine we'd want to somehow group the capturer stats with the corresponding VideoSenderInfo, where I think entries are identified by ssrc, via GetTransportIdFromProxy and PrepareReport. But it's pretty unclear how to map from a VideoTrack to an ssrc, if at all possible.
On 2016/03/24 12:45:58, nisse-webrtc wrote: > On 2016/03/24 12:25:54, nisse-webrtc wrote: > > Hi, this cl moves the capturer->GetStats out of the videoengine. Instead, > adding > > a GetInfo method on the VideoTrackSourceInterface. Not yet working > > (investigating crash in peerconnection_unittests). > > The crash is because I use StatsReport::Id(), which returns NULL. > > Anyway, I imagine we'd want to somehow group the capturer stats with the > corresponding VideoSenderInfo, where I think entries are identified by ssrc, via > GetTransportIdFromProxy and PrepareReport. > > But it's pretty unclear how to map from a VideoTrack to an ssrc, if at all > possible. To me this looks better that having GetInfo on rtc::VideoSourceInterface. (but even better to not have it all but I understand that is probably not going to happen...) I have lost grip of the statscollector. Tommi and Harald have worked most on it lately but we can try to figure it out next week of all agrees this is what we should do.
https://codereview.webrtc.org/1827023002/diff/1/webrtc/api/mediastreaminterfa... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1827023002/diff/1/webrtc/api/mediastreaminterfa... webrtc/api/mediastreaminterface.h:135: virtual bool GetInfo(VideoSourceInfo* info) = 0; Why not just call this GetStats? And call the struct VideoTrackSourceInterface::Stats? https://codereview.webrtc.org/1827023002/diff/1/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/1827023002/diff/1/webrtc/api/statscollector.cc#... webrtc/api/statscollector.cc:839: // TODO(nisse): What id to use??? Based on the code here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... The ID should be: StatsReport::NewIdWithDirection(StatsReport::kStatsReportTypeSsrc, rtc::ToString<uint32_t>(ssrc), StatsReport::kSend) But to know the SSRC, you need to know the RtpSender object instead of the StreamCollectionInterface, like so: for (rtc::scoped_refptr<RtpSenderInterface> sender : pc_->GetSenders()) { uint32_t ssrc = sender->ssrc(); rtc::scoped_refptr<MediaStreamTrackInterface> track = sender->track(); if (!track || sender->track()->kind() != MediaStreamTrackInterface::kVideoKind) { continue; } StatsReport::Id stats_id = StatsReport::NewIdWithDirection(StatsReport::kStatsReportTypeSsrc, rtc::ToString<uint32_t>(ssrc), StatsReport::kSend); track->GetSource()->GetInfo(&info); ... } Now, there's just one problem: RtpSender::GetTrack is a MediaStreamTrackInterface, not a VideoTrackSourceInterface. Have you considered moving this code to RtpSender by implementing an RtpSender::GetStats? That way you could do this: sender->GetStats(&report); And then the sender could do report->AddInt(...), etc. In fact, it might be worth moving the logic for the stats_id to the sender as well so that you would just have: for (rtc::scoped_refptr<RtpSenderInterface> sender : pc_->GetSenders()) { StatsReport* report = report_.FindOrAddNew(sender->stats_id()); sender->GetStats(&report); } And you'd be done.
https://codereview.webrtc.org/1827023002/diff/1/webrtc/api/mediastreaminterfa... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1827023002/diff/1/webrtc/api/mediastreaminterfa... webrtc/api/mediastreaminterface.h:135: virtual bool GetInfo(VideoSourceInfo* info) = 0; On 2016/03/24 18:10:25, pthatcher1 wrote: > Why not just call this GetStats? And call the struct > VideoTrackSourceInterface::Stats? Renamed. https://codereview.webrtc.org/1827023002/diff/1/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/1827023002/diff/1/webrtc/api/statscollector.cc#... webrtc/api/statscollector.cc:839: // TODO(nisse): What id to use??? On 2016/03/24 18:10:25, pthatcher1 wrote: > Have you considered moving this code to RtpSender by implementing an > RtpSender::GetStats? That way you could do this: > > sender->GetStats(&report); > > > And then the sender could do report->AddInt(...), etc. In fact, it might be > worth moving the logic for the stats_id to the sender as well so that you would > just have: > > for (rtc::scoped_refptr<RtpSenderInterface> sender : pc_->GetSenders()) { > StatsReport* report = report_.FindOrAddNew(sender->stats_id()); > sender->GetStats(&report); > } > > And you'd be done. Nice idea. I implemented that. A few questions: Do we really want to use StatsReport directly outside of the StatsCollector? That seems like the most reasonable way if we want to provide some other stats for AudioRtpSender. I named the RtpSender method ReportStats rather then GetStats because I think the latter name is more appropriate for a method populating a simple struct with values. Do we need any synchronization in VideoRtpSender? Or is all relevant code running on the same thread? The current version will produce empty reports in some cases. E.g., if track_ is NULL, or if GetSource()->GetStats(&stats) returns false (happens for remote sources), VideoRtpSender::stats_id returns non-NULL, but ReportStats doesn't fill out any values. To get that right, it would be nice with a single method producing either both the id and the report, or nothing, but that collides a bit with the use of FindOrAddNew. Not sure if it matters, maybe empty reports are pruned later anyway?
https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.cc File webrtc/api/rtpsender.cc (right): https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.cc#n... webrtc/api/rtpsender.cc:357: VideoTrackSourceInterface::Stats stats; Why not skip VideoTrackSourceInterface::Stats all together and call GetSource->GetStats(report); https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.h File webrtc/api/rtpsender.h (right): https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.h#ne... webrtc/api/rtpsender.h:107: void ReportStats(StatsReport* report) override{}; GetStats ? https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsenderinter... File webrtc/api/rtpsenderinterface.h (right): https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsenderinter... webrtc/api/rtpsenderinterface.h:59: // Returns a scoped_refptr(nullptr) if there are no stats. ? StatsReport::Id is a scoped_refptr? https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsenderinter... webrtc/api/rtpsenderinterface.h:61: virtual void ReportStats(StatsReport* report) = 0; Prefer GetStats anyway (saw your comments) since its widely used in the code base for stats. https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/statscollector... webrtc/api/statscollector.cc:837: sender->ReportStats(report); Why not just sender->GetStats(&reports_); ? Let the sender add its now report if it has an id. No need then to have the Stats::Id in the public interface.
On 2016/03/29 11:03:39, perkj_webrtc wrote: > https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.cc > File webrtc/api/rtpsender.cc (right): > > https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.cc#n... > webrtc/api/rtpsender.cc:357: VideoTrackSourceInterface::Stats stats; > Why not skip VideoTrackSourceInterface::Stats all together and call > GetSource->GetStats(report); > > https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.h > File webrtc/api/rtpsender.h (right): > > https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.h#ne... > webrtc/api/rtpsender.h:107: void ReportStats(StatsReport* report) override{}; > GetStats ? > > https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsenderinter... > File webrtc/api/rtpsenderinterface.h (right): > > https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsenderinter... > webrtc/api/rtpsenderinterface.h:59: // Returns a scoped_refptr(nullptr) if there > are no stats. > ? StatsReport::Id is a scoped_refptr? > > https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsenderinter... > webrtc/api/rtpsenderinterface.h:61: virtual void ReportStats(StatsReport* > report) = 0; > Prefer GetStats anyway (saw your comments) since its widely used in the code > base for stats. > > https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/statscollector.cc > File webrtc/api/statscollector.cc (right): > > https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/statscollector... > webrtc/api/statscollector.cc:837: sender->ReportStats(report); > Why not just sender->GetStats(&reports_); ? Let the sender add its now report if > it has an id. No need then to have the Stats::Id in the public interface. btw- RtpSender and PeerConnection operate on the same thread so this is ok. But we should add a thread checker just for ducumenting that. (not necessarily in this cl).
nisse@webrtc.org changed reviewers: + tommi@webrtc.org
Tommi, do you know the StatsCollector machinery? Do you think it's a good or bad idea to pass a StatsReport* or a StatsCollection* down to the component which generates the stats? https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.cc File webrtc/api/rtpsender.cc (right): https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.cc#n... webrtc/api/rtpsender.cc:357: VideoTrackSourceInterface::Stats stats; On 2016/03/29 11:03:39, perkj_webrtc wrote: > Why not skip VideoTrackSourceInterface::Stats all together and call > GetSource->GetStats(report); I don't quite like adding dependencies to StatsReport all over the code, it seems nicer to fill out a plain struct with fixed fields. But if you think that's the right direction, I can do it. https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsenderinter... File webrtc/api/rtpsenderinterface.h (right): https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsenderinter... webrtc/api/rtpsenderinterface.h:59: // Returns a scoped_refptr(nullptr) if there are no stats. On 2016/03/29 11:03:39, perkj_webrtc wrote: > ? StatsReport::Id is a scoped_refptr? Yes, defined in statstypes.h as typedef rtc::scoped_refptr<IdBase> Id; https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsenderinter... webrtc/api/rtpsenderinterface.h:61: virtual void ReportStats(StatsReport* report) = 0; On 2016/03/29 11:03:39, perkj_webrtc wrote: > Prefer GetStats anyway (saw your comments) since its widely used in the code > base for stats. Most other GetStats methods either take an observer, or they work with a plain struct. But I'll rename it if you really want. I think StatsReport is a too opaque type to be used for anything not directly tied to StatsCollector. E.g., if you write test cases related to the stats for some component, it's going to be annoying and error prone to have to access the produced stats via StatsReport::FindValue(StatsReport::kSomething).int_val(), not to speak about error checking needed if you want the testcase to report the precise problem rather than crash. https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/statscollector... webrtc/api/statscollector.cc:837: sender->ReportStats(report); On 2016/03/29 11:03:39, perkj_webrtc wrote: > Why not just sender->GetStats(&reports_); ? Let the sender add its now report if > it has an id. No need then to have the Stats::Id in the public interface. I like the reduction to a single method. We then get a StatsCollection in the RtpSender interface instead.
let's discuss today https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.cc File webrtc/api/rtpsender.cc (right): https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.cc#n... webrtc/api/rtpsender.cc:356: if (track_) { early returns are preferred. i.e.: if (!track_) return; ... https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.h File webrtc/api/rtpsender.h (right): https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.h#ne... webrtc/api/rtpsender.h:107: void ReportStats(StatsReport* report) override{}; void ReportStats(StatsReport* report) override {} https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsenderinter... File webrtc/api/rtpsenderinterface.h (right): https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsenderinter... webrtc/api/rtpsenderinterface.h:61: virtual void ReportStats(StatsReport* report) = 0; On 2016/03/29 11:37:31, nisse-webrtc wrote: > On 2016/03/29 11:03:39, perkj_webrtc wrote: > > Prefer GetStats anyway (saw your comments) since its widely used in the code > > base for stats. > > Most other GetStats methods either take an observer, or they work with a plain > struct. But I'll rename it if you really want. > > I think StatsReport is a too opaque type to be used for anything not directly > tied to StatsCollector. E.g., if you write test cases related to the stats for > some component, it's going to be annoying and error prone to have to access the > produced stats via StatsReport::FindValue(StatsReport::kSomething).int_val(), > not to speak about error checking needed if you want the testcase to report the > precise problem rather than crash. Is this method's implementation expected to block at any time? If so, should it be designed to be async? (as GetStats is) https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsenderinter... webrtc/api/rtpsenderinterface.h:81: PROXY_METHOD1(void, ReportStats, StatsReport*) as is, all proxy methods involve a synchronous thread hop. This is unfortunate and I'd like for us to design the APIs with more asynchronicity in mind.
https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.cc File webrtc/api/rtpsender.cc (right): https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.cc#n... webrtc/api/rtpsender.cc:356: if (track_) { On 2016/03/30 11:46:44, tommi-webrtc wrote: > early returns are preferred. i.e.: > > if (!track_) > return; > > ... Done. https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.h File webrtc/api/rtpsender.h (right): https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsender.h#ne... webrtc/api/rtpsender.h:107: void ReportStats(StatsReport* report) override{}; On 2016/03/30 11:46:44, tommi-webrtc wrote: > void ReportStats(StatsReport* report) override {} Fixed. For some reason, git cl format wants to remove the space between override and {}. https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsenderinter... File webrtc/api/rtpsenderinterface.h (right): https://codereview.webrtc.org/1827023002/diff/20001/webrtc/api/rtpsenderinter... webrtc/api/rtpsenderinterface.h:61: virtual void ReportStats(StatsReport* report) = 0; On 2016/03/30 11:46:45, tommi-webrtc wrote: > On 2016/03/29 11:37:31, nisse-webrtc wrote: > > On 2016/03/29 11:03:39, perkj_webrtc wrote: > > > Prefer GetStats anyway (saw your comments) since its widely used in the code > > > base for stats. > > > > Most other GetStats methods either take an observer, or they work with a plain > > struct. But I'll rename it if you really want. > > > > I think StatsReport is a too opaque type to be used for anything not directly > > tied to StatsCollector. E.g., if you write test cases related to the stats for > > some component, it's going to be annoying and error prone to have to access > the > > produced stats via StatsReport::FindValue(StatsReport::kSomething).int_val(), > > not to speak about error checking needed if you want the testcase to report > the > > precise problem rather than crash. > > Is this method's implementation expected to block at any time? If so, should it > be designed to be async? (as GetStats is) It shouldn't block. If the RtpSender is connected to a camera, it ends up in cricket::VideoCapturer::GetStats which returns the size of the most recent captured frame (after taking the frame_stats_crit_ lock, which only competes with UpdateStats). In all other cases, it does nothing.
New version. Tommi suggested movig the logic back to StatsCollector. Then it needs to check the media type and do an explicit cast to VideoTrackInterface. But we don't need any changes to the RtpSender interface, and we reduce the amount of code which needs to be aware of the StatsCollector types.
Description was changed from ========== Get VideoCapturer stats via VideoTrackSourceInterface in StatsCollector, without involving the VideoMediaChannel. BUG=webrtc:5426 ========== to ========== Get VideoCapturer stats via VideoTrackSourceInterface in StatsCollector, without involving the VideoMediaChannel. BUG=webrtc:5426 ==========
deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org
lgtm I like the latest decisions. But please follow up on my comments. I'll LG because I'm OOO the rest of the week and I'd like you to be able to submit. https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:833: for (const auto &sender : pc_->GetSenders()) { const auto& sender https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:838: VideoTrackSourceInterface *source = VideoTrackSourceInterface* source https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:839: static_cast<VideoTrackInterface*>(track.get())->GetSource(); I agree with the decision that you and Tommi made to keep this logic here. However, if we're going to be relying on the fact that any MediaStreamTrackInterface with kind == MediaStreamTrackInterface::kVideoKind is really a VideoTrackInterface, we should document that clearly in two places: 1. In mediastreaminterface.h, where MediaStreamTrackInterface is defined. 2. Here, saying "this is safe because ...; see mediastreaminterface.h". https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:842: if (source->GetStats(&stats)) { I'd prefer: if (!source->GetStats(&stats)) { continue: } ...
https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:835: if (!track || track->kind() != MediaStreamTrackInterface::kVideoKind) nit: {}s https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:845: rtc::ToString<uint32_t>(sender->ssrc()), It's possible that a sender doesn't have an SSRC (ssrc() == 0), if no local description has been set yet. In that case it doesn't make sense to report these stats, since they hinge off an SSRC. So you should add "|| sender->ssrc() == 0" to the above "if" statement.
Thanks for the review. https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:833: for (const auto &sender : pc_->GetSenders()) { On 2016/03/30 16:48:43, pthatcher1 wrote: > const auto& sender Done. https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:835: if (!track || track->kind() != MediaStreamTrackInterface::kVideoKind) On 2016/03/30 17:42:32, Taylor Brandstetter wrote: > nit: {}s Done. https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:838: VideoTrackSourceInterface *source = On 2016/03/30 16:48:43, pthatcher1 wrote: > VideoTrackSourceInterface* source Done. https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:839: static_cast<VideoTrackInterface*>(track.get())->GetSource(); On 2016/03/30 16:48:43, pthatcher1 wrote: > I agree with the decision that you and Tommi made to keep this logic here. > However, if we're going to be relying on the fact that any > MediaStreamTrackInterface with kind == MediaStreamTrackInterface::kVideoKind is > really a VideoTrackInterface, we should document that clearly in two places: > > 1. In mediastreaminterface.h, where MediaStreamTrackInterface is defined. > > 2. Here, saying "this is safe because ...; see mediastreaminterface.h". Done. We're not using dynamic_cast<>? This must be the type of situation it is intended for. https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:842: if (source->GetStats(&stats)) { On 2016/03/30 16:48:43, pthatcher1 wrote: > I'd prefer: > > if (!source->GetStats(&stats)) { > continue: > } > > ... Done. https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:845: rtc::ToString<uint32_t>(sender->ssrc()), On 2016/03/30 17:42:32, Taylor Brandstetter wrote: > It's possible that a sender doesn't have an SSRC (ssrc() == 0), if no local > description has been set yet. In that case it doesn't make sense to report these > stats, since they hinge off an SSRC. So you should add "|| sender->ssrc() == 0" > to the above "if" statement. I added it as a separate "if"; no need to fiddle with the track refcount if we're not going to use it. My understanding is that ssrc == 0 is technically valid in the webrtc wire protocols, so this logic is something we'd like to change eventually?
https://codereview.webrtc.org/1827023002/diff/100001/webrtc/api/mediastreamin... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1827023002/diff/100001/webrtc/api/mediastreamin... webrtc/api/mediastreaminterface.h:139: virtual bool GetStats(Stats* stats) = 0; I assume this is intended to fail if we haven't gotten a first frame? Can you document when GetStats would be expected to fail? Otherwise: Can this be void instead? https://codereview.webrtc.org/1827023002/diff/100001/webrtc/api/videocapturer... File webrtc/api/videocapturertracksource.cc (right): https://codereview.webrtc.org/1827023002/diff/100001/webrtc/api/videocapturer... webrtc/api/videocapturertracksource.cc:367: video_capturer_->GetStats(&last_captured_frame_format); Are these reasonable values initially?
lgtm if GetStats return void. https://codereview.webrtc.org/1827023002/diff/100001/webrtc/api/mediastreamin... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1827023002/diff/100001/webrtc/api/mediastreamin... webrtc/api/mediastreaminterface.h:139: virtual bool GetStats(Stats* stats) = 0; On 2016/03/31 11:50:46, pbos-webrtc wrote: > I assume this is intended to fail if we haven't gotten a first frame? Can you > document when GetStats would be expected to fail? > > Otherwise: Can this be void instead? prefer void.
https://codereview.webrtc.org/1827023002/diff/100001/webrtc/api/mediastreamin... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1827023002/diff/100001/webrtc/api/mediastreamin... webrtc/api/mediastreaminterface.h:139: virtual bool GetStats(Stats* stats) = 0; On 2016/03/31 11:50:46, pbos-webrtc wrote: > I assume this is intended to fail if we haven't gotten a first frame? Can you > document when GetStats would be expected to fail? > > Otherwise: Can this be void instead? It's intended to fail if the source doesn't have any stats. In particular, a remote source will always return false. https://codereview.webrtc.org/1827023002/diff/100001/webrtc/api/mediastreamin... webrtc/api/mediastreaminterface.h:139: virtual bool GetStats(Stats* stats) = 0; On 2016/03/31 11:57:30, perkj_webrtc wrote: > On 2016/03/31 11:50:46, pbos-webrtc wrote: > > I assume this is intended to fail if we haven't gotten a first frame? Can you > > document when GetStats would be expected to fail? > > > > Otherwise: Can this be void instead? > > prefer void. What should a VideoTrackSource (used for remote sources) do then? https://codereview.webrtc.org/1827023002/diff/100001/webrtc/api/videocapturer... File webrtc/api/videocapturertracksource.cc (right): https://codereview.webrtc.org/1827023002/diff/100001/webrtc/api/videocapturer... webrtc/api/videocapturertracksource.cc:367: video_capturer_->GetStats(&last_captured_frame_format); On 2016/03/31 11:50:46, pbos-webrtc wrote: > Are these reasonable values initially? The video capturer makes a copy of its VideoFormat last_captured_frame_format_; It looks like VideoFormat has a default constructor setting everything to zero. So I think we'll get width and height zero. No idea if that's reasonable or not, but it's unchanged behaviour.
https://codereview.webrtc.org/1827023002/diff/100001/webrtc/api/mediastreamin... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1827023002/diff/100001/webrtc/api/mediastreamin... webrtc/api/mediastreaminterface.h:139: virtual bool GetStats(Stats* stats) = 0; On 2016/03/31 12:07:27, nisse-webrtc wrote: > On 2016/03/31 11:50:46, pbos-webrtc wrote: > > I assume this is intended to fail if we haven't gotten a first frame? Can you > > document when GetStats would be expected to fail? > > > > Otherwise: Can this be void instead? > > It's intended to fail if the source doesn't have any stats. In particular, a > remote source will always return false. I see. Ok - please then document when this returns true/false.
I've added some comments, did cosmetic reformatting of the statscollector code,
and I changed the VideoCapturer GetStats method to GetInputSize. THims makes
GetStats return false if no frame has been seen, and it eliminates a VideoFormat
copy.
Please have a final look at the changes to videocapturer.{h,cc}, in particular
the GUARDED_BY things I added.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827023002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827023002/140001
On 2016/03/31 12:52:01, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1827023002/140001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1827023002/140001 lgtm
lgtm https://codereview.webrtc.org/1827023002/diff/140001/webrtc/api/statscollecto... File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/1827023002/diff/140001/webrtc/api/statscollecto... webrtc/api/statscollector.cc:834: if (!sender->ssrc()) { Since ssrc == 0 is technically valid, make a TODO to have another way to check if this has a set SSRC yet.
I intend to land this tomorrow morning (Stockholm time). Taylor, Tommi, if you have any further comments, please speak up.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827023002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827023002/160001
lgtm https://codereview.webrtc.org/1827023002/diff/160001/webrtc/api/mediastreamin... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1827023002/diff/160001/webrtc/api/mediastreamin... webrtc/api/mediastreaminterface.h:142: // source, or a source which has not seen its first frame yet. add a note that the implementation should avoid blocking? https://codereview.webrtc.org/1827023002/diff/160001/webrtc/api/mediastreamin... webrtc/api/mediastreaminterface.h:143: virtual bool GetStats(Stats* stats) = 0; have you checked if there are implementations of VideoTrackSourceInterface in other repos we work on, e.g. Chrome?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:845: rtc::ToString<uint32_t>(sender->ssrc()), On 2016/03/31 06:31:22, nisse-webrtc wrote: > On 2016/03/30 17:42:32, Taylor Brandstetter wrote: > > It's possible that a sender doesn't have an SSRC (ssrc() == 0), if no local > > description has been set yet. In that case it doesn't make sense to report > these > > stats, since they hinge off an SSRC. So you should add "|| sender->ssrc() == > 0" > > to the above "if" statement. > > I added it as a separate "if"; no need to fiddle with the track refcount if > we're not going to use it. > > My understanding is that ssrc == 0 is technically valid in the webrtc wire > protocols, so this logic is something we'd like to change eventually? We should change it to an rtc::Optional in that case. It would make things more clear anyway. I'll assign a bug to myself to remind myself to do this.
I'll try to land this now. https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:845: rtc::ToString<uint32_t>(sender->ssrc()), On 2016/03/31 18:25:03, Taylor Brandstetter wrote: > On 2016/03/31 06:31:22, nisse-webrtc wrote: > > On 2016/03/30 17:42:32, Taylor Brandstetter wrote: > > > It's possible that a sender doesn't have an SSRC (ssrc() == 0), if no local > > > description has been set yet. In that case it doesn't make sense to report > > these > > > stats, since they hinge off an SSRC. So you should add "|| sender->ssrc() == > > 0" > > > to the above "if" statement. > > > > I added it as a separate "if"; no need to fiddle with the track refcount if > > we're not going to use it. > > > > My understanding is that ssrc == 0 is technically valid in the webrtc wire > > protocols, so this logic is something we'd like to change eventually? > > We should change it to an rtc::Optional in that case. It would make things more > clear anyway. I'll assign a bug to myself to remind myself to do this. If possible, it would be even clearer if ssrc is always set, or always set if a track is set. Is it reasonable to attach a track before the ssrc is known? https://codereview.webrtc.org/1827023002/diff/160001/webrtc/api/mediastreamin... File webrtc/api/mediastreaminterface.h (right): https://codereview.webrtc.org/1827023002/diff/160001/webrtc/api/mediastreamin... webrtc/api/mediastreaminterface.h:142: // source, or a source which has not seen its first frame yet. On 2016/03/31 14:59:56, tommi-webrtc wrote: > add a note that the implementation should avoid blocking? Done. https://codereview.webrtc.org/1827023002/diff/160001/webrtc/api/mediastreamin... webrtc/api/mediastreaminterface.h:143: virtual bool GetStats(Stats* stats) = 0; On 2016/03/31 14:59:56, tommi-webrtc wrote: > have you checked if there are implementations of VideoTrackSourceInterface in > other repos we work on, e.g. Chrome? I don't find any. This class was added by Per quite recently.
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, pbos@webrtc.org, perkj@webrtc.org, tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/1827023002/#ps180001 (title: "Comment update.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827023002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827023002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_mips_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_mips_db...) android_compile_x64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) android_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/9936) android_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/1...) ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/6385) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/6373) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/8711) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/8632) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/13914) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/12547) linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/13637) linux_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...) linux_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/9796) linux_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/13942) linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/11056) linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/939) linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/13555) mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/10272) 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/2026) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/8254) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13964) presubmit on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/4534) win_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_compile_dbg/builds/...) win_x64_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, pbos@webrtc.org, perkj@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/1827023002/#ps200001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1827023002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1827023002/200001
Message was sent while issue was closed.
Description was changed from ========== Get VideoCapturer stats via VideoTrackSourceInterface in StatsCollector, without involving the VideoMediaChannel. BUG=webrtc:5426 ========== to ========== Get VideoCapturer stats via VideoTrackSourceInterface in StatsCollector, without involving the VideoMediaChannel. BUG=webrtc:5426 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Get VideoCapturer stats via VideoTrackSourceInterface in StatsCollector, without involving the VideoMediaChannel. BUG=webrtc:5426 ========== to ========== Get VideoCapturer stats via VideoTrackSourceInterface in StatsCollector, without involving the VideoMediaChannel. BUG=webrtc:5426 Committed: https://crrev.com/fcc640f8f6c844211fb91dea49cb3f727dc07a00 Cr-Commit-Position: refs/heads/master@{#12193} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/fcc640f8f6c844211fb91dea49cb3f727dc07a00 Cr-Commit-Position: refs/heads/master@{#12193}
Message was sent while issue was closed.
https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector.cc File webrtc/api/statscollector.cc (right): https://codereview.webrtc.org/1827023002/diff/80001/webrtc/api/statscollector... webrtc/api/statscollector.cc:845: rtc::ToString<uint32_t>(sender->ssrc()), On 2016/04/01 06:28:28, nisse-webrtc wrote: > On 2016/03/31 18:25:03, Taylor Brandstetter wrote: > > On 2016/03/31 06:31:22, nisse-webrtc wrote: > > > On 2016/03/30 17:42:32, Taylor Brandstetter wrote: > > > > It's possible that a sender doesn't have an SSRC (ssrc() == 0), if no > local > > > > description has been set yet. In that case it doesn't make sense to report > > > these > > > > stats, since they hinge off an SSRC. So you should add "|| sender->ssrc() > == > > > 0" > > > > to the above "if" statement. > > > > > > I added it as a separate "if"; no need to fiddle with the track refcount if > > > we're not going to use it. > > > > > > My understanding is that ssrc == 0 is technically valid in the webrtc wire > > > protocols, so this logic is something we'd like to change eventually? > > > > We should change it to an rtc::Optional in that case. It would make things > more > > clear anyway. I'll assign a bug to myself to remind myself to do this. > > If possible, it would be even clearer if ssrc is always set, or always set if a > track is set. Is it reasonable to attach a track before the ssrc is known? Yes, it is reasonable. Through the API you can use "addTransceiver" to create a sender, and then add a track to it, before any SDP negotiation (which decides SSRCs) has been done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
