Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit e4716f23 authored by Linus Nilsson's avatar Linus Nilsson
Browse files

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
parent daec9b4d

media/libmediatranscoding/transcoder/MediaTrackTranscoder.cpp

+1 −0
Original line number Diff line number Diff line
@@ -94,6 +94,7 @@ bool MediaTrackTranscoder::stop() { 
    if (mState == STARTED) {

        abortTranscodeLoop();

        mTranscodingThread.join();

        mOutputQueue->abort();  // Wake up any threads waiting for samples.

        mState = STOPPED;

        return true;

    }

media/libmediatranscoding/transcoder/MediaTranscoder.cpp

+4 −4
Original line number Diff line number Diff line
@@ -224,11 +224,11 @@ media_status_t MediaTranscoder::configureTrackFormat(size_t trackIndex, AMediaFo 
        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 @@ media_status_t MediaTranscoder::configureTrackFormat(size_t trackIndex, AMediaFo 
            }

        }



        transcoder = std::make_unique<VideoTrackTranscoder>(shared_from_this());

        transcoder = VideoTrackTranscoder::create(shared_from_this());



        AMediaFormat* mergedFormat =

                mergeMediaFormats(mSourceTrackFormats[trackIndex].get(), trackFormat);

media/libmediatranscoding/transcoder/VideoTrackTranscoder.cpp

+90 −42
Original line number Diff line number Diff line
@@ -59,44 +59,80 @@ T VideoTrackTranscoder::BlockingQueue<T>::pop() { 
    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);

        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;

        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.get()) {

                } 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);

        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.get()) {

            if (codec == transcoder->mEncoder->getCodec()) {

                transcoder->mCodecMessageQueue.push(

                        [transcoder, format] { transcoder->updateTrackFormat(format); });

            }

        }

    }



    static void onAsyncError(AMediaCodec* codec, void* userdata, media_status_t error,

                             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);

        VideoTrackTranscoder::CodecWrapper* wrapper =

                static_cast<VideoTrackTranscoder::CodecWrapper*>(userdata);

        if (auto transcoder = wrapper->getTranscoder()) {

            transcoder->mCodecMessageQueue.push(

                    [transcoder, error] {

                        transcoder->mStatus = error;
@@ -104,8 +140,15 @@ struct AsyncCodecCallbackDispatch { 
                    },

                    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 @@ media_status_t VideoTrackTranscoder::configureDestinationFormat( 
        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 @@ media_status_t VideoTrackTranscoder::configureDestinationFormat( 
            .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 @@ void VideoTrackTranscoder::transferBuffer(int32_t bufferIndex, AMediaCodecBuffer 


    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,11 +336,13 @@ void VideoTrackTranscoder::dequeueOutputSample(int32_t bufferIndex, 
                                               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(
@@ -309,7 +358,7 @@ void VideoTrackTranscoder::dequeueOutputSample(int32_t bufferIndex, 
            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 @@ media_status_t VideoTrackTranscoder::runTranscodeLoop() { 
    });



    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 @@ media_status_t VideoTrackTranscoder::runTranscodeLoop() { 
    }



    AMediaCodec_stop(mDecoder);

    // TODO: Stop invalidates all buffers. Stop encoder when last buffer is released.

    //    AMediaCodec_stop(mEncoder.get());

    return mStatus;

}

media/libmediatranscoding/transcoder/include/media/MediaTranscoder.h

+1 −1
Original line number Diff line number Diff line
@@ -133,7 +133,7 @@ private: 
    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);

media/libmediatranscoding/transcoder/include/media/VideoTrackTranscoder.h

+10 −5
Original line number Diff line number Diff line
@@ -34,10 +34,12 @@ namespace android { 
 * 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 @@ private: 
        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 @@ private: 
    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;