From 2bde85046ad85757279d7bd0bf701ee0f70a0272 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sami=20Kalliom=C3=A4ki?= Date: Thu, 15 Feb 2018 13:58:15 +0100 Subject: [PATCH] Fix memory leak in NativeToJavaVideoFrame. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This method used to just wrap frame when passed a native frame and create a new one when passed non-native frame. This caused a memory leak when a new frame was returned because the caller didn't release the frame. Now the method always returns a new frame and the caller is responsible for releasing it. Bug: webrtc:8892, b/72675429 Change-Id: I06d67a6ed4c059cae1d709c51b0266f9c72fef1a Reviewed-on: https://webrtc-review.googlesource.com/53840 Reviewed-by: Anders Carlsson Commit-Queue: Sami Kalliomäki Cr-Commit-Position: refs/heads/master@{#22033} --- sdk/android/api/org/webrtc/VideoFrame.java | 1 + sdk/android/native_api/video/wrapper.cc | 2 +- sdk/android/native_api/video/wrapper.h | 3 ++- sdk/android/src/jni/androidmediaencoder.cc | 9 ++++++--- sdk/android/src/jni/videoencoderwrapper.cc | 6 ++++-- sdk/android/src/jni/videoframe.cc | 14 ++++++++++---- sdk/android/src/jni/videoframe.h | 7 +++++-- sdk/android/src/jni/videosink.cc | 4 +++- 8 files changed, 32 insertions(+), 14 deletions(-) diff --git a/sdk/android/api/org/webrtc/VideoFrame.java b/sdk/android/api/org/webrtc/VideoFrame.java index 3e8a1e4d8d..3b73543c55 100644 --- a/sdk/android/api/org/webrtc/VideoFrame.java +++ b/sdk/android/api/org/webrtc/VideoFrame.java @@ -177,6 +177,7 @@ public class VideoFrame { buffer.retain(); } + @CalledByNative public void release() { buffer.release(); } diff --git a/sdk/android/native_api/video/wrapper.cc b/sdk/android/native_api/video/wrapper.cc index 0f1bcafa1b..cc626c817a 100644 --- a/sdk/android/native_api/video/wrapper.cc +++ b/sdk/android/native_api/video/wrapper.cc @@ -26,7 +26,7 @@ std::unique_ptr> JavaToNativeVideoSink( ScopedJavaLocalRef NativeToJavaVideoFrame(JNIEnv* jni, const VideoFrame& frame) { - return jni::NativeToJavaFrame(jni, frame); + return jni::NativeToJavaVideoFrame(jni, frame); } } // namespace webrtc diff --git a/sdk/android/native_api/video/wrapper.h b/sdk/android/native_api/video/wrapper.h index 35c305ca6c..fc670ea916 100644 --- a/sdk/android/native_api/video/wrapper.h +++ b/sdk/android/native_api/video/wrapper.h @@ -26,7 +26,8 @@ std::unique_ptr> JavaToNativeVideoSink( JNIEnv* jni, jobject video_sink); -// Creates a Java VideoFrame object from a native VideoFrame. +// Creates a Java VideoFrame object from a native VideoFrame. The returned +// object has to be released by calling release. ScopedJavaLocalRef NativeToJavaVideoFrame(JNIEnv* jni, const VideoFrame& frame); diff --git a/sdk/android/src/jni/androidmediaencoder.cc b/sdk/android/src/jni/androidmediaencoder.cc index f71b508ef4..7f835f76dc 100644 --- a/sdk/android/src/jni/androidmediaencoder.cc +++ b/sdk/android/src/jni/androidmediaencoder.cc @@ -728,11 +728,14 @@ int32_t MediaCodecVideoEncoder::Encode( case AndroidVideoFrameBuffer::AndroidType::kTextureBuffer: encode_status = EncodeTexture(jni, key_frame, input_frame); break; - case AndroidVideoFrameBuffer::AndroidType::kJavaBuffer: + case AndroidVideoFrameBuffer::AndroidType::kJavaBuffer: { + ScopedJavaLocalRef j_frame = + NativeToJavaVideoFrame(jni, frame); encode_status = - EncodeJavaFrame(jni, key_frame, NativeToJavaFrame(jni, input_frame), - j_input_buffer_index); + EncodeJavaFrame(jni, key_frame, j_frame, j_input_buffer_index); + ReleaseJavaVideoFrame(jni, j_frame); break; + } default: RTC_NOTREACHED(); return WEBRTC_VIDEO_CODEC_ERROR; diff --git a/sdk/android/src/jni/videoencoderwrapper.cc b/sdk/android/src/jni/videoencoderwrapper.cc index 5ef30c18fd..ef0cdb485e 100644 --- a/sdk/android/src/jni/videoencoderwrapper.cc +++ b/sdk/android/src/jni/videoencoderwrapper.cc @@ -128,8 +128,10 @@ int32_t VideoEncoderWrapper::Encode( info.timestamp_rtp = frame.timestamp(); frame_extra_infos_.push_back(info); - ScopedJavaLocalRef ret = Java_VideoEncoder_encode( - jni, encoder_, NativeToJavaFrame(jni, frame), encode_info); + ScopedJavaLocalRef j_frame = NativeToJavaVideoFrame(jni, frame); + ScopedJavaLocalRef ret = + Java_VideoEncoder_encode(jni, encoder_, j_frame, encode_info); + ReleaseJavaVideoFrame(jni, j_frame); return HandleReturnCode(jni, ret); } diff --git a/sdk/android/src/jni/videoframe.cc b/sdk/android/src/jni/videoframe.cc index f638e196b9..22867e47c6 100644 --- a/sdk/android/src/jni/videoframe.cc +++ b/sdk/android/src/jni/videoframe.cc @@ -383,8 +383,8 @@ static bool IsJavaVideoBuffer(rtc::scoped_refptr buffer) { AndroidVideoFrameBuffer::AndroidType::kJavaBuffer; } -ScopedJavaLocalRef NativeToJavaFrame(JNIEnv* jni, - const VideoFrame& frame) { +ScopedJavaLocalRef NativeToJavaVideoFrame(JNIEnv* jni, + const VideoFrame& frame) { rtc::scoped_refptr buffer = frame.video_frame_buffer(); if (IsJavaVideoBuffer(buffer)) { @@ -396,9 +396,11 @@ ScopedJavaLocalRef NativeToJavaFrame(JNIEnv* jni, AndroidVideoBuffer* android_video_buffer = static_cast(android_buffer); + ScopedJavaLocalRef j_video_frame_buffer( + jni, android_video_buffer->video_frame_buffer()); + Java_Buffer_retain(jni, j_video_frame_buffer); return Java_VideoFrame_Constructor( - jni, android_video_buffer->video_frame_buffer(), - static_cast(frame.rotation()), + jni, j_video_frame_buffer, static_cast(frame.rotation()), static_cast(frame.timestamp_us() * rtc::kNumNanosecsPerMicrosec)); } else { @@ -410,6 +412,10 @@ ScopedJavaLocalRef NativeToJavaFrame(JNIEnv* jni, } } +void ReleaseJavaVideoFrame(JNIEnv* jni, const JavaRef& j_video_frame) { + Java_VideoFrame_release(jni, j_video_frame); +} + static void JNI_VideoFrame_CropAndScaleI420( JNIEnv* jni, const JavaParamRef&, diff --git a/sdk/android/src/jni/videoframe.h b/sdk/android/src/jni/videoframe.h index 5cb7a2d142..6f5720d78d 100644 --- a/sdk/android/src/jni/videoframe.h +++ b/sdk/android/src/jni/videoframe.h @@ -155,8 +155,11 @@ VideoFrame JavaToNativeFrame(JNIEnv* jni, const JavaRef& j_video_frame, uint32_t timestamp_rtp); -ScopedJavaLocalRef NativeToJavaFrame(JNIEnv* jni, - const VideoFrame& frame); +// NOTE: Returns a new video frame that has to be released by calling +// ReleaseJavaVideoFrame. +ScopedJavaLocalRef NativeToJavaVideoFrame(JNIEnv* jni, + const VideoFrame& frame); +void ReleaseJavaVideoFrame(JNIEnv* jni, const JavaRef& j_video_frame); int64_t GetJavaVideoFrameTimestampNs(JNIEnv* jni, const JavaRef& j_video_frame); diff --git a/sdk/android/src/jni/videosink.cc b/sdk/android/src/jni/videosink.cc index 89d6d2e827..bbdda31431 100644 --- a/sdk/android/src/jni/videosink.cc +++ b/sdk/android/src/jni/videosink.cc @@ -23,7 +23,9 @@ VideoSinkWrapper::~VideoSinkWrapper() {} void VideoSinkWrapper::OnFrame(const VideoFrame& frame) { JNIEnv* jni = AttachCurrentThreadIfNeeded(); - Java_VideoSink_onFrame(jni, j_sink_, NativeToJavaFrame(jni, frame)); + ScopedJavaLocalRef j_frame = NativeToJavaVideoFrame(jni, frame); + Java_VideoSink_onFrame(jni, j_sink_, j_frame); + ReleaseJavaVideoFrame(jni, j_frame); } } // namespace jni