Closed
Bug 1241064
Opened 8 years ago
Closed 8 years ago
getStats API returns 0 values for some attributes for audio
Categories
(Core :: WebRTC, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla47
backlog | webrtc/webaudio+ |
People
(Reporter: pehrsons, Assigned: ng)
Details
(Keywords: regression)
Attachments
(1 file, 4 obsolete files)
58 bytes,
text/x-review-board-request
|
jib
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
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
Assignee | ||
Comment 1•8 years ago
|
||
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.
Updated•8 years ago
|
backlog: --- → webrtc/webaudio+
Rank: 17
Priority: -- → P1
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → na-g
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11012635fe87
Assignee | ||
Updated•8 years ago
|
Attachment #8723738 -
Flags: review?(jib)
Comment 4•8 years ago
|
||
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?
Attachment #8723738 -
Flags: review?(jib) → review+
Updated•8 years ago
|
Flags: needinfo?(rjesup)
Assignee | ||
Comment 5•8 years ago
|
||
Nit fixes.
Attachment #8723738 -
Attachment is obsolete: true
Attachment #8726006 -
Flags: review?(jib)
Comment 6•8 years ago
|
||
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.
Attachment #8726006 -
Flags: review?(jib) → review+
Assignee | ||
Comment 7•8 years ago
|
||
Removing GetSSRC()
Attachment #8726006 -
Attachment is obsolete: true
Attachment #8726257 -
Flags: review?(jib)
Comment 8•8 years ago
|
||
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).
Attachment #8726257 -
Flags: review?(jib) → review+
Assignee | ||
Comment 9•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Attachment #8726278 -
Flags: review?(jib)
Comment 10•8 years ago
|
||
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
Attachment #8726278 -
Flags: review?(jib) → review+
Assignee | ||
Comment 11•8 years ago
|
||
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/
Attachment #8726278 -
Attachment description: MozReview Request: bug1241064 - reviewed → MozReview Request: fix bug 1241064 getStats API returns 0 values for some attributes for audio; r?jib
Updated•8 years ago
|
Attachment #8726257 -
Attachment is obsolete: true
Updated•8 years ago
|
Keywords: regression
Assignee | ||
Comment 12•8 years ago
|
||
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/
Attachment #8726278 -
Attachment description: MozReview Request: fix bug 1241064 getStats API returns 0 values for some attributes for audio; r?jib → MozReview Request: fix bug 1241064 - updating stats filter SSRC when audio channel SSRC changes; r?jib
Assignee | ||
Updated•8 years ago
|
Attachment #8726278 -
Attachment is obsolete: true
Assignee | ||
Comment 13•8 years ago
|
||
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/
Attachment #8726342 -
Flags: review?(jib)
Assignee | ||
Comment 14•8 years ago
|
||
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/
Attachment #8726342 -
Attachment description: MozReview Request: fix bug 1241064 - updating stats filter SSRC when audio channel SSRC changes; r?jib → MozReview Request: bug 1241064 - updating stats filter SSRC when audio channel SSRC changes; r?jib
Comment 15•8 years ago
|
||
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
Attachment #8726342 -
Flags: review?(jib) → review+
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7209701f8975
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Reporter | ||
Comment 18•8 years ago
|
||
Nico, could you request uplift for this if it goes cleanly?
Flags: needinfo?(na-g)
Assignee | ||
Comment 19•8 years ago
|
||
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #18) > Nico, could you request uplift for this if it goes cleanly? Sure.
Flags: needinfo?(na-g)
Comment 20•8 years ago
|
||
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
Attachment #8726342 -
Flags: approval-mozilla-beta?
Comment 21•8 years ago
|
||
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.
Attachment #8726342 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 22•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/0905c9f4d55c
status-firefox46:
--- → fixed
Updated•8 years ago
|
status-firefox45:
--- → wontfix
Updated•4 years ago
|
Flags: needinfo?(rjesup)
You need to log in
before you can comment on or make changes to this bug.
Description
•