Transcoder: Fixed codec leaks on cancel.
When transcoding was cancelled, the VideoTrackTranscoder
held strong references to itself through its internal
codec handler work queue.
This commit clears the work queue and blocks new messages
from being stored on cancel.
Test: Unit test and address sanitizer.
Fixes: 168522198
Change-Id: I9c3a4469378a91a492c6872bafc6d494dadf2853
diff --git a/media/libmediatranscoding/transcoder/Android.bp b/media/libmediatranscoding/transcoder/Android.bp
index c153a42..258ed9a 100644
--- a/media/libmediatranscoding/transcoder/Android.bp
+++ b/media/libmediatranscoding/transcoder/Android.bp
@@ -14,8 +14,8 @@
* limitations under the License.
*/
-cc_library_shared {
- name: "libmediatranscoder",
+cc_defaults {
+ name: "mediatranscoder_defaults",
srcs: [
"MediaSampleQueue.cpp",
@@ -60,3 +60,17 @@
cfi: true,
},
}
+
+cc_library_shared {
+ name: "libmediatranscoder",
+ defaults: ["mediatranscoder_defaults"],
+}
+
+cc_library_shared {
+ name: "libmediatranscoder_asan",
+ defaults: ["mediatranscoder_defaults"],
+
+ sanitize: {
+ address: true,
+ },
+}
diff --git a/media/libmediatranscoding/transcoder/MediaTranscoder.cpp b/media/libmediatranscoding/transcoder/MediaTranscoder.cpp
index 61cc459..e850d66 100644
--- a/media/libmediatranscoding/transcoder/MediaTranscoder.cpp
+++ b/media/libmediatranscoding/transcoder/MediaTranscoder.cpp
@@ -155,7 +155,7 @@
}
void MediaTranscoder::onTrackError(const MediaTrackTranscoder* transcoder, media_status_t status) {
- LOG(DEBUG) << "TrackTranscoder " << transcoder << " returned error " << status;
+ LOG(ERROR) << "TrackTranscoder " << transcoder << " returned error " << status;
sendCallback(status);
}
diff --git a/media/libmediatranscoding/transcoder/VideoTrackTranscoder.cpp b/media/libmediatranscoding/transcoder/VideoTrackTranscoder.cpp
index c7d775c..5579868 100644
--- a/media/libmediatranscoding/transcoder/VideoTrackTranscoder.cpp
+++ b/media/libmediatranscoding/transcoder/VideoTrackTranscoder.cpp
@@ -40,7 +40,11 @@
template <typename T>
void VideoTrackTranscoder::BlockingQueue<T>::push(T const& value, bool front) {
{
- std::unique_lock<std::mutex> lock(mMutex);
+ std::scoped_lock lock(mMutex);
+ if (mAborted) {
+ return;
+ }
+
if (front) {
mQueue.push_front(value);
} else {
@@ -52,7 +56,7 @@
template <typename T>
T VideoTrackTranscoder::BlockingQueue<T>::pop() {
- std::unique_lock<std::mutex> lock(mMutex);
+ std::unique_lock lock(mMutex);
while (mQueue.empty()) {
mCondition.wait(lock);
}
@@ -61,6 +65,14 @@
return value;
}
+// Note: Do not call if another thread might waiting in pop.
+template <typename T>
+void VideoTrackTranscoder::BlockingQueue<T>::abort() {
+ std::scoped_lock lock(mMutex);
+ mAborted = true;
+ mQueue.clear();
+}
+
// The CodecWrapper class is used to let AMediaCodec instances outlive the transcoder object itself
// by giving the codec a weak pointer to the transcoder. Codecs wrapped in this object are kept
// alive by the transcoder and the codec's outstanding buffers. Once the transcoder stops and all
@@ -484,12 +496,14 @@
message();
}
+ mCodecMessageQueue.abort();
+ AMediaCodec_stop(mDecoder);
+
// Return error if transcoding was stopped before it finished.
if (mStopRequested && !mEosFromEncoder && mStatus == AMEDIA_OK) {
mStatus = AMEDIA_ERROR_UNKNOWN; // TODO: Define custom error codes?
}
- AMediaCodec_stop(mDecoder);
return mStatus;
}
diff --git a/media/libmediatranscoding/transcoder/include/media/VideoTrackTranscoder.h b/media/libmediatranscoding/transcoder/include/media/VideoTrackTranscoder.h
index 0a7bf33..d000d7f 100644
--- a/media/libmediatranscoding/transcoder/include/media/VideoTrackTranscoder.h
+++ b/media/libmediatranscoding/transcoder/include/media/VideoTrackTranscoder.h
@@ -51,11 +51,13 @@
public:
void push(T const& value, bool front = false);
T pop();
+ void abort();
private:
std::mutex mMutex;
std::condition_variable mCondition;
std::deque<T> mQueue;
+ bool mAborted = false;
};
class CodecWrapper;
diff --git a/media/libmediatranscoding/transcoder/tests/Android.bp b/media/libmediatranscoding/transcoder/tests/Android.bp
index 4160c30..7ae6261 100644
--- a/media/libmediatranscoding/transcoder/tests/Android.bp
+++ b/media/libmediatranscoding/transcoder/tests/Android.bp
@@ -17,7 +17,7 @@
"libbase",
"libcutils",
"libmediandk",
- "libmediatranscoder",
+ "libmediatranscoder_asan",
"libutils",
],
@@ -26,6 +26,15 @@
"-Wall",
],
+ sanitize: {
+ misc_undefined: [
+ "unsigned-integer-overflow",
+ "signed-integer-overflow",
+ ],
+ cfi: true,
+ address: true,
+ },
+
data: [":test_assets"],
test_config_template: "AndroidTestTemplate.xml",
test_suites: ["device-tests", "TranscoderTests"],
@@ -80,4 +89,5 @@
name: "MediaTranscoderTests",
defaults: ["testdefaults"],
srcs: ["MediaTranscoderTests.cpp"],
+ shared_libs: ["libbinder_ndk"],
}
diff --git a/media/libmediatranscoding/transcoder/tests/MediaTranscoderTests.cpp b/media/libmediatranscoding/transcoder/tests/MediaTranscoderTests.cpp
index 53bd2a2..7a968eb 100644
--- a/media/libmediatranscoding/transcoder/tests/MediaTranscoderTests.cpp
+++ b/media/libmediatranscoding/transcoder/tests/MediaTranscoderTests.cpp
@@ -20,6 +20,7 @@
#define LOG_TAG "MediaTranscoderTests"
#include <android-base/logging.h>
+#include <android/binder_process.h>
#include <fcntl.h>
#include <gtest/gtest.h>
#include <media/MediaSampleReaderNDK.h>
@@ -72,7 +73,13 @@
}
virtual void onProgressUpdate(const MediaTranscoder* transcoder __unused,
- int32_t progress __unused) override {}
+ int32_t progress) override {
+ std::unique_lock<std::mutex> lock(mMutex);
+ if (progress > 0 && !mProgressMade) {
+ mProgressMade = true;
+ mCondition.notify_all();
+ }
+ }
virtual void onCodecResourceLost(const MediaTranscoder* transcoder __unused,
const std::shared_ptr<const Parcel>& pausedState
@@ -85,12 +92,19 @@
}
}
+ void waitForProgressMade() {
+ std::unique_lock<std::mutex> lock(mMutex);
+ while (!mProgressMade && !mFinished) {
+ mCondition.wait(lock);
+ }
+ }
media_status_t mStatus = AMEDIA_OK;
private:
std::mutex mMutex;
std::condition_variable mCondition;
bool mFinished = false;
+ bool mProgressMade = false;
};
// Write-only, create file if non-existent, don't overwrite existing file.
@@ -106,6 +120,7 @@
void SetUp() override {
LOG(DEBUG) << "MediaTranscoderTests set up";
mCallbacks = std::make_shared<TestCallbacks>();
+ ABinderProcess_startThreadPool();
}
void TearDown() override {
@@ -126,9 +141,16 @@
return (float)diff * 100.0f / s1.st_size;
}
+ typedef enum {
+ kRunToCompletion,
+ kCancelAfterProgress,
+ kCancelAfterStart,
+ } TranscodeExecutionControl;
+
using FormatConfigurationCallback = std::function<AMediaFormat*(AMediaFormat*)>;
media_status_t transcodeHelper(const char* srcPath, const char* destPath,
- FormatConfigurationCallback formatCallback) {
+ FormatConfigurationCallback formatCallback,
+ TranscodeExecutionControl executionControl = kRunToCompletion) {
auto transcoder = MediaTranscoder::create(mCallbacks, nullptr);
EXPECT_NE(transcoder, nullptr);
@@ -160,7 +182,18 @@
media_status_t startStatus = transcoder->start();
EXPECT_EQ(startStatus, AMEDIA_OK);
if (startStatus == AMEDIA_OK) {
- mCallbacks->waitForTranscodingFinished();
+ switch (executionControl) {
+ case kCancelAfterProgress:
+ mCallbacks->waitForProgressMade();
+ FALLTHROUGH_INTENDED;
+ case kCancelAfterStart:
+ transcoder->cancel();
+ break;
+ case kRunToCompletion:
+ default:
+ mCallbacks->waitForTranscodingFinished();
+ break;
+ }
}
close(srcFd);
close(dstFd);
@@ -310,6 +343,41 @@
EXPECT_GT(getFileSizeDiffPercent(destPath1, destPath2), 40);
}
+static AMediaFormat* getAVCVideoFormat(AMediaFormat* sourceFormat) {
+ AMediaFormat* format = nullptr;
+ const char* mime = nullptr;
+ AMediaFormat_getString(sourceFormat, AMEDIAFORMAT_KEY_MIME, &mime);
+
+ if (strncmp(mime, "video/", 6) == 0) {
+ format = AMediaFormat_new();
+ AMediaFormat_setString(format, AMEDIAFORMAT_KEY_MIME, AMEDIA_MIMETYPE_VIDEO_AVC);
+ }
+
+ return format;
+}
+
+TEST_F(MediaTranscoderTests, TestCancelAfterProgress) {
+ const char* srcPath = "/data/local/tmp/TranscodingTestAssets/longtest_15s.mp4";
+ const char* destPath = "/data/local/tmp/MediaTranscoder_Cancel.MP4";
+
+ for (int i = 0; i < 32; ++i) {
+ EXPECT_EQ(transcodeHelper(srcPath, destPath, getAVCVideoFormat, kCancelAfterProgress),
+ AMEDIA_OK);
+ mCallbacks = std::make_shared<TestCallbacks>();
+ }
+}
+
+TEST_F(MediaTranscoderTests, TestCancelAfterStart) {
+ const char* srcPath = "/data/local/tmp/TranscodingTestAssets/longtest_15s.mp4";
+ const char* destPath = "/data/local/tmp/MediaTranscoder_Cancel.MP4";
+
+ for (int i = 0; i < 32; ++i) {
+ EXPECT_EQ(transcodeHelper(srcPath, destPath, getAVCVideoFormat, kCancelAfterStart),
+ AMEDIA_OK);
+ mCallbacks = std::make_shared<TestCallbacks>();
+ }
+}
+
} // namespace android
int main(int argc, char** argv) {
diff --git a/media/libmediatranscoding/transcoder/tests/build_and_run_all_unit_tests.sh b/media/libmediatranscoding/transcoder/tests/build_and_run_all_unit_tests.sh
index 241178d..b848b4c 100755
--- a/media/libmediatranscoding/transcoder/tests/build_and_run_all_unit_tests.sh
+++ b/media/libmediatranscoding/transcoder/tests/build_and_run_all_unit_tests.sh
@@ -25,22 +25,22 @@
echo "========================================"
echo "testing MediaSampleReaderNDK"
-adb shell /data/nativetest64/MediaSampleReaderNDKTests/MediaSampleReaderNDKTests
+adb shell ASAN_OPTIONS=detect_container_overflow=0 /data/nativetest64/MediaSampleReaderNDKTests/MediaSampleReaderNDKTests
echo "testing MediaSampleQueue"
-adb shell /data/nativetest64/MediaSampleQueueTests/MediaSampleQueueTests
+adb shell ASAN_OPTIONS=detect_container_overflow=0 /data/nativetest64/MediaSampleQueueTests/MediaSampleQueueTests
echo "testing MediaTrackTranscoder"
-adb shell /data/nativetest64/MediaTrackTranscoderTests/MediaTrackTranscoderTests
+adb shell ASAN_OPTIONS=detect_container_overflow=0 /data/nativetest64/MediaTrackTranscoderTests/MediaTrackTranscoderTests
echo "testing VideoTrackTranscoder"
-adb shell /data/nativetest64/VideoTrackTranscoderTests/VideoTrackTranscoderTests
+adb shell ASAN_OPTIONS=detect_container_overflow=0 /data/nativetest64/VideoTrackTranscoderTests/VideoTrackTranscoderTests
echo "testing PassthroughTrackTranscoder"
-adb shell /data/nativetest64/PassthroughTrackTranscoderTests/PassthroughTrackTranscoderTests
+adb shell ASAN_OPTIONS=detect_container_overflow=0 /data/nativetest64/PassthroughTrackTranscoderTests/PassthroughTrackTranscoderTests
echo "testing MediaSampleWriter"
-adb shell /data/nativetest64/MediaSampleWriterTests/MediaSampleWriterTests
+adb shell ASAN_OPTIONS=detect_container_overflow=0 /data/nativetest64/MediaSampleWriterTests/MediaSampleWriterTests
echo "testing MediaTranscoder"
-adb shell /data/nativetest64/MediaTranscoderTests/MediaTranscoderTests
+adb shell ASAN_OPTIONS=detect_container_overflow=0 /data/nativetest64/MediaTranscoderTests/MediaTranscoderTests