From fa9e1c726e0f404481c66aaa5e5351e624f82618 Mon Sep 17 00:00:00 2001 From: Alex Andres Date: Sun, 1 Nov 2026 18:50:53 +0100 Subject: [PATCH] fix: make decoder QP parsing flag atomic and give up codec calls without a JNI frame qpParsingEnabled is written from the Java decoder's output thread while Decode reads it, so it is now atomic. A failed PushLocalFrame no longer clears the OutOfMemoryError silently: it is reported like other codec exceptions and the call gives up, except release, which still runs and frees its one reference itself. Encode drops the frame instead of failing, since WebRTC switches away from an encoder that fails. --- .../media/video/codec/JavaLocalFrame.h | 13 ++- .../media/video/codec/VideoDecoderWrapper.h | 6 +- .../src/media/video/codec/JavaLocalFrame.cpp | 23 ++++-- .../codec/VideoDecoderFactoryWrapper.cpp | 6 +- .../media/video/codec/VideoDecoderWrapper.cpp | 35 +++++++- .../codec/VideoEncoderFactoryWrapper.cpp | 6 +- .../media/video/codec/VideoEncoderWrapper.cpp | 44 +++++++++-- .../onvoid/webrtc/VideoCodecFactoryTests.java | 79 ++++++++++++++++--- 8 files changed, 182 insertions(+), 30 deletions(-) diff --git a/webrtc-jni/src/main/cpp/include/media/video/codec/JavaLocalFrame.h b/webrtc-jni/src/main/cpp/include/media/video/codec/JavaLocalFrame.h index 23c46af6b..eb0da4855 100644 --- a/webrtc-jni/src/main/cpp/include/media/video/codec/JavaLocalFrame.h +++ b/webrtc-jni/src/main/cpp/include/media/video/codec/JavaLocalFrame.h @@ -25,18 +25,27 @@ namespace jni // thread stays attached to the JVM for as long as it runs, so without a // frame of their own the references made for one call would pile up // behind those of the next. + // + // A frame that cannot be pushed reports the JVM's exception as a codec + // exception, and the call it was meant for gives up rather than make + // references that nothing would free soon: see pushed(). class JavaLocalFrame { public: - JavaLocalFrame(JNIEnv * env, jint capacity); + // The context names the call in the log. + JavaLocalFrame(JNIEnv * env, jint capacity, const char * context); ~JavaLocalFrame(); JavaLocalFrame(const JavaLocalFrame &) = delete; JavaLocalFrame & operator=(const JavaLocalFrame &) = delete; + // Whether the frame is in place, which a call checks before it + // makes any local reference. + bool pushed() const; + private: JNIEnv * env; - bool pushed; + const bool isPushed; }; } diff --git a/webrtc-jni/src/main/cpp/include/media/video/codec/VideoDecoderWrapper.h b/webrtc-jni/src/main/cpp/include/media/video/codec/VideoDecoderWrapper.h index 8192decb6..3a4ad414f 100644 --- a/webrtc-jni/src/main/cpp/include/media/video/codec/VideoDecoderWrapper.h +++ b/webrtc-jni/src/main/cpp/include/media/video/codec/VideoDecoderWrapper.h @@ -117,8 +117,10 @@ namespace jni Settings settings; bool initialized; - // Parsing stops while the decoder provides the QP itself. - bool qpParsingEnabled; + // Parsing stops while the decoder provides the QP itself. Read on + // the decoder thread and written on the Java decoder's, which + // may deliver its frames from a thread of its own. + std::atomic qpParsingEnabled; webrtc::H264BitstreamParser h264BitstreamParser; }; } diff --git a/webrtc-jni/src/main/cpp/src/media/video/codec/JavaLocalFrame.cpp b/webrtc-jni/src/main/cpp/src/media/video/codec/JavaLocalFrame.cpp index f735831cd..d84eea04d 100644 --- a/webrtc-jni/src/main/cpp/src/media/video/codec/JavaLocalFrame.cpp +++ b/webrtc-jni/src/main/cpp/src/media/video/codec/JavaLocalFrame.cpp @@ -15,23 +15,34 @@ */ #include "media/video/codec/JavaLocalFrame.h" +#include "media/video/codec/VideoCodecUtils.h" + +#include "rtc_base/logging.h" namespace jni { - JavaLocalFrame::JavaLocalFrame(JNIEnv * env, jint capacity) : + JavaLocalFrame::JavaLocalFrame(JNIEnv * env, jint capacity, const char * context) : env(env), - pushed(env->PushLocalFrame(capacity) == JNI_OK) + isPushed(env->PushLocalFrame(capacity) == JNI_OK) { - if (!pushed) { - // Out of memory: the references then go to the enclosing frame. - env->ExceptionClear(); + if (!isPushed) { + // The JVM is out of memory and has thrown, which is reported like + // any exception of a codec call. + ClearCodecException(env, context); + + RTC_LOG(LS_WARNING) << "No local JNI frame for " << context << ", giving up the call"; } } JavaLocalFrame::~JavaLocalFrame() { - if (pushed) { + if (isPushed) { env->PopLocalFrame(nullptr); } } + + bool JavaLocalFrame::pushed() const + { + return isPushed; + } } diff --git a/webrtc-jni/src/main/cpp/src/media/video/codec/VideoDecoderFactoryWrapper.cpp b/webrtc-jni/src/main/cpp/src/media/video/codec/VideoDecoderFactoryWrapper.cpp index ce5bdd671..2ac11453f 100644 --- a/webrtc-jni/src/main/cpp/src/media/video/codec/VideoDecoderFactoryWrapper.cpp +++ b/webrtc-jni/src/main/cpp/src/media/video/codec/VideoDecoderFactoryWrapper.cpp @@ -53,7 +53,11 @@ namespace jni return nullptr; } - JavaLocalFrame localFrame(env, 16); + JavaLocalFrame localFrame(env, 16, "createDecoder"); + + if (!localFrame.pushed()) { + return nullptr; + } try { JavaLocalRef info = VideoCodecInfo::toJava(env, format); diff --git a/webrtc-jni/src/main/cpp/src/media/video/codec/VideoDecoderWrapper.cpp b/webrtc-jni/src/main/cpp/src/media/video/codec/VideoDecoderWrapper.cpp index 08b352339..97b5a2833 100644 --- a/webrtc-jni/src/main/cpp/src/media/video/codec/VideoDecoderWrapper.cpp +++ b/webrtc-jni/src/main/cpp/src/media/video/codec/VideoDecoderWrapper.cpp @@ -57,7 +57,12 @@ namespace jni initialized(false), qpParsingEnabled(true) { - JavaLocalFrame localFrame(env, kLocalFrameCapacity); + JavaLocalFrame localFrame(env, kLocalFrameCapacity, "getImplementationName"); + + if (!localFrame.pushed()) { + // A decoder without a name, taken for a software one. + return; + } jobject name = env->CallObjectMethod(decoder, javaClass->getImplementationName); @@ -102,7 +107,16 @@ namespace jni bool VideoDecoderWrapper::ConfigureInternal(JNIEnv * env) { - JavaLocalFrame localFrame(env, kLocalFrameCapacity); + JavaLocalFrame localFrame(env, kLocalFrameCapacity, "initDecode"); + + if (!localFrame.pushed()) { + // Detaching the earlier callback makes no local reference. + InvalidateCallback(env); + + initialized = false; + + return false; + } webrtc::RenderResolution resolution = settings.max_render_resolution(); @@ -154,7 +168,13 @@ namespace jni return WEBRTC_VIDEO_CODEC_ERROR; } - JavaLocalFrame localFrame(env, kLocalFrameCapacity); + JavaLocalFrame localFrame(env, kLocalFrameCapacity, "decode"); + + if (!localFrame.pushed()) { + // Given up before a record of the image is made, so the decoder + // is not waited for. + return WEBRTC_VIDEO_CODEC_ERROR; + } // The capture time identifies the frame, and received images carry // none, so it is derived from the RTP timestamp. @@ -215,11 +235,18 @@ namespace jni int32_t VideoDecoderWrapper::ReleaseInternal(JNIEnv * env) { - JavaLocalFrame localFrame(env, kLocalFrameCapacity); + // Releasing goes ahead without a frame, since giving it up would keep + // the Java decoder's resources, and the status is the only reference + // it makes. + JavaLocalFrame localFrame(env, kLocalFrameCapacity, "release"); jobject status = env->CallObjectMethod(decoder, javaClass->release); int32_t result = ToNativeCodecStatus(env, status, "release"); + if (!localFrame.pushed() && status != nullptr) { + env->DeleteLocalRef(status); + } + RTC_LOG(LS_INFO) << "Java decoder release: " << result; InvalidateCallback(env); diff --git a/webrtc-jni/src/main/cpp/src/media/video/codec/VideoEncoderFactoryWrapper.cpp b/webrtc-jni/src/main/cpp/src/media/video/codec/VideoEncoderFactoryWrapper.cpp index 9bcdd31d3..e3257cb9f 100644 --- a/webrtc-jni/src/main/cpp/src/media/video/codec/VideoEncoderFactoryWrapper.cpp +++ b/webrtc-jni/src/main/cpp/src/media/video/codec/VideoEncoderFactoryWrapper.cpp @@ -53,7 +53,11 @@ namespace jni return nullptr; } - JavaLocalFrame localFrame(env, 16); + JavaLocalFrame localFrame(env, 16, "createEncoder"); + + if (!localFrame.pushed()) { + return nullptr; + } try { JavaLocalRef info = VideoCodecInfo::toJava(env, format); diff --git a/webrtc-jni/src/main/cpp/src/media/video/codec/VideoEncoderWrapper.cpp b/webrtc-jni/src/main/cpp/src/media/video/codec/VideoEncoderWrapper.cpp index b4f8daa90..fc989dcd2 100644 --- a/webrtc-jni/src/main/cpp/src/media/video/codec/VideoEncoderWrapper.cpp +++ b/webrtc-jni/src/main/cpp/src/media/video/codec/VideoEncoderWrapper.cpp @@ -106,7 +106,16 @@ namespace jni int32_t VideoEncoderWrapper::InitEncodeInternal(JNIEnv * env) { - JavaLocalFrame localFrame(env, kLocalFrameCapacity); + JavaLocalFrame localFrame(env, kLocalFrameCapacity, "initEncode"); + + if (!localFrame.pushed()) { + // Detaching the earlier callback makes no local reference. + InvalidateCallback(env); + + initialized = false; + + return WEBRTC_VIDEO_CODEC_ERROR; + } bool automaticResizeOn; @@ -192,11 +201,18 @@ namespace jni int32_t VideoEncoderWrapper::ReleaseInternal(JNIEnv * env) { - JavaLocalFrame localFrame(env, kLocalFrameCapacity); + // Releasing goes ahead without a frame, since giving it up would keep + // the Java encoder's resources, and the status is the only reference + // it makes. + JavaLocalFrame localFrame(env, kLocalFrameCapacity, "release"); jobject status = env->CallObjectMethod(encoder, javaClass->release); int32_t result = ToNativeCodecStatus(env, status, "release"); + if (!localFrame.pushed() && status != nullptr) { + env->DeleteLocalRef(status); + } + RTC_LOG(LS_INFO) << "Java encoder release: " << result; InvalidateCallback(env); @@ -224,7 +240,15 @@ namespace jni return WEBRTC_VIDEO_CODEC_ERROR; } - JavaLocalFrame localFrame(env, kLocalFrameCapacity); + JavaLocalFrame localFrame(env, kLocalFrameCapacity, "encode"); + + if (!localFrame.pushed()) { + // Dropped rather than failed, since WebRTC gives up an encoder that + // fails, and running short of memory is no fault of the encoder. + ReportDroppedFrame(frame.rtp_timestamp()); + + return WEBRTC_VIDEO_CODEC_OK; + } const auto javaFrameClass = JavaClasses::get(env); jobject jFrame = nullptr; @@ -309,7 +333,12 @@ namespace jni return; } - JavaLocalFrame localFrame(env, kLocalFrameCapacity); + JavaLocalFrame localFrame(env, kLocalFrameCapacity, "setRates"); + + if (!localFrame.pushed()) { + // The encoder keeps its rates until the next call. + return; + } jobjectArray layers = env->NewObjectArray(webrtc::kMaxSpatialLayers, javaClass->intArrayClass, nullptr); @@ -465,7 +494,12 @@ namespace jni void VideoEncoderWrapper::UpdateEncoderInfo(JNIEnv * env) { - JavaLocalFrame localFrame(env, kLocalFrameCapacity); + JavaLocalFrame localFrame(env, kLocalFrameCapacity, "getEncoderInfo"); + + if (!localFrame.pushed()) { + // The info stays as it was. + return; + } encoderInfo.supports_native_handle = false; encoderInfo.scaling_settings = GetScalingSettings(env); diff --git a/webrtc/src/test/java/dev/onvoid/webrtc/VideoCodecFactoryTests.java b/webrtc/src/test/java/dev/onvoid/webrtc/VideoCodecFactoryTests.java index ba487a3ef..e42694adf 100644 --- a/webrtc/src/test/java/dev/onvoid/webrtc/VideoCodecFactoryTests.java +++ b/webrtc/src/test/java/dev/onvoid/webrtc/VideoCodecFactoryTests.java @@ -56,6 +56,9 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.RejectedExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; @@ -198,14 +201,26 @@ void customCodecIsAdvertised() { @Test void javaCodecRoundTrips() throws Exception { - assertRoundTrip(new VideoCodecInfo(TEST_CODEC), false); + assertRoundTrip(new VideoCodecInfo(TEST_CODEC), false, false); } @Test void javaFrameBufferRoundTrips() throws Exception { // The decoder hands back frames whose buffer is no NativeI420Buffer, // which WebRTC has to copy. - assertRoundTrip(new VideoCodecInfo(TEST_CODEC), true); + assertRoundTrip(new VideoCodecInfo(TEST_CODEC), true, false); + } + + @Test + void asyncJavaDecoderRoundTrips() throws Exception { + // The decoder hands its frames over on a thread of its own, now with + // a QP and now without, which switches the wrapper's QP parsing on + // and off while WebRTC's decoder thread goes on decoding. + TestDecoderFactory decoderFactory = assertRoundTrip(new VideoCodecInfo(TEST_CODEC), false, true); + TestDecoder decoder = decoderFactory.last.get(); + + assertTrue(decoder.framesWithQp.get() > 0, "no frame was handed over with a QP"); + assertTrue(decoder.framesWithoutQp.get() > 0, "no frame was handed over without a QP"); } @Test @@ -213,17 +228,19 @@ void h265CallRoundTrips() throws Exception { // WebRTC has no H.265 codec of its own, so this is H.265 as a codec // factory brings it: negotiated, packetized into RTP and depacketized // as H.265, which WebRTC does only when built with it. - assertRoundTrip(H265, false); + assertRoundTrip(H265, false, false); } /** * Sends a call through the test codec and checks that every frame * received is one that was sent, and how WebRTC used the codecs. */ - private void assertRoundTrip(VideoCodecInfo codec, boolean javaBuffers) throws Exception { + private TestDecoderFactory assertRoundTrip(VideoCodecInfo codec, boolean javaBuffers, boolean asyncOutput) + throws Exception { TestEncoderFactory encoderFactory = new TestEncoderFactory(codec); TestDecoderFactory decoderFactory = new TestDecoderFactory(codec); decoderFactory.javaBuffers = javaBuffers; + decoderFactory.asyncOutput = asyncOutput; PeerConnectionFactory built = testCodecFactory(encoderFactory, decoderFactory); Set encodedSizes = ConcurrentHashMap.newKeySet(); @@ -279,6 +296,8 @@ private void assertRoundTrip(VideoCodecInfo codec, boolean javaBuffers) throws E assertTrue(encoder.rates.get() > 0, "rates were never set"); assertTrue(encoder.released.get(), "encoder was never released"); assertTrue(decoderFactory.last.get().released.get(), "decoder was never released"); + + return decoderFactory; } @Test @@ -681,6 +700,8 @@ static class TestDecoderFactory implements VideoDecoderFactory { volatile boolean javaBuffers; + volatile boolean asyncOutput; + TestDecoderFactory() { this(new VideoCodecInfo(TEST_CODEC, Collections.emptyMap())); @@ -706,7 +727,7 @@ public VideoDecoder createDecoder(VideoCodecInfo info) { return builtIn.createDecoder(info); } - TestDecoder decoder = new TestDecoder(javaBuffers, isH265(codec)); + TestDecoder decoder = new TestDecoder(javaBuffers, isH265(codec), asyncOutput); last.set(decoder); return decoder; } @@ -810,22 +831,31 @@ public ScalingSettings getScalingSettings() { /** * Decodes what {@link TestEncoder} produces into a frame of the encoded - * size, filled with the encoded luma sample. + * size, filled with the encoded luma sample. With asynchronous output it + * hands the frames over on a thread of its own, every other one with a + * QP, as a hardware decoder does from its output queue. */ static class TestDecoder implements VideoDecoder { final AtomicReference released = new AtomicReference<>(false); + final AtomicInteger framesWithQp = new AtomicInteger(); + final AtomicInteger framesWithoutQp = new AtomicInteger(); private final boolean javaBuffers; private final boolean h265; + private final ExecutorService output; + private Callback callback; + private int frames; + - TestDecoder(boolean javaBuffers, boolean h265) { + TestDecoder(boolean javaBuffers, boolean h265, boolean asyncOutput) { this.javaBuffers = javaBuffers; this.h265 = h265; + this.output = asyncOutput ? Executors.newSingleThreadExecutor() : null; } @Override @@ -836,6 +866,17 @@ public VideoCodecStatus initDecode(Settings settings, Callback callback) { @Override public VideoCodecStatus release() { + if (output != null) { + output.shutdown(); + + try { + output.awaitTermination(TIMEOUT_SECONDS, TimeUnit.SECONDS); + } + catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + released.set(true); return VideoCodecStatus.OK; } @@ -868,8 +909,28 @@ public VideoCodecStatus decode(EncodedImage image) { } VideoFrame frame = new VideoFrame(buffer, image.getCaptureTimeNs()); - callback.onDecodedFrame(frame, null, null); - frame.release(); + + if (output == null) { + callback.onDecodedFrame(frame, null, null); + frame.release(); + + return VideoCodecStatus.OK; + } + + Callback frameCallback = callback; + Integer qp = frames++ % 2 == 0 ? 30 : null; + + try { + output.execute(() -> { + frameCallback.onDecodedFrame(frame, null, qp); + frame.release(); + + (qp != null ? framesWithQp : framesWithoutQp).incrementAndGet(); + }); + } + catch (RejectedExecutionException e) { + frame.release(); + } return VideoCodecStatus.OK; }