Closed Bug 1393095 Opened 7 years ago Closed 7 years ago

Remote audio stats stopped working in Firefox 56 (regression)

Categories

(Core :: WebRTC, defect, P2)

Product:
Core
Component:
WebRTC
Version:
56 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 + wontfix
firefox57 + verified

People

(Reporter: jib, Assigned: ng)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1393095 - remote audio receiver stats missing;
59 bytes, text/x-review-board-request
dminor
: review+
Details 
Basically it always reports 0 packets now and never updates. Another regression from bug 1341285.

STRs:
  Open https://jsfiddle.net/jib1/qs830j4j/

Expected result:

  Remote inbound-rtp audio 00:47:03 GMT-0500 (EST)
  SSRC: 2698498028 Received: 1233 packets (0.12 MB) Lost: 0
  Discarded packets: undefined Jitter: 0.237

Actual result:

  Remote inbound-rtp audio 00:50:52 GMT-0500 (EST)
  SSRC: 957274255 Received: 0 packets (0.00 MB) Lost: 0
  Discarded packets: undefined Jitter: 0

Regression range:

  19:16.11 INFO: Last good revision: c7428449127566105bdd94b2823b01e0e5e007d5
  19:16.11 INFO: First bad revision: 5bbdb7d36ee3c136a0ed03be9d5b012d05dfd08e
  19:16.11 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c7428449127566105bdd94b2823b01e0e5e007d5&tochange=5bbdb7d36ee3c136a0ed03be9d5b012d05dfd08e
From IRC: jesup:	jib_: might be able to add the test by just making one of the mochitests do a waitForMediaFlow (or waitForRtpFlow) on audio.
If waitForMediaFlow is changed to wait for audio test_peerConnection_stats.html should already be setup to test the remote audio stats.
Assignee: nobody → na-g
[Tracking Requested - why for this release]:

A regression. WebRTC calling services may interpret 0 packets received remotely as an error or indication of networking issues, and either abort or downgrade quality to compensate, leading to sub-par performance.
tracking-firefox56: --- → ?
tracking-firefox57: --- → ?
This behavior seems only to be exhibited when one peer connection is only a sender and the remote peer connection is only a receiver. See: http://jsfiddle.net/kj80f28d/
Track 56+/57+ as regression.
tracking-firefox56: ? → +
tracking-firefox57: ? → +
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Attachment #8908690 - Flags: review?(rjesup)
Comment on attachment 8908690 [details]
Bug 1393095 - remote audio receiver stats missing;

https://reviewboard.mozilla.org/r/180334/#review185492

So, bigger question: is there an alternative to making large changes to channel.cc/etc?  I'm guessing not... Do newer webrtc.org versions fix this?  If so, we can cherrypick changes from those and drop them on update.
Large changes make it hard to merge, unless we can get them upstreamed (which would require a lot of additions to the unit tests in the webrtc.org code)

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp
(Diff revision 1)
> -  mChannelProxy->GetRTPStatistics(averageJitterMs, maxJitterMs, discardedPackets, cumulative);
> -  *jitterMs = averageJitterMs;
> -
> -  // XXX Note: timestamp is not correct per the spec... should be time the
> -  // rtcp was received (remote) or sent (local)
> -  *timestamp = webrtc::Clock::GetRealTimeClock()->TimeInMilliseconds();

Is the value returned by GetRTCPReceiverStatistics()  the reception timestamp?  I.e. does it match the spec?  If not, retain the comment (and file a bug if there's isn't one, and mark the number here)

::: media/webrtc/trunk/webrtc/voice_engine/channel.cc:425
(Diff revision 1)
>      }
>      packet_counter_ = packet_counter;
>   };
>  
> + // Called when we receive RTCP receiver reports
> + // TODO @@NG does this need to accept and filter out sender reports?

Figure this out before landing, and fix or remove the comment

