[android] Refine video decoder for tunnel mode #1835

jasonzhangxx · 2023-10-23T23:34:08Z

Send placeholder frames to video renderer in tunnel mode.
Calculate dropped frames based on the placeholder frames.
Determine preroll status based on the placeholder frames.
Add OnFirstTunnelFrameReadyListener.
Remove video frame tracker as it's no longer needed.

b/175890319
b/175888740
b/302198176
b/253469969

codecov · 2023-10-24T00:52:28Z

Codecov Report

Merging #1835 (81ecfbd) into main (896a0ec) will increase coverage by 0.22%.
Report is 149 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1835      +/-   ##
==========================================
+ Coverage   57.57%   57.80%   +0.22%     
==========================================
  Files        1906     1915       +9     
  Lines       94789    95191     +402     
==========================================
+ Hits        54573    55023     +450     
+ Misses      40216    40168      -48

see 51 files with indirect coverage changes

starboard/android/apk/app/src/main/java/dev/cobalt/media/MediaCodecBridge.java

starboard/android/shared/video_decoder.cc

xiaomings · 2023-10-26T23:29:18Z

starboard/android/shared/video_render_algorithm.cc

@@ -26,7 +26,7 @@ namespace shared {

 namespace {

-const SbTimeMonotonic kBufferTooLateThreshold = -32 * kSbTimeMillisecond;
+const SbTimeMonotonic kBufferTooLateThreshold = -16 * kSbTimeMillisecond;


This is also used for non-tunneled playback, and is probably carefully tuned. What's the implications for non-tunneled playback if it's reduced?

I think 32ms is a too large tolerance for dropped frames. But as it's carefully tuned, I changed it back to 32ms.

It's ok to modify it with a fallback plan (i.e. if this causes issues in production, we'll revert the behavior from the backend).

xiaomings · 2023-10-26T23:37:11Z

starboard/android/shared/video_render_algorithm.cc

@@ -107,8 +107,10 @@ void VideoRenderAlgorithm::Render(
      frames->pop_front();
      ++dropped_frames_;
    } else if (early_us < kBufferReadyThreshold) {
-      auto status = draw_frame_cb(frames->front(), adjusted_release_time_ns);
-      SB_DCHECK(status == VideoRendererSink::kReleased);
+      if (!video_decoder_->IsTunnelMode()) {


It seems to me that what the algorithm for non-tunneled mode does is:

If the frame arrives too late (16 or 32 ms later than the time it should be displayed), we drop it.

Else if the frame should be rendered in the next 50 ms, we display it.

Else do nothing.

What do we want for tunnel mode frames? Will we get the same accuracy after tick interval is reduced to 10ms?

Besides prerolling and dropped frame calculation, calling sink_->Render() is to pop out cached frames, so renderer would ask for new video frames. It's ok to reduce the tick interval to 10ms and I have reduced it in the latest patch.

1. Send placeholder frames to video renderer in tunnel mode. 2. Calculate dropped frames based on the placeholder frames. 3. Determing preroll status based on the placeholder frames. 4. Add OnFirstTunnelFrameReadyListener. 5. Remove video frame tracker as it's no longer needed. b/175890319 b/175888740 b/302198176 b/253469969

xiaomings · 2023-11-07T02:20:08Z

starboard/android/apk/app/src/main/java/dev/cobalt/media/MediaCodecBridge.java

-            }
-          };
-      mMediaCodec.setOnFrameRenderedListener(mTunnelModeFrameRendererListener, null);
+      // TODO (b/306236129): disable the listeners as frames to be discarded would invoke the


We are not implementing BUFFER_FLAG_DECODE_ONLY now, do we still need to remove the two listeners here?
Also nit:

Remove the space after TODO

Refine the commit message mentioning that OnFirstTunnelFrameReadyListener is commented out.

xiaomings · 2023-11-07T02:20:59Z

starboard/android/apk/app/src/main/java/dev/cobalt/media/MediaCodecBridge.java

+      mMediaCodec.setOnFrameRenderedListener(null, null);
+      mMediaCodec.setOnFirstTunnelFrameReadyListener(null, null);


Let's also comment them out, since the setting of them are commented out. Also, if we are going to set the callback to null, we should set variables like mTunnelModeFrameRendererListener to null too.

xiaomings · 2023-11-07T02:22:24Z

starboard/android/apk/app/src/main/java/dev/cobalt/media/MediaCodecBridge.java

@@ -1304,6 +1303,47 @@ private int getAudioFormat(int channelCount) {
    }
  }

