1241064 - getStats API returns 0 values for some attributes for audio

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

Looking at stats reported by Firefox (both 43 and 45) I see some attributes always being 0, per the following:

For { type: inboundrtp, isRemote: false, mediaType: audio }, packetsLost is always 0
For { type: inboundrtp, isRemote: false, mediaType: audio }, jitter is always 0

Comment 1

[Nico Grunbaum [:ng, @chew:mozilla.org]]

This issue does not appear to have shown up at the same time as bug 1241321. Via mozregression I looked at versions 30 : 39. The last version that I was able to observe jitter reported was 31. Note: I did not test in an environment that was producing significant jitter, and the highest value I saw was 0.002.

Comment 2

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Fix for try: -b o -p linux64,macosx64,win32 -u mochitests -t none

Comment 3

[Nico Grunbaum [:ng, @chew:mozilla.org]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=11012635fe87

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8723738 [details] [diff] [review]
Fix for  try: -b o -p linux64,macosx64,win32 -u mochitests -t none

Review of attachment 8723738 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits. Do we need to uplift this?

::: media/webrtc/trunk/webrtc/voice_engine/channel.cc
@@ +65,3 @@
>        return;
>  
>      CriticalSectionScoped cs(stats_lock_.get());

Design nit: This obtains the critical section twice. If we moved the above if-statement below this line instead, then we could just do:

   if (ssrc != ssrc_)
      return;

And we wouldn't need GetSSRC(). Simpler and less overhead IMHO (I'm not a fan of calling sibling member functions for this reason).

@@ +76,5 @@
> +  uint32_t GetSSRC() const {
> +    uint32_t ssrc;
> +    {
> +      CriticalSectionScoped cs(stats_lock_.get());
> +      ssrc = ssrc_;

Nit: (If we keep GetSSRC) While I'm a fan of using brackets for scope like this, they seem unnecessary here.

  uint32_t GetSSRC() const {
    CriticalSectionScoped cs(stats_lock_.get());
    return ssrc_;
  }

would seem to suffice?

Comment 5

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: bug1241064-r2.patch

Nit fixes.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8726006 [details] [diff] [review]
bug1241064-r2.patch

Review of attachment 8726006 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/trunk/webrtc/voice_engine/channel.cc
@@ +73,5 @@
>  
>    void CNameChanged(const char* cname, uint32_t ssrc) override {}
>  
> +  uint32_t GetSSRC() const {
> +    CriticalSectionScoped cs(stats_lock_.get());

Isn't this dead code? I would defer adding this till when/if needed, but up to you.

Comment 7

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: bug1241064-r3.patch

Removing GetSSRC()

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8726257 [details] [diff] [review]
bug1241064-r3.patch

Review of attachment 8726257 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm (note that with 'r=me with nits' you don't have to request review again if you're just fixing the nits).

Comment 9

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: MozReview Request: fix bug 1241064 - updating stats filter SSRC when audio channel SSRC changes; r?jib

Collapsing review edits
* Fixing nits.
* Removing GetSSRC()

Review commit: https://reviewboard.mozilla.org/r/37901/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37901/

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8726278 [details]
MozReview Request: fix bug 1241064 - updating stats filter SSRC when audio channel SSRC changes; r?jib

https://reviewboard.mozilla.org/r/37901/#review34441

Comment 11

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8726278 [details]
MozReview Request: fix bug 1241064 - updating stats filter SSRC when audio channel SSRC changes; r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37901/diff/1-2/

Comment 12

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8726278 [details]
MozReview Request: fix bug 1241064 - updating stats filter SSRC when audio channel SSRC changes; r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37901/diff/2-3/

Comment 13

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: MozReview Request: bug 1241064 - updating stats filter SSRC when audio channel SSRC changes; r?jib

now updating SSRC filter on statistics update callback to match audio channel ssrc
getStats API now returns statistics for correct SSRC: jitter, packets lost, etc.

Review commit: https://reviewboard.mozilla.org/r/37933/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37933/

Comment 14

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8726342 [details]
MozReview Request: bug 1241064 - updating stats filter SSRC when audio channel SSRC changes; r?jib

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37933/diff/1-2/

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8726342 [details]
MozReview Request: bug 1241064 - updating stats filter SSRC when audio channel SSRC changes; r?jib

https://reviewboard.mozilla.org/r/37933/#review34509

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7209701f8975

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7209701f8975

Comment 18

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

Nico, could you request uplift for this if it goes cleanly?

Comment 19

[Nico Grunbaum [:ng, @chew:mozilla.org]]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #18)
> Nico, could you request uplift for this if it goes cleanly?

Sure.

Comment 20

[Maire Reavy [:mreavy]]

Comment on attachment 8726342 [details]
MozReview Request: bug 1241064 - updating stats filter SSRC when audio channel SSRC changes; r?jib

Approval Request Comment
[Feature/regressing bug #]: Regressed prior to Firefox 40
[User impact if declined]: No stats reported for some audio attributes which breaks some applications that rely on it.  This makes it impossible for applications to track certain, important aspects of call quality.
[Describe test coverage new/current, TreeHerder]: No automated test coverage; it can viewed manually 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 (including at least one partner site); without this we/they can't track important aspects of call quality.
[String/UUID change made/needed]: No strings

Comment 21

[Liz Henry (:lizzard)]

Comment on attachment 8726342 [details]
MozReview Request: bug 1241064 - updating stats filter SSRC when audio channel SSRC changes; r?jib

Call quality improvements, fix for an older regression.
This should land in beta 2.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0905c9f4d55c