Joe Reporter
	Description • 7 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

Go to this page:
https://webrtc.github.io/samples/src/content/peerconnection/constraints/
Click Get media and then Connect
Search for framesEncoded


Actual results:

framesEncoded is not found as it is not being returned


Expected results:

framesEncoded should be displayed representing the total number of frames successfully encoded for the RTP media stream. Open this page in Chrome to see this workign as expected.

	Nils Ohlmeier [:drno]
	Comment 1 • 7 years ago

Rather then taking Chrome as the reference lets use the spec: https://www.w3.org/TR/webrtc-stats/#dom-rtcoutboundrtpstreamstats-framesencoded

Niko is this one of the stats we are working on already?

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 2 • 7 years ago

Nils, yes and in fact t his depends on and can probably be easily done at the same time as the framesDropped fix, bug 1339134.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8849845 [details]
Bug 1348657 - implement framesEncoded, pliCount, nackCount and firCount for webrtc stats

https://reviewboard.mozilla.org/r/122574/#review124942

Lgtm. Just a nit or two, and a double-check question.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h:308
(Diff revision 1)
> -                            uint32_t* droppedFrames) override;
> +                            uint32_t* droppedFrames,
> +                            uint32_t* encodedFrames) override;

On naming:

We already have DroppedFrames and droppedFrames.
This patch adds FramesEncoded and encodedFrames.

It seems either FramesEncoded and framesEncoded,
or if we prefer encodedFrames and encodedFrames

