Closed Bug 951496 Opened 6 years ago Closed 6 years ago

WebRTC codec statistics - fps, bit rate and frame drops

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: slee, Assigned: jib)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [p=1])

Attachments

(5 files, 15 obsolete files)

8.06 KB, patch
jib
: review+
Details | Diff | Splinter Review
12.63 KB, patch
jib
: review+
Details | Diff | Splinter Review
23.57 KB, patch
jib
: review+
Details | Diff | Splinter Review
7.77 KB, patch
jib
: review+
Details | Diff | Splinter Review
17.25 KB, text/html
Details
Attached patch webrtcLog.patch (obsolete) — Splinter Review
We need the statistics data for checking the status of codec.
Attachment #8349152 - Flags: review?(rjesup)
Attachment #8349152 - Flags: feedback?(cku)
Attached patch webrtcLog.patch (obsolete) — Splinter Review
Sorry, I uploaded wrong patch. This is the correct one.
Attachment #8349152 - Attachment is obsolete: true
Attachment #8349152 - Flags: review?(rjesup)
Attachment #8349152 - Flags: feedback?(cku)
Attachment #8349286 - Flags: review?(rjesup)
Flags: needinfo?(cku)
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.
(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.
(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
Flags: needinfo?(cku)
Attached patch webrtcLog.patch (obsolete) — Splinter Review
After discussing with CJ, I added some more statistics data, standard deviation, minimum and maximum values.
Attachment #8349286 - Attachment is obsolete: true
Attachment #8349286 - Flags: review?(rjesup)
Blocks: 964161
Summary: WbeRTC codec statistics - fps, bit rate and frame drops → WebRTC codec statistics - fps, bit rate and frame drops
Assignee: nobody → slee
Blocks: 970426
No longer depends on: 970426
Assignee: slee → jib
Priority: -- → P2
Whiteboard: [p=2]
Target Milestone: --- → mozilla32
Isn't this going into the v2.0 release? if so, I think we should start tracking this.
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
Blocks: 1011547
Blocks: 1011683
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?)
Flags: needinfo?(rjesup)
Attachment #8350551 - Attachment is obsolete: true
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.
> > +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!
Flags: needinfo?(rjesup)
Broke out typo fix into separate patch.
Attachment #8433628 - Flags: review?(rjesup)
Unbitrot and updated to work with 3.50. Addressed most feedback, except:

Working on adding MovingAverage and a Standard deviation without accumulating history next.
Attachment #8431097 - Attachment is obsolete: true
Attachment #8433628 - Flags: review?(rjesup) → review+
- 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.
Attachment #8433631 - Attachment is obsolete: true
Attachment #8434451 - Flags: review?(rjesup)
Whoops, wrong patch. This is the right one.
Attachment #8434451 - Attachment is obsolete: true
Attachment #8434451 - Flags: review?(rjesup)
Attachment #8434454 - Flags: review?(rjesup)
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.
Attachment #8434454 - Flags: review?(rjesup) → review+
Updated commit msg.
Attachment #8433628 - Attachment is obsolete: true
Attachment #8434671 - Flags: review+
Incorporated all feedback. Carrying forward r=jesup.
Attachment #8434454 - Attachment is obsolete: true
Attachment #8434673 - Flags: review+
Attached patch Part 3: codec getStats WIP (obsolete) — Splinter Review
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 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 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)
Attached patch Part 3: codec getStats (2) (obsolete) — Splinter Review
- 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).
Attachment #8435321 - Attachment is obsolete: true
Attachment #8436219 - Flags: review?(rjesup)
Attachment #8436219 - Flags: review?(bugs)
Attachment #8436219 - Flags: review?(rjesup) → review+
Attached patch Part 4: codec telemetry (obsolete) — Splinter Review
That should be it. Try - https://tbpl.mozilla.org/?tree=Try&rev=52291a4eeab4
Attachment #8436269 - Flags: review?(rjesup)
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.
Attachment #8436269 - Flags: review?(rjesup) → review+
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?
Attachment #8436219 - Flags: feedback-
(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.
- Made new moz-prefixed stats dictionary members internal-only for now.
- I'll take the first DOM review I can get. ;-)
Attachment #8436219 - Attachment is obsolete: true
Attachment #8436219 - Flags: review?(bugs)
Attachment #8436318 - Flags: review?(peterv)
Attachment #8436318 - Flags: review?(bugs)
Addressed nits. Carrying forward r=jesup.

- Reduced bitrate ceilings in histogram by an order of 10.
Attachment #8436269 - Attachment is obsolete: true
Attachment #8436320 - Flags: review+
Comment on attachment 8436219 [details] [diff] [review]
Part 3: codec getStats (2)

un-obsoleting for further consideration
Attachment #8436219 - Attachment is obsolete: false
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).
Attachment #8436219 - Flags: review?(bugs)
Attachment #8436219 - Flags: review?(peterv)
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.)
Attachment #8436219 - Flags: review?(peterv)
Attachment #8436219 - Flags: review?(bugs)
Attachment #8436219 - Flags: review-
Attachment #8436318 - Flags: review?(peterv)
Attachment #8436318 - Flags: review?(bugs)
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.
Attachment #8436219 - Attachment is obsolete: true
Attachment #8436318 - Attachment is obsolete: true
Attachment #8436383 - Flags: review+
Unbitrot (removed 'moz' prefixes from new stats). Carrying forward r+ from jesup.
Attachment #8436320 - Attachment is obsolete: true
Attachment #8436384 - Flags: review+
Unbitrot (removed 'moz' prefixes from new stats).
Attachment #8436220 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [p=2] → [p=1]
Why did this land with standing r-?
Flags: needinfo?(jib)
ms2ger: see comment 33 - verbal r+ from smaug on IRC given removing the moz prefixes; he didn't go back and update the bug
Flags: needinfo?(jib)
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+.
No longer blocks: webrtc_upstream_bugs
You need to log in before you can comment on or make changes to this bug.