Transcoder: Fix video track transcoding crash.
The VideoTrackTranscoder had a bug where the encoder could
outlive the transcoder object, because the encoder owns the
output buffers. This caused a crash when the encoder sent async
callbacks to the transcoder after it had been released. This fix
gives the encoder a weak reference to the transcoder and gives
outstanding buffers a strong reference to the encoder.
Fixes: 160711746
Test: Transcoder unit tests (build_and_run_all_unit_tests.sh).
Test: New unit test to trigger the crash before the fix.
Change-Id: I8141591399b6cff642d2d322809d3254adbefaaf
diff --git a/media/libmediatranscoding/transcoder/MediaTrackTranscoder.cpp b/media/libmediatranscoding/transcoder/MediaTrackTranscoder.cpp
index b485826..e3996b0 100644
--- a/media/libmediatranscoding/transcoder/MediaTrackTranscoder.cpp
+++ b/media/libmediatranscoding/transcoder/MediaTrackTranscoder.cpp
@@ -94,6 +94,7 @@
if (mState == STARTED) {
abortTranscodeLoop();
mTranscodingThread.join();
+ mOutputQueue->abort(); // Wake up any threads waiting for samples.
mState = STOPPED;
return true;
}
diff --git a/media/libmediatranscoding/transcoder/MediaTranscoder.cpp b/media/libmediatranscoding/transcoder/MediaTranscoder.cpp
index bde1cf6..999e226 100644
--- a/media/libmediatranscoding/transcoder/MediaTranscoder.cpp
+++ b/media/libmediatranscoding/transcoder/MediaTranscoder.cpp
@@ -224,11 +224,11 @@
return AMEDIA_ERROR_INVALID_PARAMETER;
}
- std::unique_ptr<MediaTrackTranscoder> transcoder = nullptr;
- std::shared_ptr<AMediaFormat> format = nullptr;
+ std::shared_ptr<MediaTrackTranscoder> transcoder;
+ std::shared_ptr<AMediaFormat> format;
if (trackFormat == nullptr) {
- transcoder = std::make_unique<PassthroughTrackTranscoder>(shared_from_this());
+ transcoder = std::make_shared<PassthroughTrackTranscoder>(shared_from_this());
} else {
const char* srcMime = nullptr;
if (!AMediaFormat_getString(mSourceTrackFormats[trackIndex].get(), AMEDIAFORMAT_KEY_MIME,
@@ -253,7 +253,7 @@
}
}
- transcoder = std::make_unique<VideoTrackTranscoder>(shared_from_this());
+ transcoder = VideoTrackTranscoder::create(shared_from_this());
AMediaFormat* mergedFormat =
mergeMediaFormats(mSourceTrackFormats[trackIndex].get(), trackFormat);
diff --git a/media/libmediatranscoding/transcoder/VideoTrackTranscoder.cpp b/media/libmediatranscoding/transcoder/VideoTrackTranscoder.cpp
index 8ee252f..65dcad3 100644
--- a/media/libmediatranscoding/transcoder/VideoTrackTranscoder.cpp
+++ b/media/libmediatranscoding/transcoder/VideoTrackTranscoder.cpp
@@ -59,36 +59,70 @@
return value;
}
+// 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
+// output buffers have been released by downstream components the codec will also be released.
+class VideoTrackTranscoder::CodecWrapper {
+public:
+ CodecWrapper(AMediaCodec* codec, const std::weak_ptr<VideoTrackTranscoder>& transcoder)
+ : mCodec(codec), mTranscoder(transcoder), mCodecStarted(false) {}
+ ~CodecWrapper() {
+ if (mCodecStarted) {
+ AMediaCodec_stop(mCodec);
+ }
+ AMediaCodec_delete(mCodec);
+ }
+
+ AMediaCodec* getCodec() { return mCodec; }
+ std::shared_ptr<VideoTrackTranscoder> getTranscoder() const { return mTranscoder.lock(); };
+ void setStarted() { mCodecStarted = true; }
+
+private:
+ AMediaCodec* mCodec;
+ std::weak_ptr<VideoTrackTranscoder> mTranscoder;
+ bool mCodecStarted;
+};
+
// Dispatch responses to codec callbacks onto the message queue.
struct AsyncCodecCallbackDispatch {
static void onAsyncInputAvailable(AMediaCodec* codec, void* userdata, int32_t index) {
- VideoTrackTranscoder* transcoder = static_cast<VideoTrackTranscoder*>(userdata);
- if (codec == transcoder->mDecoder) {
- transcoder->mCodecMessageQueue.push(
- [transcoder, index] { transcoder->enqueueInputSample(index); });
+ VideoTrackTranscoder::CodecWrapper* wrapper =
+ static_cast<VideoTrackTranscoder::CodecWrapper*>(userdata);
+ if (auto transcoder = wrapper->getTranscoder()) {
+ if (codec == transcoder->mDecoder) {
+ transcoder->mCodecMessageQueue.push(
+ [transcoder, index] { transcoder->enqueueInputSample(index); });
+ }
}
}
static void onAsyncOutputAvailable(AMediaCodec* codec, void* userdata, int32_t index,
AMediaCodecBufferInfo* bufferInfoPtr) {
- VideoTrackTranscoder* transcoder = static_cast<VideoTrackTranscoder*>(userdata);
+ VideoTrackTranscoder::CodecWrapper* wrapper =
+ static_cast<VideoTrackTranscoder::CodecWrapper*>(userdata);
AMediaCodecBufferInfo bufferInfo = *bufferInfoPtr;
- transcoder->mCodecMessageQueue.push([transcoder, index, codec, bufferInfo] {
- if (codec == transcoder->mDecoder) {
- transcoder->transferBuffer(index, bufferInfo);
- } else if (codec == transcoder->mEncoder.get()) {
- transcoder->dequeueOutputSample(index, bufferInfo);
- }
- });
+ if (auto transcoder = wrapper->getTranscoder()) {
+ transcoder->mCodecMessageQueue.push([transcoder, index, codec, bufferInfo] {
+ if (codec == transcoder->mDecoder) {
+ transcoder->transferBuffer(index, bufferInfo);
+ } else if (codec == transcoder->mEncoder->getCodec()) {
+ transcoder->dequeueOutputSample(index, bufferInfo);
+ }
+ });
+ }
}
static void onAsyncFormatChanged(AMediaCodec* codec, void* userdata, AMediaFormat* format) {
- VideoTrackTranscoder* transcoder = static_cast<VideoTrackTranscoder*>(userdata);
- const char* kCodecName = (codec == transcoder->mDecoder ? "Decoder" : "Encoder");
- LOG(DEBUG) << kCodecName << " format changed: " << AMediaFormat_toString(format);
- if (codec == transcoder->mEncoder.get()) {
- transcoder->mCodecMessageQueue.push(
- [transcoder, format] { transcoder->updateTrackFormat(format); });
+ VideoTrackTranscoder::CodecWrapper* wrapper =
+ static_cast<VideoTrackTranscoder::CodecWrapper*>(userdata);
+ if (auto transcoder = wrapper->getTranscoder()) {
+ const char* kCodecName = (codec == transcoder->mDecoder ? "Decoder" : "Encoder");
+ LOG(DEBUG) << kCodecName << " format changed: " << AMediaFormat_toString(format);
+ if (codec == transcoder->mEncoder->getCodec()) {
+ transcoder->mCodecMessageQueue.push(
+ [transcoder, format] { transcoder->updateTrackFormat(format); });
+ }
}
}
@@ -96,16 +130,25 @@
int32_t actionCode, const char* detail) {
LOG(ERROR) << "Error from codec " << codec << ", userdata " << userdata << ", error "
<< error << ", action " << actionCode << ", detail " << detail;
- VideoTrackTranscoder* transcoder = static_cast<VideoTrackTranscoder*>(userdata);
- transcoder->mCodecMessageQueue.push(
- [transcoder, error] {
- transcoder->mStatus = error;
- transcoder->mStopRequested = true;
- },
- true);
+ VideoTrackTranscoder::CodecWrapper* wrapper =
+ static_cast<VideoTrackTranscoder::CodecWrapper*>(userdata);
+ if (auto transcoder = wrapper->getTranscoder()) {
+ transcoder->mCodecMessageQueue.push(
+ [transcoder, error] {
+ transcoder->mStatus = error;
+ transcoder->mStopRequested = true;
+ },
+ true);
+ }
}
};
+// static
+std::shared_ptr<VideoTrackTranscoder> VideoTrackTranscoder::create(
+ const std::weak_ptr<MediaTrackTranscoderCallback>& transcoderCallback) {
+ return std::shared_ptr<VideoTrackTranscoder>(new VideoTrackTranscoder(transcoderCallback));
+}
+
VideoTrackTranscoder::~VideoTrackTranscoder() {
if (mDecoder != nullptr) {
AMediaCodec_delete(mDecoder);
@@ -159,17 +202,17 @@
LOG(ERROR) << "Unable to create encoder for type " << destinationMime;
return AMEDIA_ERROR_UNSUPPORTED;
}
- mEncoder = std::shared_ptr<AMediaCodec>(encoder,
- std::bind(AMediaCodec_delete, std::placeholders::_1));
+ mEncoder = std::make_shared<CodecWrapper>(encoder, shared_from_this());
- status = AMediaCodec_configure(mEncoder.get(), mDestinationFormat.get(), NULL /* surface */,
- NULL /* crypto */, AMEDIACODEC_CONFIGURE_FLAG_ENCODE);
+ status = AMediaCodec_configure(mEncoder->getCodec(), mDestinationFormat.get(),
+ NULL /* surface */, NULL /* crypto */,
+ AMEDIACODEC_CONFIGURE_FLAG_ENCODE);
if (status != AMEDIA_OK) {
LOG(ERROR) << "Unable to configure video encoder: " << status;
return status;
}
- status = AMediaCodec_createInputSurface(mEncoder.get(), &mSurface);
+ status = AMediaCodec_createInputSurface(mEncoder->getCodec(), &mSurface);
if (status != AMEDIA_OK) {
LOG(ERROR) << "Unable to create an encoder input surface: %d" << status;
return status;
@@ -203,13 +246,17 @@
.onAsyncFormatChanged = AsyncCodecCallbackDispatch::onAsyncFormatChanged,
.onAsyncError = AsyncCodecCallbackDispatch::onAsyncError};
- status = AMediaCodec_setAsyncNotifyCallback(mDecoder, asyncCodecCallbacks, this);
+ // Note: The decoder does not need its own wrapper because its lifetime is tied to the
+ // transcoder. But the same callbacks are reused for decoder and encoder so we pass the encoder
+ // wrapper as userdata here but never read the codec from it in the callback.
+ status = AMediaCodec_setAsyncNotifyCallback(mDecoder, asyncCodecCallbacks, mEncoder.get());
if (status != AMEDIA_OK) {
LOG(ERROR) << "Unable to set decoder to async mode: " << status;
return status;
}
- status = AMediaCodec_setAsyncNotifyCallback(mEncoder.get(), asyncCodecCallbacks, this);
+ status = AMediaCodec_setAsyncNotifyCallback(mEncoder->getCodec(), asyncCodecCallbacks,
+ mEncoder.get());
if (status != AMEDIA_OK) {
LOG(ERROR) << "Unable to set encoder to async mode: " << status;
return status;
@@ -277,7 +324,7 @@
if (bufferInfo.flags & AMEDIACODEC_BUFFER_FLAG_END_OF_STREAM) {
LOG(DEBUG) << "EOS from decoder.";
- media_status_t status = AMediaCodec_signalEndOfInputStream(mEncoder.get());
+ media_status_t status = AMediaCodec_signalEndOfInputStream(mEncoder->getCodec());
if (status != AMEDIA_OK) {
LOG(ERROR) << "SignalEOS on encoder returned error: " << status;
mStatus = status;
@@ -289,12 +336,14 @@
AMediaCodecBufferInfo bufferInfo) {
if (bufferIndex >= 0) {
size_t sampleSize = 0;
- uint8_t* buffer = AMediaCodec_getOutputBuffer(mEncoder.get(), bufferIndex, &sampleSize);
+ uint8_t* buffer =
+ AMediaCodec_getOutputBuffer(mEncoder->getCodec(), bufferIndex, &sampleSize);
- MediaSample::OnSampleReleasedCallback bufferReleaseCallback = [encoder = mEncoder](
- MediaSample* sample) {
- AMediaCodec_releaseOutputBuffer(encoder.get(), sample->bufferId, false /* render */);
- };
+ MediaSample::OnSampleReleasedCallback bufferReleaseCallback =
+ [encoder = mEncoder](MediaSample* sample) {
+ AMediaCodec_releaseOutputBuffer(encoder->getCodec(), sample->bufferId,
+ false /* render */);
+ };
std::shared_ptr<MediaSample> sample = MediaSample::createWithReleaseCallback(
buffer, bufferInfo.offset, bufferIndex, bufferReleaseCallback);
@@ -309,7 +358,7 @@
return;
}
} else if (bufferIndex == AMEDIACODEC_INFO_OUTPUT_FORMAT_CHANGED) {
- AMediaFormat* newFormat = AMediaCodec_getOutputFormat(mEncoder.get());
+ AMediaFormat* newFormat = AMediaCodec_getOutputFormat(mEncoder->getCodec());
LOG(DEBUG) << "Encoder output format changed: " << AMediaFormat_toString(newFormat);
}
@@ -400,11 +449,12 @@
});
mCodecMessageQueue.push([this] {
- media_status_t status = AMediaCodec_start(mEncoder.get());
+ media_status_t status = AMediaCodec_start(mEncoder->getCodec());
if (status != AMEDIA_OK) {
LOG(ERROR) << "Unable to start video encoder: " << status;
mStatus = status;
}
+ mEncoder->setStarted();
});
// Process codec events until EOS is reached, transcoding is stopped or an error occurs.
@@ -419,8 +469,6 @@
}
AMediaCodec_stop(mDecoder);
- // TODO: Stop invalidates all buffers. Stop encoder when last buffer is released.
- // AMediaCodec_stop(mEncoder.get());
return mStatus;
}
diff --git a/media/libmediatranscoding/transcoder/include/media/MediaTranscoder.h b/media/libmediatranscoding/transcoder/include/media/MediaTranscoder.h
index 7a36c8c..33fa558 100644
--- a/media/libmediatranscoding/transcoder/include/media/MediaTranscoder.h
+++ b/media/libmediatranscoding/transcoder/include/media/MediaTranscoder.h
@@ -133,7 +133,7 @@
std::shared_ptr<MediaSampleReader> mSampleReader;
std::unique_ptr<MediaSampleWriter> mSampleWriter;
std::vector<std::shared_ptr<AMediaFormat>> mSourceTrackFormats;
- std::vector<std::unique_ptr<MediaTrackTranscoder>> mTrackTranscoders;
+ std::vector<std::shared_ptr<MediaTrackTranscoder>> mTrackTranscoders;
std::mutex mTracksAddedMutex;
std::unordered_set<const MediaTrackTranscoder*> mTracksAdded GUARDED_BY(mTracksAddedMutex);
diff --git a/media/libmediatranscoding/transcoder/include/media/VideoTrackTranscoder.h b/media/libmediatranscoding/transcoder/include/media/VideoTrackTranscoder.h
index 1ba205b..0a7bf33 100644
--- a/media/libmediatranscoding/transcoder/include/media/VideoTrackTranscoder.h
+++ b/media/libmediatranscoding/transcoder/include/media/VideoTrackTranscoder.h
@@ -34,10 +34,12 @@
* using a native surface (ANativeWindow). Codec callback events are placed on a message queue and
* serviced in order on the transcoding thread managed by MediaTrackTranscoder.
*/
-class VideoTrackTranscoder : public MediaTrackTranscoder {
+class VideoTrackTranscoder : public std::enable_shared_from_this<VideoTrackTranscoder>,
+ public MediaTrackTranscoder {
public:
- VideoTrackTranscoder(const std::weak_ptr<MediaTrackTranscoderCallback>& transcoderCallback)
- : MediaTrackTranscoder(transcoderCallback){};
+ static std::shared_ptr<VideoTrackTranscoder> create(
+ const std::weak_ptr<MediaTrackTranscoderCallback>& transcoderCallback);
+
virtual ~VideoTrackTranscoder() override;
private:
@@ -55,6 +57,10 @@
std::condition_variable mCondition;
std::deque<T> mQueue;
};
+ class CodecWrapper;
+
+ VideoTrackTranscoder(const std::weak_ptr<MediaTrackTranscoderCallback>& transcoderCallback)
+ : MediaTrackTranscoder(transcoderCallback){};
// MediaTrackTranscoder
media_status_t runTranscodeLoop() override;
@@ -77,8 +83,7 @@
void updateTrackFormat(AMediaFormat* outputFormat);
AMediaCodec* mDecoder = nullptr;
- // Sample release callback holds a reference to the encoder, hence the shared_ptr.
- std::shared_ptr<AMediaCodec> mEncoder;
+ std::shared_ptr<CodecWrapper> mEncoder;
ANativeWindow* mSurface = nullptr;
bool mEosFromSource = false;
bool mEosFromEncoder = false;
diff --git a/media/libmediatranscoding/transcoder/tests/MediaTrackTranscoderTests.cpp b/media/libmediatranscoding/transcoder/tests/MediaTrackTranscoderTests.cpp
index 71d3a4e..502d5aa 100644
--- a/media/libmediatranscoding/transcoder/tests/MediaTrackTranscoderTests.cpp
+++ b/media/libmediatranscoding/transcoder/tests/MediaTrackTranscoderTests.cpp
@@ -53,7 +53,7 @@
switch (GetParam()) {
case VIDEO:
- mTranscoder = std::make_shared<VideoTrackTranscoder>(mCallback);
+ mTranscoder = VideoTrackTranscoder::create(mCallback);
break;
case PASSTHROUGH:
mTranscoder = std::make_shared<PassthroughTrackTranscoder>(mCallback);
@@ -164,8 +164,8 @@
ASSERT_TRUE(mTranscoder->start());
drainOutputSampleQueue();
EXPECT_EQ(mCallback->waitUntilFinished(), AMEDIA_OK);
- EXPECT_TRUE(mTranscoder->stop());
joinDrainThread();
+ EXPECT_TRUE(mTranscoder->stop());
EXPECT_FALSE(mQueueWasAborted);
EXPECT_TRUE(mGotEndOfStream);
}
@@ -232,9 +232,9 @@
ASSERT_TRUE(mTranscoder->start());
drainOutputSampleQueue();
EXPECT_EQ(mCallback->waitUntilFinished(), AMEDIA_OK);
+ joinDrainThread();
EXPECT_TRUE(mTranscoder->stop());
EXPECT_FALSE(mTranscoder->start());
- joinDrainThread();
EXPECT_FALSE(mQueueWasAborted);
EXPECT_TRUE(mGotEndOfStream);
}
@@ -247,9 +247,8 @@
mTranscoderOutputQueue->abort();
drainOutputSampleQueue();
EXPECT_EQ(mCallback->waitUntilFinished(), AMEDIA_ERROR_IO);
- EXPECT_TRUE(mTranscoder->stop());
-
joinDrainThread();
+ EXPECT_TRUE(mTranscoder->stop());
EXPECT_TRUE(mQueueWasAborted);
EXPECT_FALSE(mGotEndOfStream);
}
@@ -265,8 +264,8 @@
drainOutputSampleQueue();
EXPECT_EQ(mCallback->waitUntilFinished(), AMEDIA_OK);
- EXPECT_TRUE(mTranscoder->stop());
joinDrainThread();
+ EXPECT_TRUE(mTranscoder->stop());
EXPECT_FALSE(mQueueWasAborted);
EXPECT_TRUE(mGotEndOfStream);
diff --git a/media/libmediatranscoding/transcoder/tests/VideoTrackTranscoderTests.cpp b/media/libmediatranscoding/transcoder/tests/VideoTrackTranscoderTests.cpp
index 1eb9e5a..5f2cd12 100644
--- a/media/libmediatranscoding/transcoder/tests/VideoTrackTranscoderTests.cpp
+++ b/media/libmediatranscoding/transcoder/tests/VideoTrackTranscoderTests.cpp
@@ -95,12 +95,13 @@
TEST_F(VideoTrackTranscoderTests, SampleSanity) {
LOG(DEBUG) << "Testing SampleSanity";
std::shared_ptr<TestCallback> callback = std::make_shared<TestCallback>();
- VideoTrackTranscoder transcoder{callback};
+ auto transcoder = VideoTrackTranscoder::create(callback);
- EXPECT_EQ(transcoder.configure(mMediaSampleReader, mTrackIndex, mDestinationFormat), AMEDIA_OK);
- ASSERT_TRUE(transcoder.start());
+ EXPECT_EQ(transcoder->configure(mMediaSampleReader, mTrackIndex, mDestinationFormat),
+ AMEDIA_OK);
+ ASSERT_TRUE(transcoder->start());
- std::shared_ptr<MediaSampleQueue> outputQueue = transcoder.getOutputQueue();
+ std::shared_ptr<MediaSampleQueue> outputQueue = transcoder->getOutputQueue();
std::thread sampleConsumerThread{[&outputQueue] {
uint64_t sampleCount = 0;
std::shared_ptr<MediaSample> sample;
@@ -137,7 +138,7 @@
}};
EXPECT_EQ(callback->waitUntilFinished(), AMEDIA_OK);
- EXPECT_TRUE(transcoder.stop());
+ EXPECT_TRUE(transcoder->stop());
sampleConsumerThread.join();
}
@@ -148,11 +149,70 @@
std::shared_ptr<TestCallback> callback = std::make_shared<TestCallback>();
std::shared_ptr<AMediaFormat> nullFormat;
- VideoTrackTranscoder transcoder{callback};
- EXPECT_EQ(transcoder.configure(mMediaSampleReader, 0 /* trackIndex */, nullFormat),
+ auto transcoder = VideoTrackTranscoder::create(callback);
+ EXPECT_EQ(transcoder->configure(mMediaSampleReader, 0 /* trackIndex */, nullFormat),
AMEDIA_ERROR_INVALID_PARAMETER);
}
+TEST_F(VideoTrackTranscoderTests, LingeringEncoder) {
+ struct {
+ void wait() {
+ std::unique_lock<std::mutex> lock(mMutex);
+ while (!mSignaled) {
+ mCondition.wait(lock);
+ }
+ }
+
+ void signal() {
+ std::unique_lock<std::mutex> lock(mMutex);
+ mSignaled = true;
+ mCondition.notify_all();
+ }
+
+ std::mutex mMutex;
+ std::condition_variable mCondition;
+ bool mSignaled = false;
+ } semaphore;
+
+ auto callback = std::make_shared<TestCallback>();
+ auto transcoder = VideoTrackTranscoder::create(callback);
+
+ EXPECT_EQ(transcoder->configure(mMediaSampleReader, mTrackIndex, mDestinationFormat),
+ AMEDIA_OK);
+ ASSERT_TRUE(transcoder->start());
+
+ std::shared_ptr<MediaSampleQueue> outputQueue = transcoder->getOutputQueue();
+ std::vector<std::shared_ptr<MediaSample>> samples;
+ std::thread sampleConsumerThread([&outputQueue, &samples, &semaphore] {
+ std::shared_ptr<MediaSample> sample;
+ while (samples.size() < 10 && !outputQueue->dequeue(&sample)) {
+ ASSERT_NE(sample, nullptr);
+ samples.push_back(sample);
+
+ if (sample->info.flags & SAMPLE_FLAG_END_OF_STREAM) {
+ break;
+ }
+ sample.reset();
+ }
+
+ semaphore.signal();
+ });
+
+ // Wait for the encoder to output samples before stopping and releasing the transcoder.
+ semaphore.wait();
+
+ EXPECT_TRUE(transcoder->stop());
+ transcoder.reset();
+ sampleConsumerThread.join();
+
+ // Return buffers to the codec so that it can resume processing, but keep one buffer to avoid
+ // the codec being released.
+ samples.resize(1);
+
+ // Wait for async codec events.
+ std::this_thread::sleep_for(std::chrono::seconds(1));
+}
+
} // namespace android
int main(int argc, char** argv) {