Add logging to RealtimeIncomingVideoSource to check for frame rate stability
Created attachment 402272 [details] Patch
Created attachment 402273 [details] Patch
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"
Created attachment 402449 [details] Patch
Created attachment 402465 [details] Patch
I migrated from one log point (decoder output) to two (renderer and encoded frame receiver).
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.
Created attachment 402544 [details] Patch
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
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?
Created attachment 402629 [details] Patch for landing
Created attachment 402638 [details] Patch for landing
Committed r263453: <https://trac.webkit.org/changeset/263453> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402638 [details].
<rdar://problem/64700061>