would be more consistent?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h:365
(Diff revision 1)
>       * Returns the calculate number of dropped frames
>       * @param aOutDroppedFrames: the number of dropped frames
>       */
>      void DroppedFrames(uint32_t& aOutDroppedFrames) const;
> +    /**
> +     * Returns the number of frames that have been encoded

Maybe add:

    so far

?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:131
(Diff revision 1)
>      mDroppedFrames = mSentFrames - (fc.key_frames + fc.delta_frames);
> +    mFramesEncoded = fc.key_frames + fc.delta_frames;

Nit: If you flip these you can do:

    mDroppedFrames = mSentFrames - mFramesEncoded;

Just to double-check, since I had to read [1] twice. It says on mSentFrames: "Call once for every frame delivered for encoding". Is this the number of frames *input* to the encoder, or encoded frames? Sounds like the former, which would be good, though mSentFrames is a bit of a misnomer then IMHO.

[1] https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/media/webrtc/signaling/src/media-conduit/VideoConduit.h#364-369

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8849845 [details]
Bug 1348657 - implement framesEncoded, pliCount, nackCount and firCount for webrtc stats

https://reviewboard.mozilla.org/r/122574/#review125976

Looks good. Note I'd already r+'ed framesEncoded, so there was an opportunity here to avoid losing an r+ and make review easier by logically dividing new work into new additional patches instead. Hoping for that going forward. r- for early return in constructor + various nits.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:193
(Diff revision 2)
> +      // nackCount
> +      if (!stat.inner.isRemote) {
> +        ok(stat.nackCount >= 0, stat.type + ".nackCount is sane.");
> +      } else {
> +        is(stat.nackCount, undefined, stat.type
> +          + ".nackCount is only set when isRemote is false");
> +      }
> +

This code seems duplicated unnecessarily. Can we move it up to the "Common RTCStreamStats fields" section?

::: dom/media/tests/mochitest/test_peerConnection_stats.html:214
(Diff revision 2)
>            } else { // mediaType != video
>              ok(stat[field] === undefined, stat.type + " does not have field "
>                + field + " when mediaType is not 'video'");
>            }
>          });
> -      } else {
> +      } else if (!stat.inner.isRemote && stat.inner.mediaType == "video") {

Redundant, hence confusing. Please remove. Leave a comment as a reminder of the invariant if you must.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:300
(Diff revision 2)
>        if (stat.inner.isRemote || stat.inner.mediaType != "video") {
>          expectations.videoOnly.forEach(field => {

(Not in this patch but) I notice this is effectively `expectations.localVideoOnly` as written.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:310
(Diff revision 2)
>            } else { // mediaType != video
>              ok(stat[field] === undefined, stat.type + " does not have field "
>                + field + " when mediaType is not 'video'");
>            }
>          });
> -      } else {
> +      } else if (!stat.inner.isRemote && stat.inner.mediaType == "video") {

Remove.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:358
(Diff revision 2)
> +        // firCount
> +        ok(stat.firCount >= 0 && stat.firCount < 100,
> +          stat.type + ".firCount is a sane number for a short test. value="
> +          + stat.firCount);
> +
> +        // pliCount
> +        ok(stat.pliCount >= 0 && stat.pliCount < 100,
> +          stat.type + ".pliCount is a sane number for a short test. value="
> +          + stat.pliCount);

Organizing comment: I think firCount and pliCount would benefit from moving to the "Common RTCStreamStats fields" section, as they live next to nackCount in the base dictionary in the spec, and this would avoid mostly duplicate code (we'd need to duplicate the test-condition instead):

    if (!stat.inner.isRemote && stat.inner.mediaType == "video") {

but that still seems like a win.

::: dom/webidl/RTCStatsReport.webidl:44
(Diff revision 2)
> +  // Video encoder measurement, not present in RTCP case
> +  unsigned long nackCount;

This comment says nackCount only applies to video. Doesn't gel with the code or the spec [1].

[1] https://w3c.github.io/webrtc-stats/#dom-rtcrtpstreamstats-nackcount

::: dom/webidl/RTCStatsReport.webidl:68
(Diff revision 2)
>    // Video encoder measurement (absent in rtcp case)
>    unsigned long droppedFrames;
> +
> +  // Video encoder measurement, not present in RTCP case
> +  unsigned long framesEncoded;

Nit: Extraneous comment. Maybe lump this in under the almost-identical existing comnment above droppedFrames, or at least use the same phrasing?

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:168
(Diff revision 2)
> +bool WebrtcAudioConduit::GetPacketTypeStats(
> +  webrtc::RtcpPacketTypeCounter* aPacketCounts)

Nit: consider 4 space indent of broken line, or the mozilla-loved:

    bool
    WebrtcAudioConduit::GetPacketTypeStats(webrtc::RtcpPacketTypeCounter* aPacketCounts)

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:126
(Diff revision 2)
>      mDroppedFrames = mSentFrames - (fc.key_frames + fc.delta_frames);
> +    mFramesEncoded = fc.key_frames + fc.delta_frames;

I commented on this in https://reviewboard.mozilla.org/r/122574/#review124942 yet it does not appear resolved. Did you miss it?

Though I haven't found official guidance on this, I prefer the submitter goes through the previous review(s) and marks items as either resolved or dropped before requesting review again. Then I'll know whether omission was by preference or accident. It's not a very watertight process otherwise.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:180
(Diff revision 2)
>    , mEngineTransmitting(false)
>    , mEngineReceiving(false)
>    , mCapId(-1)
>    , mCodecMutex("VideoConduit codec db")
>    , mInReconfig(false)
> +  , mPacketCounts()

nit: implicit and redundant. Do you prefer it listed on purpose? Seems inconsistent with other uses e.g. of this type in our tree.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:216
(Diff revision 2)
>      if (self->mEngineTransmitting && self->mSendStream) {
> -      self->mSendStreamStats.Update(self->mSendStream->GetStats());
> +      const webrtc::VideoSendStream::Stats& aStats =
> +        self->mSendStream->GetStats();
> +      self->mSendStreamStats.Update(aStats);
> +      if (aStats.substreams.empty()) {
> +        return;
> +      }
> +      self->mPacketCounts =
> +        aStats.substreams.begin()->second.rtcp_packet_type_counts;
>      }
>      if (self->mEngineReceiving && self->mRecvStream) {

Nit: An early return in a constructor seems risky to me. Can a conduit ever be both mEngineTransmitting && mEngineReceiving? (Code added elsewhere in this patch seems to think so). If so then mRecvStreamStats wouldn't be initialized/updated here if the early return hits.

Even if this can't happen in practice, people may add more intitialization below and wonder why it doesn't run.

Maybe invert the condition around the code it protects, to avoid the return?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:217
(Diff revision 2)
>    nsTimerCallbackFunc callback = [](nsITimer* aTimer, void* aClosure) {
>      CSFLogDebug(logTag, "StreamStats polling scheduled for VideoConduit: %p", aClosure);
>      auto self = static_cast<WebrtcVideoConduit*>(aClosure);
>      MutexAutoLock lock(self->mCodecMutex);
>      if (self->mEngineTransmitting && self->mSendStream) {
> -      self->mSendStreamStats.Update(self->mSendStream->GetStats());
> +      const webrtc::VideoSendStream::Stats& aStats =

s/aStats/stats/ also feel free to use auto&

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:781
(Diff revision 2)
> -                                         uint32_t* droppedFrames)
> +                                         uint32_t* droppedFrames,
> +                                         uint32_t* encodedFrames)

Here's that naming thing again. comment 2.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:3764
(Diff revision 2)
> +        // Fill in packet type statistics
> +        webrtc::RtcpPacketTypeCounter counters;
> +        if (mp.Conduit()->GetPacketTypeStats(&counters)) {

What happens if a conduit is both mEngineTransmitting && mEngineReceiving?

In that case we'd seem to be reporting an aggregate count of both sent and received, which seems wrong.

If this can't happen, then can we assert it here?

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8849845 [details]
Bug 1348657 - implement framesEncoded, pliCount, nackCount and firCount for webrtc stats

https://reviewboard.mozilla.org/r/122574/#review126298

Lgtm.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 9 • 7 years ago

Don't forget to request DOM review.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 7 years ago

(In reply to Nils Ohlmeier [:drno] from comment #1)
> Rather then taking Chrome as the reference lets use the spec:
> https://www.w3.org/TR/webrtc-stats/#dom-rtcoutboundrtpstreamstats-
> framesencoded

/TR stands for trash ;)
https://w3c.github.io/webrtc-stats/ is hopefully the right spec.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8849845 [details]
Bug 1348657 - implement framesEncoded, pliCount, nackCount and firCount for webrtc stats

https://reviewboard.mozilla.org/r/122574/#review127366

::: dom/webidl/RTCStatsReport.webidl:44
(Diff revision 3)
>    double bitrateStdDev;
>    double framerateMean;
>    double framerateStdDev;
> +
> +  // Local only measurements, RTCP related but not communicated via RTCP. Not
> +  // preseent in RTCP case.

preseent?

::: dom/webidl/RTCStatsReport.webidl:47
(Diff revision 3)
> +
> +  // Local only measurements, RTCP related but not communicated via RTCP. Not
> +  // preseent in RTCP case.
> +  unsigned long nackCount;
> +  unsigned long pliCount;
> +  unsigned long firCount;

Could you order these in the same order as in the spec

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 13 • 7 years ago
mozreview-review-reply

Comment on attachment 8849845 [details]
Bug 1348657 - implement framesEncoded, pliCount, nackCount and firCount for webrtc stats

https://reviewboard.mozilla.org/r/122574/#review125976

> Nit: consider 4 space indent of broken line, or the mozilla-loved:
> 
>     bool
>     WebrtcAudioConduit::GetPacketTypeStats(webrtc::RtcpPacketTypeCounter* aPacketCounts)

Gotcha, done.

> I commented on this in https://reviewboard.mozilla.org/r/122574/#review124942 yet it does not appear resolved. Did you miss it?
> 
> Though I haven't found official guidance on this, I prefer the submitter goes through the previous review(s) and marks items as either resolved or dropped before requesting review again. Then I'll know whether omission was by preference or accident. It's not a very watertight process otherwise.

I appear to have missed the whole first review, yikes, and appologies!

> nit: implicit and redundant. Do you prefer it listed on purpose? Seems inconsistent with other uses e.g. of this type in our tree.

I was not sure which way the wind was going to blow on this one. Removing.

> Nit: An early return in a constructor seems risky to me. Can a conduit ever be both mEngineTransmitting && mEngineReceiving? (Code added elsewhere in this patch seems to think so). If so then mRecvStreamStats wouldn't be initialized/updated here if the early return hits.
> 
> Even if this can't happen in practice, people may add more intitialization below and wonder why it doesn't run.
> 
> Maybe invert the condition around the code it protects, to avoid the return?

It is a return in a closure, which is created in a constructor, not a return from the constructor. Assertion added.

> What happens if a conduit is both mEngineTransmitting && mEngineReceiving?
> 
> In that case we'd seem to be reporting an aggregate count of both sent and received, which seems wrong.
> 
> If this can't happen, then can we assert it here?

I don't think that is possible. Each audio conduit only has a single "channel", RTP module, and RTCP module. There is no API to query the transmitting / receiving state of a conduit. I have added a MOZ_ASSERT in VideoConduit where the packet stats are gathered.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 14 • 7 years ago

Looks like it is possible:  

GECKO(2860) | Assertion failure: !self->mEngineTransmitting || !self->mEngineReceiving (Video conduit is not both receiving and transmitting), at /home/worker/workspace/build/src/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:218

From https://treeherder.mozilla.org/logviewer.html#?job_id=87361259&repo=try&lineNumber=4397

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 15 • 7 years ago
mozreview-review-reply

Comment on attachment 8849845 [details]
Bug 1348657 - implement framesEncoded, pliCount, nackCount and firCount for webrtc stats

https://reviewboard.mozilla.org/r/122574/#review125976

> It is a return in a closure, which is created in a constructor, not a return from the constructor. Assertion added.

Ah, I missed that. My bad.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8853062 [details]
Bug 1348657 - Part 2 - split send/recv packet counts

https://reviewboard.mozilla.org/r/125150/#review127774

Lgtm with fixes.

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:174
(Diff revision 1)
> +  return !mPtrVoERTP_RTCP->GetRTCPPacketTypeCounters(mChannel, *aPacketCounts)
> +}

error: expected ';' before '}' token

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:180
(Diff revision 1)
> +  if (!mEngineReceiving) {
> +    return true;

return false;

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:765
(Diff revision 1)
> +  if (!mEngineTransmitting || !mSendStream) // Not transmitting
> +  {

Nit: { at eol (before comment)

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:778
(Diff revision 1)
> -  if ((!mEngineTransmitting || !mSendStream) // Not transmitting
> +  if (!mEngineReceiving || !mRecvStream) // Not receiving
> -      && (!mEngineReceiving || !mRecvStream)) // And not receiving
>    {

ditto

	Pulsebot
	Comment 20 • 7 years ago

Pushed by na-g@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/ecbc128c5289
implement framesEncoded, pliCount, nackCount and firCount for webrtc stats r=jib,smaug
https://hg.mozilla.org/integration/autoland/rev/133f596e47af
Part 2 - split send/recv packet counts r=jib

	Carsten Book [:Tomcat]
	Comment 21 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ecbc128c5289
https://hg.mozilla.org/mozilla-central/rev/133f596e47af

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 23 • 7 years ago

Fiddle calculating fps off of framesEncoded: https://jsfiddle.net/jib1/6j072zcx/

	Eric Shepherd [:sheppy]
	Comment 25 • 7 years ago

Adding dev-doc-needed so this gets documented at some point.

	Eric Shepherd [:sheppy]
	Comment 26 • 7 years ago

These are as documented as they're going to get for now; they're mentioned on Firefox 55 for developers, and they're all listed on https://developer.mozilla.org/en-US/docs/Web/API/RTCStatsReport.

That page will likely be getting split up at some point; I don't like covering multiple dictionaries on one page like this because it makes linking and re-use of content more difficult. I gave it a try here and don't like it. :)

However, this should be enough for now. Any remaining details will come with the completion of RTCStatsReport documentation. That's bug 1258413.

	Joe Reporter
	Comment 27 • 5 years ago

I am trying to get an accurate count of framesEncoded but have discovered that this value is only updated every second. In Chrome this value is updated for every frame that is encoded.

Is this expected behaviour or is it a bug?

Thanks,

Joe

	Joe Reporter
	Comment 28 • 5 years ago

This does feel like a bug as bytesSent and packetsSent update continuously whereas framesEncoded is only once per second.

Would it be possible to change the behaviour so that framesEncoded is accurate to more than a second?

	Nico Grunbaum [:ng, @chew:mozilla.org] Assignee
	Comment 29 • 5 years ago

Joe, this is expected but not ideal. I have filed Bug 1513996 to look into replacing the underlying polling mechanism.

Just a general rule of thumb, GetStats() is not meant to be a high frequency polling interface. In general if one finds one's self calling it faster than every second, one should make sure there aren't other interfaces available to achieve the same goal. (This is not always possible.)

	Joe Reporter
	Comment 30 • 5 years ago

Thanks for your reply Nico. I need to make a change every time a frame is encoded (with a high degree of accuracy) so polling framesEncoded is the only way I have found to do it so far. It would be much better if I could get a callback or something similar after each encoded frame but I am unaware of any other interface that would allow me to do this.

	Randell Jesup [:jesup] (needinfo me)
	Comment 31 • 5 years ago

Your usecase/want was not something covered by the Stats interface; that level of integration with the codec was not envisioned (and would have considerable problems in implementation, not the least of which is that JS execution is non-deterministic, and not realtime). To have any chance of working semi-smoothly on a per-frame basis it would have to work in a Worker, which adds a whole additional layer of needed complexity.

Even if you know when frames are encoded, that's not when they're sent necessarily, for all sorts of reasons, including congestion control.  In theory one could add a report (perhaps a batch report) back when frames passed from a MediaStreamTrack get encoded (though an event-per-frame should not assume to actually run at the time the event was generated).  But you'd have to make a convincing argument to add such an API to the next generation of WebRTC specs in the W3 working group.