RTCStats RTCMediaStreamTrackStats.droppedFrames is not sane (oscillates).

NEW
Assigned to

Status

()

Core
WebRTC
P2
normal
Rank:
19
a year ago
7 months ago

People

(Reporter: ng, Assigned: ng)

Tracking

({stale-bug})

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
RTCMediaStreamTrackStats.droppedFrames should monotonically increase, instead it ocillates. In tests it ocillates between values 0-2.
(Assignee)

Updated

a year ago
Assignee: nobody → na-g
Rank: 19
Priority: -- → P1
(Assignee)

Comment 1

a year ago
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 hidden (mozreview-request)

Comment 3

a year ago
mozreview-review
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.
Attachment #8837090 - Flags: review?(jib) → review+
(Assignee)

Updated

a year ago
Depends on: 1337525
(Assignee)

Comment 4

a year ago
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.
(Assignee)

Updated

a year ago
See Also: → bug 1348657
(Assignee)

Updated

a year ago
Blocks: 1348657
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?
Flags: needinfo?(jib)
and I'll note that framesDropped *implies* that it's on the reception side.
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
Flags: needinfo?(jib)
(Assignee)

Comment 8

a year ago
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.
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 hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8837090 - Attachment is obsolete: true

Comment 11

a year ago
mozreview-review
Comment on attachment 8856741 [details]
Bug 1339134 - remove nonspec RTCOutboundRTPStreamStats.droppedFrames;

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

Lgtm. Don't forget DOM review.
Attachment #8856741 - Flags: review?(jib) → review+
(Assignee)

Comment 12

a year ago
Reminder get DOM review and get telemetry review.

spec: https://w3c.github.io/webrtc-stats/#outboundrtpstats-dict*
Flags: needinfo?(na-g)

Comment 13

a year ago
mozreview-review
Comment on attachment 8856741 [details]
Bug 1339134 - remove nonspec RTCOutboundRTPStreamStats.droppedFrames;

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

r+ for the .webidl
Attachment #8856741 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8856741 - Flags: review?(chutten)

Comment 14

a year ago
mozreview-review
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.
Attachment #8856741 - Flags: review+
Attachment #8856741 - Flags: review?(chutten)
(Assignee)

Comment 15

a year ago
self reminder to check try results and land
Flags: needinfo?(na-g)
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.
Keywords: stale-bug
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
You need to log in before you can comment on or make changes to this bug.