+  private void addOnFrameRenderedListener() {


nit: "set" instead of "add".

xiaomings · 2023-11-07T02:24:29Z

starboard/android/apk/app/src/main/java/dev/cobalt/media/MediaCodecBridge.java

+  }
+
+  @RequiresApi(31)
+  private void addOnFirstTunnelFrameReadyListener() {


nit: "set" instead of "add", also append "V31" to the function name.

xiaomings · 2023-11-07T02:27:57Z

starboard/android/shared/audio_decoder.h

+  void OnMediaCodecFrameRendered(SbTime frame_timestamp) override {}
+  void OnFirstTunnelFrameReady() override {}


Add NOTREACHED() here.

xiaomings · 2023-11-07T03:43:11Z

starboard/android/shared/video_decoder.cc

-      }
+      // Send placeholders of frames to renderer, so it can end prerolling and
+      // calculate dropped frames.
+      decoder_status_cb_(kNeedMoreInput,


The timestamp of the frames might not be monotonic, and may get dropped in https://github.com/youtube/cobalt/blob/24.lts.13/starboard/shared/starboard/player/filter/video_renderer_internal_impl.cc#L280C37-L280C37.

We are sending the pseudo decoded frames back, before they are actually written to the platform. Note that MediaDecoder maintains a queue and there can be a delay between this call and the queuing of buffer to the platform. Shall we send the decoded frames after queueInputBuffer()?

xiaomings · 2023-11-07T03:44:22Z

starboard/android/shared/video_decoder.cc


-  if (tunnel_mode_audio_session_id_ != -1) {
-    SbTime max_timestamp = input_buffers[0]->timestamp();
+  if (IsTunnelMode()) {


The previous code does rate control at line 797, just want to double check if rate control is handled in tunnel mode.

xiaomings · 2023-11-07T03:48:51Z

starboard/android/shared/video_decoder.cc

-}
-
-void VideoDecoder::OnTunnelModePrerollTimeout() {
+void VideoDecoder::TunnelModeTick() {


Under the current algorithm, the moment we queue the input buffer to the platform, we know whether the frame will be considered as dropped. We cannot determine this in the current implementation because in the call chain of WriteInputBuffers() doesn't have access to the current media time.
It would be great if we can find an elegant way to make this decision, instead of ticking 100 times a second.

xiaomings · 2023-11-07T03:59:10Z

starboard/android/shared/video_render_algorithm.cc

@@ -26,7 +26,7 @@ namespace shared {

 namespace {

-const SbTimeMonotonic kBufferTooLateThreshold = -32 * kSbTimeMillisecond;
+const SbTimeMonotonic kBufferTooLateThreshold = -16 * kSbTimeMillisecond;


It's ok to modify it with a fallback plan (i.e. if this causes issues in production, we'll revert the behavior from the backend).

jasonzhangxx requested review from xiaomings, osagie98, borongc and sideb0ard October 23, 2023 23:34

jasonzhangxx force-pushed the tunnel-dropped-frames branch from 017e57b to 6630398 Compare October 26, 2023 20:24

xiaomings reviewed Oct 26, 2023

View reviewed changes

jasonzhangxx force-pushed the tunnel-dropped-frames branch 2 times, most recently from 5c92f07 to 95ccd7a Compare October 31, 2023 17:24

jasonzhangxx force-pushed the tunnel-dropped-frames branch from 95ccd7a to 81ecfbd Compare October 31, 2023 17:55

xiaomings reviewed Nov 7, 2023

View reviewed changes

jasonzhangxx requested a review from xiaomings November 8, 2023 18:19

jasonzhangxx marked this pull request as draft November 16, 2023 23:40

jasonzhangxx closed this Feb 1, 2024

jasonzhangxx deleted the tunnel-dropped-frames branch February 1, 2024 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[android] Refine video decoder for tunnel mode #1835

[android] Refine video decoder for tunnel mode #1835

jasonzhangxx commented Oct 23, 2023 •

edited by xiaomings

Loading

codecov bot commented Oct 24, 2023 •

edited

Loading

xiaomings Oct 26, 2023

jasonzhangxx Oct 30, 2023

xiaomings Nov 7, 2023

xiaomings Oct 26, 2023

jasonzhangxx Oct 31, 2023

xiaomings Nov 7, 2023

xiaomings Nov 7, 2023

xiaomings Nov 7, 2023

xiaomings Nov 7, 2023

xiaomings Nov 7, 2023

xiaomings Nov 7, 2023

xiaomings Nov 7, 2023

xiaomings Nov 7, 2023

xiaomings Nov 7, 2023

		mMediaCodec.setOnFrameRenderedListener(null, null);
		mMediaCodec.setOnFirstTunnelFrameReadyListener(null, null);

		void OnMediaCodecFrameRendered(SbTime frame_timestamp) override {}
		void OnFirstTunnelFrameReady() override {}

[android] Refine video decoder for tunnel mode #1835

[android] Refine video decoder for tunnel mode #1835

Conversation

jasonzhangxx commented Oct 23, 2023 • edited by xiaomings Loading

codecov bot commented Oct 24, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonzhangxx commented Oct 23, 2023 •

edited by xiaomings

Loading

codecov bot commented Oct 24, 2023 •

edited

Loading