951496 - WebRTC codec statistics - fps, bit rate and frame drops

Status: RESOLVED FIXED

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

Description

[StevenLee[:slee]]

Attachment: webrtcLog.patch

We need the statistics data for checking the status of codec.

Comment 1

[StevenLee[:slee]]

Attachment: webrtcLog.patch

Sorry, I uploaded wrong patch. This is the correct one.

Comment 2

[u459114]

Comment on attachment 8349152 [details] [diff] [review]
webrtcLog.patch

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

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +41,5 @@
>  class WebrtcVideoConduit:public VideoSessionConduit
>                           ,public webrtc::Transport
>                           ,public webrtc::ExternalRenderer
> +                         ,public webrtc::ViEEncoderObserver
> +                         ,public webrtc::ViEDecoderObserver

1. Implement webrtc::ViEEncoderObserver and  webrtc::ViEDecoderObserver inherited concrete classes, and aggregate these two observers in WebrtcVideoConduit.
2. Bypass the data you need from WebrtcVideoConduit, e.g. mPtrViECodec, into those observers.

Comment 3

[StevenLee[:slee]]

(In reply to C.J. Ku[:CJKu] from comment #2)
> 1. Implement webrtc::ViEEncoderObserver and  webrtc::ViEDecoderObserver
> inherited concrete classes, and aggregate these two observers in
> WebrtcVideoConduit.
> 2. Bypass the data you need from WebrtcVideoConduit, e.g. mPtrViECodec, into
> those observers.
ViEEncoderObserver and ViEDecoderObserver are 2 pure virtual classes, why not just inherited them? There should be only few overhead. Implementing them as objects, we need extra-memory to save them and the related pointer, such as mPtrViECodec.

Comment 4

[u459114]

(In reply to StevenLee[:slee] from comment #3)
> (In reply to C.J. Ku[:CJKu] from comment #2)
> > 1. Implement webrtc::ViEEncoderObserver and  webrtc::ViEDecoderObserver
> > inherited concrete classes, and aggregate these two observers in
> > WebrtcVideoConduit.
> > 2. Bypass the data you need from WebrtcVideoConduit, e.g. mPtrViECodec, into
> > those observers.
> ViEEncoderObserver and ViEDecoderObserver are 2 pure virtual classes, why
> not just inherited them? There should be only few overhead. Implementing
> them as objects, we need extra-memory to save them and the related pointer,
> such as mPtrViECodec.
I don't think it's a good idea to put whole thing into a single class. It's not relative to memory overhead or any implementation limitation, it's just a design logic: keep a single object handle as less things as possible

Comment 5

[StevenLee[:slee]]

Attachment: webrtcLog.patch

After discussing with CJ, I added some more statistics data, standard deviation, minimum and maximum values.

Comment 6

[Mandyam Vikram]

Isn't this going into the v2.0 release? if so, I think we should start tracking this.

Comment 7

[Maire Reavy [:mreavy]]

We are tracking this as part of the WebRTC Platform release for Fx 32 (which aligns with v2.0).  Please see the following if you want more details: https://wiki.mozilla.org/Media/WebRTC#Sprint:_mozilla32_WebRTC_Platform

Comment 8

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

Attachment: Part 1: statistics data for checking the status of codec (2)

Unbitrotted.
- Added mDroppedFrames and mDiscardedPackets (both accumulators).

Notes to self: This patch gathers
- encoder outgoing framerate, bitrate and dropped frames
- incoming framerate, bitrate, and discarded packets

For the rate-values, it calculates min, max, mean, std.deviation, and stores history. I have some concern about this scaling and am looking into calculating std.deviation without needing to hold captured values forever. 

We should also figure out which of theses values to expose to stats and telemetry (all of them?)

Comment 9

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

Comment on attachment 8431097 [details] [diff] [review]
Part 1: statistics data for checking the status of codec (2)

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

::: media/webrtc/signaling/src/media-conduit/CodecStatistics.cpp
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

mode lines per style guide

@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "CSFLog.h"
> +#include "CodecStatistics.h"

swap order here per style guide

@@ +15,5 @@
> +  mChannel(channel),
> +  mSentRawFrames(0),
> +  mPtrViECodec(codec),
> +  mEncoderDroppedFrames(0),
> +  mDecoderDiscardedPackets(0) {

{ on left edge (for all functions)

@@ +16,5 @@
> +  mSentRawFrames(0),
> +  mPtrViECodec(codec),
> +  mEncoderDroppedFrames(0),
> +  mDecoderDiscardedPackets(0) {
> +  mPtrViECodec->RegisterEncoderObserver(mChannel, *this);

Add MOZ_ASSERT(mPtrViECodec);

@@ +26,5 @@
> +  mPtrViECodec->DeregisterDecoderObserver(mChannel);
> +}
> +
> +void VideoCodecStatistics::OutgoingRate(const int video_channel,
> +    const unsigned int framerate, const unsigned int bitrate) {

indents

@@ +27,5 @@
> +}
> +
> +void VideoCodecStatistics::OutgoingRate(const int video_channel,
> +    const unsigned int framerate, const unsigned int bitrate) {
> +  unsigned int keyFrames, deltaFrames;

concrete size (uint32_t)

@@ +28,5 @@
> +
> +void VideoCodecStatistics::OutgoingRate(const int video_channel,
> +    const unsigned int framerate, const unsigned int bitrate) {
> +  unsigned int keyFrames, deltaFrames;
> +  mPtrViECodec->GetSendCodecStastistics(video_channel, keyFrames, deltaFrames);

Typo (Statistics)

@@ +31,5 @@
> +  unsigned int keyFrames, deltaFrames;
> +  mPtrViECodec->GetSendCodecStastistics(video_channel, keyFrames, deltaFrames);
> +  uint32_t dropped = mSentRawFrames - (keyFrames + deltaFrames);
> +  CSFLogDebug(logTag,
> +      "encoder statistics - framerate: %d, bitrate: %d, dropped frames: %d",

Indent; Probably all %u's

@@ +39,5 @@
> +  mEncoderDroppedFrames += dropped;
> +}
> +
> +void VideoCodecStatistics::IncomingCodecChanged(const int video_channel,
> +    const webrtc::VideoCodec& video_codec) {

indent

@@ +40,5 @@
> +}
> +
> +void VideoCodecStatistics::IncomingCodecChanged(const int video_channel,
> +    const webrtc::VideoCodec& video_codec) {
> +  CSFLogDebug(logTag, "channel %d change codec to %d ", video_channel, video_codec);

(int) video_codec perhaps

@@ +44,5 @@
> +  CSFLogDebug(logTag, "channel %d change codec to %d ", video_channel, video_codec);
> +}
> +
> +void VideoCodecStatistics::IncomingRate(const int video_channel,
> +    const unsigned int framerate, const unsigned int bitrate) {

indent

@@ +45,5 @@
> +}
> +
> +void VideoCodecStatistics::IncomingRate(const int video_channel,
> +    const unsigned int framerate, const unsigned int bitrate) {
> +  unsigned int discarded = mPtrViECodec->GetDiscardedPackets(video_channel);

unsigned int is correct here

@@ +48,5 @@
> +    const unsigned int framerate, const unsigned int bitrate) {
> +  unsigned int discarded = mPtrViECodec->GetDiscardedPackets(video_channel);
> +  CSFLogDebug(logTag,
> +      "decoder statistics - framerate: %d, bitrate: %d, discarded packets %d",
> +      framerate, bitrate, discarded);

Indent; %u %u %u

::: media/webrtc/signaling/src/media-conduit/CodecStatistics.h
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

add mode lines per style guide

@@ +7,5 @@
> +#include "nsISupportsImpl.h"
> +
> +#include "webrtc/video_engine/include/vie_codec.h"
> +
> +#include <math.h>

<> includes before mozilla includes unless you can't for some reason

@@ +9,5 @@
> +#include "webrtc/video_engine/include/vie_codec.h"
> +
> +#include <math.h>
> +
> +class webrtc::ViECodec;

Either remove this, or remove the include of vie_codec.h

@@ +12,5 @@
> +
> +class webrtc::ViECodec;
> +
> +namespace mozilla {
> +class Statistics {

Feels like this should live somewhere else (Telemetry?  mfbt?), and be a template (allowing  more than just uint32_t data to be accumulated).  At least add a comment to that effect, but I'd prefer it simply move into it's own header file.

@@ +18,5 @@
> +  Statistics() : mMin(0)
> +                ,mMax(0)
> +                ,mSum(0)
> +  {
> +  }

{}

@@ +25,5 @@
> +    if (mData.Length() == 0) {
> +      mMin = data;
> +      mMax = data;
> +    }
> +    mData.AppendElement(data);

This means it will hold an infinite amount of data, just in order to calculate StandardDeviation.  Probably not a great tradeoff, unless we know the size is bounded and/or will not be held for long.

I suggest http://en.wikipedia.org/wiki/Algorithms_for_calculating_variance#Incremental_Algorithm

@@ +38,5 @@
> +
> +    mSum += data;
> +  }
> +
> +  float Mean() {

const

@@ +42,5 @@
> +  float Mean() {
> +    return (float)(mSum/mData.Length());
> +  }
> +
> +  float StandardDeviation() {

const

@@ +45,5 @@
> +
> +  float StandardDeviation() {
> +    float mean = Mean();
> +    float variance = 0.0;
> +    for (uint32_t i = 0; i < mData.Length(); i++) {

see above about algorithm

@@ +51,5 @@
> +    }
> +    return sqrtf(variance/mData.Length());
> +  }
> +
> +  uint32_t Min() {

const

@@ +55,5 @@
> +  uint32_t Min() {
> +    return mMin;
> +  }
> +
> +  uint32_t Max() {

const

@@ +65,5 @@
> +  uint32_t mMax;
> +  uint32_t mSum;
> +};
> +
> +class VideoCodecStatistics : public webrtc::ViEEncoderObserver

comments?

@@ +71,5 @@
> +{
> +public:
> +  VideoCodecStatistics(int channel, webrtc::ViECodec* vieCodec);
> +  ~VideoCodecStatistics();
> +  void InsertOneFrame();

SentFrame() perhaps?  InsertOneFrame is kinda the wrong name...

@@ +73,5 @@
> +  VideoCodecStatistics(int channel, webrtc::ViECodec* vieCodec);
> +  ~VideoCodecStatistics();
> +  void InsertOneFrame();
> +  virtual void OutgoingRate(const int video_channel,
> +    const unsigned int framerate, const unsigned int bitrate);

be consistent about indents (in this case, have it line up with previous argument)

@@ +84,5 @@
> +                            const unsigned int bitrate);
> +
> +  virtual void RequestNewKeyFrame(const int video_channel);
> +  void Dump();
> +  NS_INLINE_DECL_REFCOUNTING(VideoCodecStatistics);

move up to after ~foo()

However, I don't see this as needed; it lives in one struct (VideoConduit); and it could be an nsAutoPtr<> there instead of a nsRefPtr<>

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +28,5 @@
>  #include <algorithm>
>  #include <math.h>
>  
> +using namespace webrtc;
> +

Let's not add this for this patch; this file has lots of webrtc::'s in it already

@@ +119,5 @@
>      mVideoEngine = nullptr;
>    } else {
>      // We can't delete the VideoEngine until all these are released!
>      // And we can't use a Scoped ptr, since the order is arbitrary
> +    mVideoCodecStat = nullptr;

This is slightly different than the rest; put it before the comment with it's own comment about releasing reference to the ViECodec - or even better perhaps right before setting mPtrViECodec to nullptr.

Comment 10

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

> > +void VideoCodecStatistics::IncomingCodecChanged(const int video_channel,
> > +    const webrtc::VideoCodec& video_codec) {
> > +  CSFLogDebug(logTag, "channel %d change codec to %d ", video_channel, video_codec);
> 
> (int) video_codec perhaps

I'll do video_codec.mPName.

> > +class Statistics {
> 
> Feels like this should live somewhere else (Telemetry?  mfbt?), and be a
> template (allowing  more than just uint32_t data to be accumulated).  At
> least add a comment to that effect, but I'd prefer it simply move into it's
> own header file.

Discussed with jesup and keeping it here for now while we adapt it for our needs.

> This means it will hold an infinite amount of data, just in order to
> calculate StandardDeviation.  Probably not a great tradeoff, unless we know
> the size is bounded and/or will not be held for long.
> 
> I suggest
> http://en.wikipedia.org/wiki/
> Algorithms_for_calculating_variance#Incremental_Algorithm

This was my concern as well, I'm looking into this. Agree with all remaining comments. Thanks!

Comment 11

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

Attachment: Part 1: fix Stastistics typo in vie_codec

Broke out typo fix into separate patch.

Comment 12

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

Attachment: Part 2: statistics data for checking the status of codec WIP (3)

Unbitrot and updated to work with 3.50. Addressed most feedback, except:

Working on adding MovingAverage and a Standard deviation without accumulating history next.

Comment 13

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

Attachment: Part 2: statistics data for checking the status of codec (4)

- Replaced Statistics class with RunningStat from
  http://www.johndcook.com/standard_deviation.html
- Dropping min/max. Doing doing mean and std deviation.

I hope this algorithm is good enough for both on-demand stats and PER_CALL telemetry at the end.

Working on tying it to our getStats code in part 3.

Comment 14

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

Attachment: Part 2: statistics data for checking the status of codec (5)

Whoops, wrong patch. This is the right one.

Comment 15

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

Comment on attachment 8434454 [details] [diff] [review]
Part 2: statistics data for checking the status of codec (5)

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

r+ but will need license/copyright clarification before landing, or recoding the algorithm.

::: media/webrtc/signaling/src/media-conduit/CodecStatistics.cpp
@@ +11,5 @@
> +
> +// use the same tag as VideoConduit
> +static const char* logTag ="WebrtcVideoSessionConduit";
> +
> +void VideoCodecStatistics::RunningStat::Push(double x)

Move to file...

@@ +14,5 @@
> +
> +void VideoCodecStatistics::RunningStat::Push(double x)
> +{
> +  mN++;
> +  

spaces (here and later)

::: media/webrtc/signaling/src/media-conduit/CodecStatistics.h
@@ +15,5 @@
> +
> +class VideoCodecStatistics : public webrtc::ViEEncoderObserver
> +                           , public webrtc::ViEDecoderObserver
> +{
> +  // From "Accurately computing running variance - John D. Cook"

Include URL & copyright status
Should be broken into a separate file if not pure PD.  even then, probably.

@@ +78,5 @@
> +                             int current_delay_ms,
> +                             int target_delay_ms,
> +                             int jitter_buffer_ms,
> +                             int min_playout_delay_ms,
> +                             int render_delay_ms) OVERRIDE {}

MOZ_OVERRIDE

@@ +83,5 @@
> +  void Dump();
> +private:
> +  int mChannel;
> +  uint32_t mSentRawFrames;
> +  webrtc::ViECodec* mPtrViECodec;

Ok, we need to be careful with these.  Raw ptrs == dangerous.  ScopedCustomReleasePtr?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +120,5 @@
>      // We can't delete the VideoEngine until all these are released!
>      // And we can't use a Scoped ptr, since the order is arbitrary
>      mPtrViEBase = nullptr;
>      mPtrViECapture = nullptr;
> +    mVideoCodecStat = nullptr; // has ptr to mPtrViECodec

move this to one end or the other; out of the list of ScopedCustomReleasePtrs

More explicit comment as to why we have to null it here.

Comment 16

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

Attachment: Part 1: fix Stastistics typo in vie_codec (2) r=jesup

Updated commit msg.

Comment 17

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

Attachment: Part 2: statistics data for checking the status of codec (6) r=jesup

Incorporated all feedback. Carrying forward r=jesup.

Comment 18

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

Attachment: Part 3: codec getStats WIP

Work in progress.
- Encoder settings work. Decoder side not appearing yet (not inited properly)

Next is hooking up about:webrtc and PER_CALL telemetry.

Comment 19

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

Attachment: specstats.html - Test page displaying bitrate, framerate and dropped frames.

Comment 20

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

Comment on attachment 8435321 [details] [diff] [review]
Part 3: codec getStats WIP

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

Overall looks good; will need DOM peer of course

::: media/webrtc/signaling/src/media-conduit/AudioConduit.h
@@ +181,5 @@
> +  bool GetVideoEncoderStats(double* framerateMean,
> +                            double* framerateStdDev,
> +                            double* bitrateMean,
> +                            double* bitrateStdDev,
> +                            uint32_t* droppedFrames);

Just { return false; } here for not-relevant base-class funcs

Comment 21

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

Comment on attachment 8434671 [details] [diff] [review]
Part 1: fix Stastistics typo in vie_codec (2) r=jesup

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

This patch should be submitted to upstream (after uplift)

Comment 22

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

Attachment: Part 3: codec getStats (2)

- Works. Had to put a stent in the VideoStatistics constructor to not
  register both encoder and decoder observer unless needed, as it was
  competing with other instances for this single-observer callback.
  Hopefully not too ugly.

- Added to about:webrtc

- Webidl needs dom review (should be trivial, just dictionary changes).

Comment 23

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

Attachment: specstats.html - Test page displaying bitrate, framerate and dropped frames (2)

Comment 24

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

Attachment: Part 4: codec telemetry

That should be it. Try - https://tbpl.mozilla.org/?tree=Try&rev=52291a4eeab4

Comment 25

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

Comment on attachment 8436269 [details] [diff] [review]
Part 4: codec telemetry

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

::: toolkit/components/telemetry/Histograms.json
@@ +5340,5 @@
>    },
> +  "WEBRTC_VIDEO_ENCODER_BITRATE_AVG_PER_CALL_KBPS": {
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "high": 100000,

Is this too high?  Highest I expect is 2Mbps (2000 here, low will be 200 typ; perhaps 100ish if a drop the lower bound a bit); of course it's exponential...  Perhaps this is fine.

Comment 26

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8436219 [details] [diff] [review]
Part 3: codec getStats (2)

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

Why does this violate our no-vendor-prefixes policy?

Comment 27

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

(In reply to :Ms2ger from comment #26)
> Why does this violate our no-vendor-prefixes policy?

As discussed on IRC and per https://wiki.mozilla.org/WebAPI/ExposureGuidelines I'll change the patch to make these additions chrome-only for now, so we can land it for about:webrtc and telemetry.

Comment 28

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

Attachment: Part 3: codec getStats (3) r=jesup

- Made new moz-prefixed stats dictionary members internal-only for now.
- I'll take the first DOM review I can get. ;-)

Comment 29

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

Attachment: Part 4: codec telemetry (2) r=jesup

Addressed nits. Carrying forward r=jesup.

- Reduced bitrate ceilings in histogram by an order of 10.

Comment 30

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

Comment on attachment 8436219 [details] [diff] [review]
Part 3: codec getStats (2)

un-obsoleting for further consideration

Comment 31

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

Comment on attachment 8436219 [details] [diff] [review]
Part 3: codec getStats (2)

Reinstating r? to smaug

Per discussion in #content, this stats interface is under active development in the WG and both we and google are experimenting with uses.  It lives behind a moz-prefixed API (mozRTCPeerConnection), so this doesn't expose a new moz API to the web.  Also, per the discussion in SpecialCases in the policy, this is important to FxOS apps being developed, where about:webrtc isn't practical to use or ask users to use and cut-and-paste (and apps want to report back bandwidth and framerates).

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8436219 [details] [diff] [review]
Part 3: codec getStats (2)

I don't really see reasons to use prefixes here.

If we think the API is stable enough, just add non-prefixed stuff, and in the
other case we need to be ready to change the API anyway, whether or not 
we have prefixes.

(We should try hard to not add more prefixed stuff to web, and 
mozRTCPeerConnection is already prefixed. So if prefixes bring in anything good, 
that should be enough.)

Comment 33

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

Attachment: Part 3: codec getStats (4) r=smaug, r=jesup

Got r+ from smaug on IRC in #developers an hour ago. Carrying forward r+ from jesup.

- Removed 'moz' prefixes from new stats dictionary members.
- Removed chrome-only from  new stats dictionary members.

Comment 34

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

Attachment: Part 4: codec telemetry (3) r=jesup

Unbitrot (removed 'moz' prefixes from new stats). Carrying forward r+ from jesup.

Comment 35

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

Attachment: specstats.html - Test page displaying bitrate, framerate and dropped frames (3)

Unbitrot (removed 'moz' prefixes from new stats).

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99dc3a6c8432
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dd8777a4453
https://hg.mozilla.org/integration/mozilla-inbound/rev/e30a256cb1fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8f0896469c7

Comment 37

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Why did this land with standing r-?

Comment 38

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

ms2ger: see comment 33 - verbal r+ from smaug on IRC given removing the moz prefixes; he didn't go back and update the bug

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

well, r+ patch with moz-prefix would be tiny bit odd.

But my understanding was that we're ok exposing the API as such, and prefixes aren't needed.
So, r+.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99dc3a6c8432
https://hg.mozilla.org/mozilla-central/rev/6dd8777a4453
https://hg.mozilla.org/mozilla-central/rev/e30a256cb1fc
https://hg.mozilla.org/mozilla-central/rev/b8f0896469c7