::: media/webrtc/trunk/webrtc/voice_engine/channel.cc:429
(Diff revision 1)
> +    if(!mReceiverReports.empty()) { // Don't lock if we have nothing to do.
> +      rtc::CritScope cs(&stats_lock_);
> +      for(const auto& report : mReceiverReports) {

spaces before ( for if, for, etc
Match file style
Attachment #8908690 - Flags: review?(rjesup) → review+
Comment on attachment 8908690 [details]
Bug 1393095 - remote audio receiver stats missing;

https://reviewboard.mozilla.org/r/180334/#review185496

Looks good to me, deferring to jesup for final review.

::: media/webrtc/trunk/webrtc/voice_engine/channel.cc:341
(Diff revision 1)
> +      if (!mFirstExtendedSequenceNumber && initialSequenceNum) {
> +        mFirstExtendedSequenceNumber = *initialSequenceNum;
> +      }
> +      // No initial sequence number available!
> +      if (!mFirstExtendedSequenceNumber) {
> +        // This is as good a guess as we can get if the initial

This seems unexpected, is it worth logging this?

Is it possible that we will be called with a valid initialSequenceNum after we've guessed?

::: media/webrtc/trunk/webrtc/voice_engine/channel.cc:379
(Diff revision 1)
> +    // The RTP sender must record the first sequence number used
> +    // so that number of packets received can be calculated from ...
> +    uint32_t mFirstExtendedSequenceNumber = 0;
> +    // The most recent sequence number seen by the receiver at the time
> +    // Receiver Report was generated
> +    uint32_t mLatestHighExtenedSequenceNumber = 0;

typo: s/Extened/Extended here and where used

::: media/webrtc/trunk/webrtc/voice_engine/channel.cc:386
(Diff revision 1)
> +    // The amount of jitter measured in MS, derived from the
> +    // RTCP reported jitter (measured in frames), and the
> +    // effective playout frequency.
> +    double JitterMs() const {
> +      if (!mEncoderFrequencyHz) {
> +        // TODO WARN - can't calculate a jitter without a sample rate!

Maybe log something here?

::: media/webrtc/trunk/webrtc/voice_engine/channel.cc:425
(Diff revision 1)
>      }
>      packet_counter_ = packet_counter;
>   };
>  
> + // Called when we receive RTCP receiver reports
> + // TODO @@NG does this need to accept and filter out sender reports?

Can you detect a sender report by looking at the ssrcs in ReportBlockList?

::: media/webrtc/trunk/webrtc/voice_engine/channel.cc:433
(Diff revision 1)
> +                                const int64_t aReceptionTime) {
> +    if(!mReceiverReports.empty()) { // Don't lock if we have nothing to do.
> +      rtc::CritScope cs(&stats_lock_);
> +      for(const auto& report : mReceiverReports) {
> +        // Creates a new report if necessary before updating
> +        mReceiverReportDerivedStats.emplace(std::piecewise_construct,

I think this would be easier to read if you used a temporary variable to hold the result of the emplace call. Presumably the compiler would optimize it away.

::: media/webrtc/trunk/webrtc/voice_engine/channel.cc:445
(Diff revision 1)
> +                                     aReceptionTime);
> +      }
> +    }
> +  }
> +  void OnSendCodecFrequencyChanged(uint32_t aFrequency) {
> +    rtc::CritScope cs(&stats_lock_);

You could make mPlayoutFrequency atomic and avoid the lock here.

::: media/webrtc/trunk/webrtc/voice_engine/channel.cc:451
(Diff revision 1)
> +    mPlayoutFrequency = aFrequency;
> +  }
> +
> +  void OnInitialSequenceNumberSet(uint32_t aSequenceNumber) {
> +    rtc::CritScope cs(&stats_lock_);
> +    mInitialSequenceNumber.emplace(aSequenceNumber);

Less sure of this, but rtc::Optional might be able to hold an atomic uint32_t in which case you could then also avoid a lock here.

::: media/webrtc/trunk/webrtc/voice_engine/channel.cc:477
(Diff revision 1)
>    rtc::CriticalSection stats_lock_;
>    uint32_t ssrc_;
>    ChannelStatistics stats_;
>    RtcpPacketTypeCounter packet_counter_;
> +
> +  typedef uint32_t ssrc_t; // SSRC type alias for key

I don't think this is necessary, you only use it two lines below, and all other ssrcs here are uint32_t.

::: media/webrtc/trunk/webrtc/voice_engine/channel.cc:569
(Diff revision 1)
> +  *packetsReceived = stats->PacketsReceived();
> +  *packetsFractionLost = stats->FractionOfPacketsLost();
> +  *rtt = stats->mRoundTripTime;
> +
> +  // bytesReceived is only an estimate, which we derive from the locally
> +  // generated RTCP sender reports, and the remotely genderated receiver

typo: generated not genderated
Attachment #8908690 - Flags: review?(dminor) → review+
Comment on attachment 8908690 [details]
Bug 1393095 - remote audio receiver stats missing;

https://reviewboard.mozilla.org/r/180334/#review185492

I don't think there is a substantially smaller change that could be made. This bug is marked as Won't Fix upstream, with the plan being to implment this correctly in the new audio interface. The StatisticsProxy could be pulled into it's own file, which would eliminate the bulk of the changes.
Comment on attachment 8908690 [details]
Bug 1393095 - remote audio receiver stats missing;

https://reviewboard.mozilla.org/r/180334/#review185496

> This seems unexpected, is it worth logging this?
> 
> Is it possible that we will be called with a valid initialSequenceNum after we've guessed?

Added the logging. I think the only sensible thing to do in the case where a initialSequenceNum comes in after a receiver report is to reset the stats. Adding that as well.

> You could make mPlayoutFrequency atomic and avoid the lock here.

This should be a very infrequent call. I think in most cases it will only be called once.

> Less sure of this, but rtc::Optional might be able to hold an atomic uint32_t in which case you could then also avoid a lock here.

Keeping this lock because of the changes to the first issue.
Pushed by na-g@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/e354373c68a2
remote audio receiver stats missing;r=dminor,jesup
https://hg.mozilla.org/mozilla-central/rev/e354373c68a2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
We're building the RC for Fx56 on Monday. This looks pretty scary to uplift at the last second, so calling it wontfix for 56. Feel free to set it back to affected and plead your case for mozilla-release approval if you feel otherwise, though :)
status-firefox56: affected → wontfix
status-firefox-esr52: --- → unaffected
Keywords: regression
I managed to reproduce the bug using an old version of Nightly from 2017-08-23 on Windows 10x64. I tested using the link from comment 0 and the result for audio was 0 received packets.
I retested everything using beta 57.0b3 and latest Nightly 58.0a1 on Windows 10x64, Mac OS x 10.11 and Ubuntu 14.04x32 and the bug is not reproducing anymore.
Thanks Oana for your testing! Marking Verified fixed per comment 20.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: