Add new consecutiveDroppedFrames callback #1917
Conversation
| * Called to report the number of consecutive frames dropped by the video renderer. Consecutive | ||
| * dropped frames are reported when a frame is renderered after a the previous consecutive frames | ||
| * were not rendered optionally, whenever the consecutive dropped frame count is above a specified | ||
| * threshold whilst the renderer is started. |
There was a problem hiding this comment.
The javadoc for this method is a bit confusing. Perhaps there were some copy-paste logic errors from the onDroppedFrames javadoc?
With your proposed changes, MediaCodecVideoRenderer will call onConsecutiveDroppedFrames when the renderer successfully renderers a frame after dropping previous frames. Perhaps some developers may subclass MediaCodecVideoRenderer and DefaultRenderersFactory in order to add a threshold for reporting this value as well.
| @@ -1945,6 +1947,17 @@ private void maybeNotifyDroppedFrames() { | |||
| } | |||
| } | |||
|
|
|||
| private void maybeNotifyConsecutiveDroppedFrames() { | |||
| // TODO: Figure out the notification threshold for consecutive dropped frames. | |||
| if (consecutiveDroppedFrameCount > 0) { | |||
There was a problem hiding this comment.
Can you add a threshold value, maxConsecutiveDroppedFramesToNotify? This would include adding an addition constructor for MediaCodecVideoRenderer and a default value in DefaultRenderersFactory.
Our team will discuss the best initial value but for now you can set it the same as MAX_DROPPED_VIDEO_FRAME_COUNT_TO_NOTIFY which is 50.
Tying the callback to a threshold will require applications that care about this metric to set a reasonable threshold for themselves. If an application does not care about this metric then the event will not be dispatched as often.
| handler.post( | ||
| () -> | ||
| castNonNull(listener) | ||
| .onConsecutiveDroppedFrames(consecutiveDroppedFrameCount, elapsedMs)); |
There was a problem hiding this comment.
Can you add a test to MediaCodecVideoRendererTest to make sure that provided a very late buffer that is dropped that this event is invoked? You can look at MediaCodecVideoRendererTest::render_withLateBuffer_dropsBuffer. May also be helpful to have unit tests for the event testing the output for if dropped frames occur prior to any successful renders and between successful renders.
| @@ -1244,6 +1248,18 @@ default void onVideoInputFormatChanged( | |||
| @UnstableApi | |||
| default void onDroppedVideoFrames(EventTime eventTime, int droppedFrames, long elapsedMs) {} | |||
|
|
|||
| /** | |||
| * Called after consecutive video frames have been dropped. | |||
There was a problem hiding this comment.
Note that this event is called after an 'end' to the run of consecutive dropped frames.
| private void maybeNotifyConsecutiveDroppedFrames() { | ||
| // TODO: Figure out the notification threshold for consecutive dropped frames. | ||
| if (consecutiveDroppedFrameCount > 0) { | ||
| long elapsed = getClock().elapsedRealtime() - consecutiveDroppedFrameAccumulationStartTimeMs ; |
There was a problem hiding this comment.
nit: Include unit in variable name, elapsedMs.
This is similar to the `droppedFrames` callback, but represents the number of consecutive frames that we dropped before rendering a frame or seeking or stopping the renderer. While we already have a `maxConsecutiveDroppedFrame` available in the `DecoderCounters`, this doesn't provide enough visibility into the actual statistics of dropped frames. If we get 200 dropped frames and a `maxConsecutive` of 20, we don't know if we dropped 20 frames in a row once and then dropped a single frame 180 times or if we dropped 20 frames 10 times. We could add some code on our `OnDroppedFrames` callback to estimate if two calls are for consecutive frames, but that seems very fragile. Specifying when to invoke the callback is controlled by `minConsecutiveDroppedFramesToNotify` similar to the `maxDroppedFramesToNotify` but that would only notify if more than X consecutive frames were dropped. Adding support for both `MediaCodecVideoRenderer` and `DecoderVideoRenderer`.
khouzam
force-pushed
the
consecutiveDroppedFrames
branch
from
b011836
to
4ff5889
Compare
This is similar to the
droppedFramescallback, but represents the number of consecutive frames that we dropped before rendering a frame or seeking or stopping the renderer.While we already have a
maxConsecutiveDroppedFrameavailable in theDecoderCounters, this doesn't provide enough visibility into the actual statistics of dropped frames.If we get 200 dropped frames and a
maxConsecutiveof 20, we don't know if we dropped 20 frames in a row once and then dropped a single frame 180 times or if we dropped 20 frames 10 times.We could add some code on our
OnDroppedFramescallback to estimate if two calls are for consecutive frames, but that seems very fragile.There is an open issue currently on how often to notify, and my preference would be to add a new
minConsecutiveDroppedFramesToNotifysimilar to themaxDroppedFramesToNotifybut that would only notify if more than X consecutive frames were dropped.