Add logging to WebRTC video pipeline to check for frame rate stability

[youenn fablet]

Add logging to RealtimeIncomingVideoSource to check for frame rate stability

[youenn fablet]

Created attachment 402272 [details]
Patch

[youenn fablet]

Created attachment 402273 [details]
Patch

[Eric Carlson]

Comment on attachment 402273 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402273&action=review

> Source/WebCore/platform/mediastream/RealtimeIncomingVideoSource.cpp:120
> +        callOnMainThread([identifier = LOGIDENTIFIER, this, protectedThis = makeRef(*this), frameTime,  lastFrameTime, observedFrameRate = m_observedFrameRate] {

Nit: extra space before "lastFrameTime"

[youenn fablet]

Created attachment 402449 [details]
Patch

[youenn fablet]

Created attachment 402465 [details]
Patch

[youenn fablet]

I migrated from one log point (decoder output) to two (renderer and encoded frame receiver).

[Darin Adler]

Comment on attachment 402465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402465&action=review

> Source/WebCore/platform/FrameRateMonitor.cpp:31
> +void FrameRateMonitor::newFrame()

This function is full of unexplained magic numbers. 5, 3, and 2. I have no idea how we selected those.

> Source/WebCore/platform/FrameRateMonitor.h:34
> +    using Callback = Function<void(double frameTime, double lastFrameTime)>;

Why do these use double rather than one of our time types? Seems a missed opportunity to be clear on what units these times are in.

> Source/WebCore/platform/FrameRateMonitor.h:35
> +    explicit FrameRateMonitor(Callback&&);

I suggest making the type name clear, explaining what the callback is for. Presumably the callback is about periods of time with many dropped frames. There’s probably some simple term of art for that.

> Source/WebCore/platform/FrameRateMonitor.h:37
> +    void newFrame();

It’s not ideal to have a function name be a noun “frame”, when it doesn’t return a frame object. We can probably come up with a better name for this.

> Source/WebCore/platform/FrameRateMonitor.h:40
> +    size_t frameCount() const { return m_frameCount; }

size_t is a strange choice for the integer type of a frame count. It’s only appropriate when this is somehow related to the size of some in-memory data structure. I suggest either unsigned/uint32_t or uint64_t.

[youenn fablet]

Created attachment 402544 [details]
Patch

[youenn fablet]

Thanks for review.

(In reply to Darin Adler from comment #7)
> Comment on attachment 402465 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402465&action=review
> 
> > Source/WebCore/platform/FrameRateMonitor.cpp:31
> > +void FrameRateMonitor::newFrame()
> 
> This function is full of unexplained magic numbers. 5, 3, and 2. I have no
> idea how we selected those.

I added static members to improve readability.
And removed 5 since burst of callbacks is not a big issue here.

> > Source/WebCore/platform/FrameRateMonitor.h:34
> > +    using Callback = Function<void(double frameTime, double lastFrameTime)>;
> 
> Why do these use double rather than one of our time types? Seems a missed
> opportunity to be clear on what units these times are in.

Went with a struct of MonotonicTime and LateFrameCallback as a name.

> > Source/WebCore/platform/FrameRateMonitor.h:35
> > +    explicit FrameRateMonitor(Callback&&);
> 
> I suggest making the type name clear, explaining what the callback is for.
> Presumably the callback is about periods of time with many dropped frames.
> There’s probably some simple term of art for that.
> 
> > Source/WebCore/platform/FrameRateMonitor.h:37
> > +    void newFrame();
> 
> It’s not ideal to have a function name be a noun “frame”, when it doesn’t
> return a frame object. We can probably come up with a better name for this.

Changed to update.
 
> > Source/WebCore/platform/FrameRateMonitor.h:40
> > +    size_t frameCount() const { return m_frameCount; }
> 
> size_t is a strange choice for the integer type of a frame count. It’s only
> appropriate when this is somehow related to the size of some in-memory data
> structure. I suggest either unsigned/uint32_t or uint64_t.

OK, changed to uint64_t

[Eric Carlson]

Comment on attachment 402544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402544&action=review

> Source/ThirdParty/libwebrtc/Source/webrtc/video/rtp_video_stream_receiver.cc:745
> +    static const unsigned MaxFrameDelayCount = 3;

Why is this const and the constants below are constexpr?

> Source/ThirdParty/libwebrtc/Source/webrtc/video/rtp_video_stream_receiver.cc:746
> +    static constexpr unsigned TimeStampStorageDurationInMs = 2000;

Maybe TimeStampQueueDurationInMs?

> Source/ThirdParty/libwebrtc/Source/webrtc/video/rtp_video_stream_receiver.cc:764
> +    auto interval = frameTime - observedFrameTimeStamps_.front();

Maybe "queueDuration" instead of "interval"?

> Source/WebCore/platform/FrameRateMonitor.h:44
> +    double frameRate() const { return m_observedFrameRate; }

Should this be "observedFrameRate"?

> Source/WebCore/platform/FrameRateMonitor.h:50
> +    static constexpr Seconds MinimumAverageDuration = 1_s;
> +    static constexpr Seconds TimeStampStorageDuration = 2_s;
> +    static constexpr unsigned MaxFrameDelayCount = 3;

Is there a benefit to declaring these here rather than where they are used as you do with the constants in RtpVideoStreamReceiver?

[youenn fablet]

Created attachment 402629 [details]
Patch for landing

[youenn fablet]

Created attachment 402638 [details]
Patch for landing

[EWS]

Committed r263453: <https://trac.webkit.org/changeset/263453>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402638 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64700061>