1069047 - WebRTC encoder statistics reports incorrect dropped frames for H.264

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Jay]

It appears that SPS/PPS are counts as part of key frame and causes incorrect dropped frame count from the following equation:
uint32_t dropped = mSentRawFrames - (keyFrames + deltaFrames);Please, refer to [1] for the complete source.

This causes the dropped count become negative.

[1]http://git.mozilla.org/?p=releases/gecko.git;a=blob;f=media/webrtc/signaling/src/media-conduit/CodecStatistics.cpp;h=39b7fd87ed11f1965e044708c2a9b9e370b5ae3d;hb=HEAD#l54

Comment 1

[Jay]

[Blocking Requested - why for this release]:

Comment 2

[bhavana bajaj [:bajaj]]

:mreavy, can you help comment on this one ? Do any blockers that we've fixed, to support H.264 may have caused this?

Comment 3

[Maire Reavy [:mreavy]]

This bug goes back to the beginning basically.  I've asked jesup to take a quick look at this to see if there's an easy fix.

Comment 4

[Andrew Overholt [:overholt]]

CAF p2 = blocking+

Comment 5

[bhavana bajaj [:bajaj]]

NI :jesup for feedback on comment #3 and if we will be able to have a low risk fix here in the next week or so for 2.1

Comment 6

[Maire Reavy [:mreavy]]

Randell believes he may have a low risk solution for this.

Comment 7

[bhavana bajaj [:bajaj]]

Jesup, is having a patch here and is currently testing it out before landing..

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Only count one frame (key vs delta) in stats per timestamp

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8505032 [details] [diff] [review]
Only count one frame (key vs delta) in stats per timestamp

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: Incorrect/unusable statistics for dropped frames and keyframes vs delta frames.

Testing completed: Manually tested on desktop/Openh264

Risk to taking this patch (and alternatives if risky): Virtually no risk; just turns off bumping the stats values if the timestamp didn't change.

String or UUID changes made by this patch: none

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/556ad64e03e2

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/556ad64e03e2

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18b785142556

Do we need this on the Fx release branches as well?