1339134 - RTCStats RTCMediaStreamTrackStats.droppedFrames is not sane (oscillates).

Status: NEW

(Core :: WebRTC, defect, P3)

Description

[Nico Grunbaum [:ng, @chew:mozilla.org]]

RTCMediaStreamTrackStats.droppedFrames should monotonically increase, instead it ocillates. In tests it ocillates between values 0-2.

Comment 1

[Nico Grunbaum [:ng, @chew:mozilla.org]]

In more tests it looks to underflow u32. The underflow may be a timing issue with mDroppedFrames being calculated between the updates to key_frames + delta_frames, and the update to mSentFrames.

Comment 2

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1339134 - stop oscillation of droppedFrames stat;

WIP - Still trying to track down where the bulk of the frame drops are occurring.

Review commit: https://reviewboard.mozilla.org/r/112340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112340/

Comment 3

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

Comment on attachment 8837090 [details]
Bug 1339134 - stop oscillation of droppedFrames stat;

https://reviewboard.mozilla.org/r/112340/#review113720

r=me with issues resolved.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:131
(Diff revision 1)
>        aStats.substreams.begin()->second.frame_counts;
>      CSFLogVerbose(logTag, "%s: framerate: %u, bitrate: %u, dropped frames delta: %u",
>                    __FUNCTION__, aStats.encode_frame_rate, aStats.media_bitrate_bps,
>                    (mSentFrames - (fc.key_frames + fc.delta_frames)) - mDroppedFrames);
>      mDroppedFrames = mSentFrames - (fc.key_frames + fc.delta_frames);
> +    printf("@@NG mDroppedFrames=%d vs. VideoSendStream.stats.dropped_frames=%d\n", static_cast<int>(mDroppedFrames), static_cast<int>(aStats.dropped_frames));

Remove debug cruft

::: media/webrtc/trunk/webrtc/video/send_statistics_proxy.h:32
(Diff revision 1)
> +class FrameDroppedObserver {
> +  virtual void OnFrameDropped() = 0;
> +};

Is there any benefit to this unused abstract class? It seems like we can mirror OnIncomingFrame instead here.

::: media/webrtc/trunk/webrtc/video/vie_encoder.cc:344
(Diff revision 1)
> +    stats_proxy_->OnFrameDropped();
>      return;
>    }
>    VideoCodecType codec_type;
>    {
>      CriticalSectionScoped cs(data_cs_.get());
>      time_of_last_frame_activity_ms_ = TickTime::MillisecondTimestamp();
>      if (EncoderPaused()) {
> +      if (stats_proxy_) {
> +        stats_proxy_->OnFrameDropped();

Inconsistent treatment of stats_proxy_ as invariant.

From inspection, stats_proxy_ looks to be invariant in practice, even in ViEEncoder where it's passed in.

Either add if() statements, or add an assert() on construction [1] and remove the existing ones. The latter is probably touching more code thant we need to though for upstream code.

[1] https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/media/webrtc/trunk/webrtc/video/vie_encoder.cc#139

::: media/webrtc/trunk/webrtc/video/vie_encoder.cc:404
(Diff revision 1)
> -    vcm_->AddVideoFrame(*frame_to_send, vp_->GetContentMetrics(),
> -                        &codec_specific_info);
> +    if (VCM_OK != vcm_->AddVideoFrame(*frame_to_send,
> +        vp_->GetContentMetrics(), &codec_specific_info)) {

Nit: comparison order is inconsistent with the rest of this file.

::: media/webrtc/trunk/webrtc/video/vie_encoder.cc:412
(Diff revision 1)
> +          stats_proxy_->OnFrameDropped();
> +      }
> +    }
>      return;
>    }
> -  vcm_->AddVideoFrame(*frame_to_send);
> +  if (VCM_OK != vcm_->AddVideoFrame(*frame_to_send)){

same here

::: media/webrtc/trunk/webrtc/video_send_stream.h:66
(Diff revision 1)
> +    uint32_t dropped_frames = 0;
>      int input_frame_rate = 0;
>      int encode_frame_rate = 0;
>      int avg_encode_time_ms = 0;
>      int encode_usage_percent = 0;
>      int target_media_bitrate_bps = 0;
>      int media_bitrate_bps = 0;

We generally favor consistency with existing code in cases like this.

Comment 4

[Nico Grunbaum [:ng, @chew:mozilla.org]]

This needs a different approach because libvpx is silently dropping frames. My idea is to keep a history of frame timestamps going into and coming out of libvpx.

Comment 5

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

The spec has no droppedFrames (anymore?)  it only has:
 framesDropped of type unsigned long

    Only valid for video. It is the total number of frames dropped predecode or dropped because the frame missed its display deadline for this MediastreamTrack. It is the same definition as droppedVideoFrames in Section 5 of [MEDIA-SOURCE].

And I'll note that there is *no* droppedVideoFrames (or anything close to it) in Section 5 of MEDIA-SOURCE, or anywhere else in MEDIA-SOURCE.

Perhaps we're way behind vs the spec here?

Comment 6

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

and I'll note that framesDropped *implies* that it's on the reception side.

Comment 7

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

Ah, nice catch. You're right our droppedFrames is an encoder stat, and framesDropped is not.

Looks like the spec instead has the inverse: framesEncoded [1]. I.e. I suppose one would do:

    let droppedFrames = track.getSettings().frameRate - stat.framesEncoded;

If you agree, should we look at reporting framesEncoded instead of fixing droppedFrames?

[1] https://w3c.github.io/webrtc-stats/#dom-rtcoutboundrtpstreamstats-framesencoded

Comment 8

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Jib, I am not confident that calculating the frames dropped by the encoder this way would be accurate. It would be subject to the same type of oscillations we are seeing by not directly measuring the dropped frames. Also, one would have to keep track of the time the stream was not encoding frames because it was muted.

Comment 9

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

framerate is not constant; you can't use it here.

Also: I don't think anyone cares (much) about droppedFrames.  EncodedFrames, sure.  We should just report framesEncoded.  If they care, they can measure the frames inserted and compare to framesEncoded (though there can be a time lag...)

Comment 10

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1339134 - remove nonspec RTCOutboundRTPStreamStats.droppedFrames;

Review commit: https://reviewboard.mozilla.org/r/128662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128662/

Comment 11

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

Comment on attachment 8856741 [details]
Bug 1339134 - remove nonspec RTCOutboundRTPStreamStats.droppedFrames;

https://reviewboard.mozilla.org/r/128662/#review131178

Lgtm. Don't forget DOM review.

Comment 12

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Reminder get DOM review and get telemetry review.

spec: https://w3c.github.io/webrtc-stats/#outboundrtpstats-dict*

Comment 13

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

Comment on attachment 8856741 [details]
Bug 1339134 - remove nonspec RTCOutboundRTPStreamStats.droppedFrames;

https://reviewboard.mozilla.org/r/128662/#review132750

r+ for the .webidl

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8856741 [details]
Bug 1339134 - remove nonspec RTCOutboundRTPStreamStats.droppedFrames;

https://reviewboard.mozilla.org/r/128662/#review133298

Telemetry-wise this is a straight forward histogram removal, which looks fine.
Just be aware of that this histogram will not show up in most of our analysis tools anymore, even for historic data.

Comment 15

[Nico Grunbaum [:ng, @chew:mozilla.org]]

self reminder to check try results and land

Comment 16

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 17

[Bulk Bug Changes for mreavy's org]

Mass change P1->P2 to align with new Mozilla triage process

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information