Add a low latency decoder option and actually apply the MediaCodec keys - #113
Conversation
writeAndroidPerformanceParams() was never called: both call sites in AndroidMediaFormatHelper.h were commented out, and the same keys sat commented out a second time in VideoDecoder::configureStartDecoder(). So every decoder was configured without "low-latency" and without "priority", i.e. MediaCodec kept its default reorder/output queue, which on a live stream with no B-frames only adds latency. The keys are now written, but behind a switch so a device whose decoder does not like them can be put back on the stock pipeline: - Settings -> Video -> Low latency, persisted as "low_latency_decoder", default on - plumbed through VideoPlayer.setLowLatency() / nativeSetLowLatency() to VideoDecoder, applied when the decoder is configured - writeAndroidPerformanceParams() also gained the vendor low-latency keys that were commented out in VideoDecoder.cpp (Qualcomm, HiSilicon, rtc-ext) and is now static like the two functions next to it Unknown AMediaFormat keys are ignored by MediaCodec, so writing all variants is safe across vendors and Android versions.
PR Summary by QodoApply MediaCodec low-latency keys behind a user setting
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record |
… by nobody With video working in flat mode, switching to immersive mode and back left no video in either. The activity dump explains it: topResumedActivity: .VideoActivity t5249 topResumedActivity: .XrVideoActivity t5253 threads: xr-goggle (no wfb-* at all) Both activities were alive in separate tasks - Horizon OS keeps the immersive one in its own task, so its onDestroy(), and with it stopAdapters(), never ran - while the flat one had also given the adapter up. Each activity constructs its own WfbNgLink, so with two of them the adapter belongs to neither. ModeOwner makes that impossible: whoever starts last evicts the other, whose onDestroy() releases the adapter. A WeakReference so a finished activity is not retained. The real answer is a single owner outliving both - a foreground service, which would also fix the flat mode dropping the link in onPause - but this removes the failure without that rework. Also corrects a wrong reading in the previous commit: three threads named "wfb-001/002" is *normal* for one adapter, because the TX and adaptive-link threads inherit the RX thread's name - nothing renames them. It is not three RX loops. The duplicate guard is still correct, since two activities each with their own WfbNgLink genuinely can double-start, but the evidence quoted for it was misread. Confirmed on hardware in the same run: the MediaCodec low-latency keys from OpenIPC#113 are accepted by Meta's H.265 decoder - "low-latency: 1, vendor.qti-ext-dec-low-latency.enable: 1, priority: 0" followed by "AMediaCodec_configure: OK".
* Restart the app when the low latency toggle changes The low-latency keys from #113 are written into the AMediaFormat when the codec is configured, and the codec is only torn down when its surface goes away. VideoPlayer stop()/start() recycle the UDP receivers and leave the codec alone, and a channel change never touches the surface. So the toast "applies on next video start" promised something that only happened after backgrounding the app or restarting it; a pilot who turned the option off in the field and changed channel kept flying on the old configuration. Do what the VR mode toggle does and restart the app. The preference is read on startup, so the runtime setLowLatency() call in the click handler goes away. Two leftovers from #113 in the same area: setLowLatency() now runs verifyApplicationThread() like every other VideoPlayer setter, and the commented-out AMEDIAFORMAT_KEY_PRIORITY line in h264_configureAMediaFormat is dropped, since writeAndroidPerformanceParams() sets the real key. * Commit the low latency pref and finalize the DVR before restarting Two review findings on the restart path. The preference was written with apply(), and resetApp() ends the process with System.exit() a few lines later. apply() flushes to disk on a background thread, so the new process could read the old value and come up with the configuration the pilot just switched away from. The VR mode toggle already uses commit() for the same reason; do the same here. resetApp() also killed an active DVR recording without closing the MP4: System.exit() skips every lifecycle callback, and the file is only finalized when the DVR thread exits. stopDvr() joins that thread, so call it first. This covers the VR mode toggle too, which had the same hole.
Note
Compile tested only (arm64-v8a + armeabi-v7a). Not yet flown. A glass-to-glass
comparison with the switch on/off on real hardware would be very welcome,
especially on non-Qualcomm SoCs.
The problem
writeAndroidPerformanceParams()inapp/videonative/src/main/cpp/helper/AndroidMediaFormatHelper.hsets the two decoder keys that matter for a live stream:
It is never called. Both call sites are commented out:
h264_configureAMediaFormat()—// writeAndroidPerformanceParams(format);h265_configureAMediaFormat()—// writeAndroidPerformanceParams(format);and the same keys sit commented out a second time in
VideoDecoder::configureStartDecoder(), including the vendor variants:So every decoder is configured with the stock MediaCodec pipeline. Without
KEY_LOW_LATENCYthe codec may hold output frames back for reordering, whichfor a majestic stream (no B-frames, low slice count) buys nothing and only
costs frames of latency. Without
priority = 0the codec runs as best effortinstead of realtime, so it competes with everything else on the device.
The change
The keys are written now, but behind a switch. "Change how the decoder
behaves" is exactly the kind of thing that should be escapable on a device
whose vendor codec does not like it, so:
Settings → Video → Low latency, persisted as
low_latency_decoder,default on.
Plumbing:
VideoPlayer.setLowLatency(boolean)→nativeSetLowLatency()→VideoDecoder::setLowLatency()VideoDecoder::configureStartDecoder()callswriteAndroidPerformanceParams()only when the flag is setVideoActivitypushes the persisted value down as soon as the player exists,so the setting survives a restart
Because the decoder is created lazily once SPS/PPS arrive, toggling the switch
takes effect the next time the decoder is configured (next video start /
channel change), not on a decoder that is already running. The menu says so.
writeAndroidPerformanceParams()also picked up the vendor keys that werecommented out in
VideoDecoder.cpp(Qualcomm, HiSilicon, rtc-ext) and is nowstatic, like the two functions next to it. UnknownAMediaFormatkeys areignored by MediaCodec, so writing all variants is safe across vendors and
Android versions.
Not in this PR
VideoDecoder::feedDecoder()blocks the receive thread for up toBUFFER_TIMEOUT_US(17 ms) per NALU and retries for up to a second, andBufferedPacketQueuehas no time bound on how long it holds a reorder buffer.Both also cost latency but need their own discussion.
Part of a series of independent fixes found while building an immersive (OpenXR) mode on a
Quest 3, each standalone and mergeable in any order:
wirelessInfo()safeVideoPlayer/WfbNgLinktake aContext#113 and #116 are now confirmed on hardware (Quest 3, Horizon OS, Android 14).