Closed
Bug 1241321
Opened 8 years ago
Closed 8 years ago
No RTCP stats for audio streams
Categories
(Core :: WebRTC, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla47
backlog | webrtc/webaudio+ |
People
(Reporter: ng, Assigned: ng)
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
4.36 KB,
patch
|
jesup
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
The first audio stream does not have associated RTCP stats. This was introduced somewhere between inbound revisions 29b05d283b00:d7e156a7a0a6 via: mozregression --good=37 --bad=38 Note: this range includes f5a4769477bf - Bug 1109248: Webrtc updated to branch 40 7864 Steps to reproduce: Navigate to https://bug951496.bmoattachments.org/attachment.cgi?id=8436385, press "Capture", accept GUM permissions, and press "Connect!". After about 5 seconds RTCP stats (blocks that starts with "Remote:") will arrive for all streams except inbound_rtp_audio_0. What is expected to happen: All streams including inbound_rtp_audio_0 display stats.
Assignee | ||
Comment 1•8 years ago
|
||
Potentially related: Bug 1241064 - getStats API returns 0 values for some attributes for audio
Assignee | ||
Updated•8 years ago
|
Summary: No RTCP stats for the first audio stream → No RTCP stats for audio streams
Assignee | ||
Comment 2•8 years ago
|
||
Further testing revealed that this is not limited to the first audio stream. Bug title has been updated, and strangeness quotient has been reduced.
Updated•8 years ago
|
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → na-g
Assignee | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7012900add6&selectedJob=16115345
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8715462 -
Flags: review?(rjesup)
Comment 5•8 years ago
|
||
Comment on attachment 8715462 [details] [diff] [review] Get Sender Reports from the RTP_RTCP module. Review of attachment 8715462 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ +209,5 @@ > + webrtc::RTCPSenderInfo senderInfo; > + webrtc::RtpRtcp * rtpRtcpModule; > + webrtc::RtpReceiver * rtp_receiver; > + bool result = > + !mPtrVoEVideoSync->GetRtpRtcp(mChannel,&rtpRtcpModule,&rtp_receiver) && video?
Assignee | ||
Comment 6•8 years ago
|
||
> video?
That is the module for keeping the audio and video synchronized. It is only used to get a direct handle on the rtpRtcpModule, because that is the only place (that I can find) where the RTCP Sender Info information is exposed. Optimally there would be an interface on mPtrRTP to get the Sender Info. The obvious choice getSenderInfo() is deprecated and unimplemented.
Comment 7•8 years ago
|
||
Comment on attachment 8715462 [details] [diff] [review] Get Sender Reports from the RTP_RTCP module. Review of attachment 8715462 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp @@ +211,5 @@ > + webrtc::RtpReceiver * rtp_receiver; > + bool result = > + !mPtrVoEVideoSync->GetRtpRtcp(mChannel,&rtpRtcpModule,&rtp_receiver) && > + !rtpRtcpModule->RemoteRTCPStat(&senderInfo); > + if (result){ if (result) { (space before brace; and spaces after commas) Though if you ask me, the separate bool is a bit ugly. if (!mPtrVoEVideoSync->GetRtpRtcp(mChannel, &rtpRtcpModule, &rtp_receiver) && !rtpRtcpModule->RemoteRTCPStat(&senderInfo)) { .... return true; } return false;
Attachment #8715462 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 8•8 years ago
|
||
Reformatted, cleaned up.
Attachment #8715462 -
Attachment is obsolete: true
Attachment #8715534 -
Flags: review?(rjesup)
Updated•8 years ago
|
Attachment #8715534 -
Flags: review?(rjesup) → review+
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/20e9e5fb3aee
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•8 years ago
|
Keywords: regression
Updated•8 years ago
|
status-firefox45:
--- → affected
status-firefox46:
--- → affected
Updated•8 years ago
|
Comment 11•8 years ago
|
||
What's our current stance on uplifting minor regressions to beta?
Comment 12•8 years ago
|
||
Or perhaps it's more correctly a major regression to a very minor feature (to most). I see we did uplift the similar bug 1241064 earlier.
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8715534 [details] [diff] [review] noRtcpAudioStatsBug1241321 Approval Request Comment [Feature/regressing bug #]: Regressed between 37 exclusive and 38 inclusive [User impact if declined]: RTCP stats are a subset call quality metrics necessary to adjust to network conditions. Without these stats clients will not be able to make fully informed decisions, resulting in potentially degraded call quality. [Describe test coverage new/current, TreeHerder]: There is no test coverage. The reported stats can be viewed in about:webrtc. [Risks and why]: Extremely low risk. We need this to fix applications in the field that rely on our getStats for audio; without this we can't track important aspects of call quality. [String/UUID change made/needed]: There are no string or UUID changes.
Attachment #8715534 -
Flags: approval-mozilla-beta?
Comment 14•8 years ago
|
||
Comment on attachment 8715534 [details] [diff] [review] noRtcpAudioStatsBug1241321 Affects WebRTC call quality, let's try it on beta 6. Nico, can you verify the fix once it's released on beta? If this causes regressions also please back it out.
Flags: needinfo?(na-g)
Attachment #8715534 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14) > Comment on attachment 8715534 [details] [diff] [review] > noRtcpAudioStatsBug1241321 > > Affects WebRTC call quality, let's try it on beta 6. > > Nico, can you verify the fix once it's released on beta? If this causes > regressions also please back it out. Liz, I can verify it though I don't think I have the required access to back it out myself.
Flags: needinfo?(na-g)
Comment 16•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/1aea9c9e03c8
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14) > Comment on attachment 8715534 [details] [diff] [review] > noRtcpAudioStatsBug1241321 > > Affects WebRTC call quality, let's try it on beta 6. > > Nico, can you verify the fix once it's released on beta? If this causes > regressions also please back it out. I have verified it on beta @ f8740db4414b.
Updated•8 years ago
|
Version: unspecified → 38 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•