Closed Bug 1241321 Opened 4 years ago Closed 4 years ago

No RTCP stats for audio streams

Categories

(Core :: WebRTC, defect, P1)

38 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox45 --- wontfix
firefox46 --- fixed
firefox47 --- fixed
Blocking Flags:

People

(Reporter: ng, Assigned: ng)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Potentially related: Bug 1241064 - getStats API returns 0 values for some attributes for audio
Summary: No RTCP stats for the first audio stream → No RTCP stats for audio streams
Further testing revealed that this is not limited to the first audio stream. Bug title has been updated, and strangeness quotient has been reduced.
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
Assignee: nobody → na-g
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?
> 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 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+
Reformatted, cleaned up.
Attachment #8715462 - Attachment is obsolete: true
Attachment #8715534 - Flags: review?(rjesup)
Attachment #8715534 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/20e9e5fb3aee
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
What's our current stance on uplifting minor regressions to beta?
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.
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 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+
(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)
(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.
Version: unspecified → 38 Branch
You need to log in before you can comment on or make changes to this bug.