Transcoder: change src/dst to fd
MediaTranscoder takes fd as input from upper level service.
Also, leave file delete to client, since service side can't do file
create/delete. File opening is done by a callback into the app;
it seems delete should just handled by app side itself.
Minor refactor to reduce header pollution when included.
bug: 154734285
bug: 156003955, 152091443, 155918341
test: transcoder unit tests.
Change-Id: I4eb8a7d9fea2ccb479f09888353ac4e65bac16f8
diff --git a/media/libmediatranscoding/transcoder/MediaTrackTranscoder.cpp b/media/libmediatranscoding/transcoder/MediaTrackTranscoder.cpp
index 10c0c6c..3a281ba 100644
--- a/media/libmediatranscoding/transcoder/MediaTrackTranscoder.cpp
+++ b/media/libmediatranscoding/transcoder/MediaTrackTranscoder.cpp
@@ -19,6 +19,7 @@
#include <android-base/logging.h>
#include <media/MediaTrackTranscoder.h>
+#include <media/MediaTrackTranscoderCallback.h>
namespace android {
diff --git a/media/libmediatranscoding/transcoder/MediaTranscoder.cpp b/media/libmediatranscoding/transcoder/MediaTranscoder.cpp
index f2f7810..3db7455 100644
--- a/media/libmediatranscoding/transcoder/MediaTranscoder.cpp
+++ b/media/libmediatranscoding/transcoder/MediaTranscoder.cpp
@@ -20,6 +20,7 @@
#include <android-base/logging.h>
#include <fcntl.h>
#include <media/MediaSampleReaderNDK.h>
+#include <media/MediaSampleWriter.h>
#include <media/MediaTranscoder.h>
#include <media/PassthroughTrackTranscoder.h>
#include <media/VideoTrackTranscoder.h>
@@ -92,11 +93,10 @@
}
// Transcoding is done and the callback to the client has been sent, so tear down the
- // pipeline but do it asynchronously to avoid deadlocks. If an error occurred then
- // automatically delete the output file.
- const bool deleteOutputFile = status != AMEDIA_OK;
+ // pipeline but do it asynchronously to avoid deadlocks. If an error occurred, client
+ // should clean up the file.
std::thread asyncCancelThread{
- [self = shared_from_this(), deleteOutputFile] { self->cancel(deleteOutputFile); }};
+ [self = shared_from_this()] { self->cancel(); }};
asyncCancelThread.detach();
}
}
@@ -115,6 +115,9 @@
sendCallback(status);
}
+MediaTranscoder::MediaTranscoder(const std::shared_ptr<CallbackInterface>& callbacks)
+ : mCallbacks(callbacks) {}
+
std::shared_ptr<MediaTranscoder> MediaTranscoder::create(
const std::shared_ptr<CallbackInterface>& callbacks,
const std::shared_ptr<Parcel>& pausedState) {
@@ -129,15 +132,9 @@
return std::shared_ptr<MediaTranscoder>(new MediaTranscoder(callbacks));
}
-media_status_t MediaTranscoder::configureSource(const char* path) {
- if (path == nullptr) {
- LOG(ERROR) << "Source path cannot be null";
- return AMEDIA_ERROR_INVALID_PARAMETER;
- }
-
- const int fd = open(path, O_RDONLY);
- if (fd <= 0) {
- LOG(ERROR) << "Unable to open source path: " << path;
+media_status_t MediaTranscoder::configureSource(int fd) {
+ if (fd < 0) {
+ LOG(ERROR) << "Invalid source fd: " << fd;
return AMEDIA_ERROR_INVALID_PARAMETER;
}
@@ -145,10 +142,9 @@
lseek(fd, 0, SEEK_SET);
mSampleReader = MediaSampleReaderNDK::createFromFd(fd, 0 /* offset */, fileSize);
- close(fd);
if (mSampleReader == nullptr) {
- LOG(ERROR) << "Unable to parse source file: " << path;
+ LOG(ERROR) << "Unable to parse source fd: " << fd;
return AMEDIA_ERROR_UNSUPPORTED;
}
@@ -239,35 +235,23 @@
return AMEDIA_OK;
}
-media_status_t MediaTranscoder::configureDestination(const char* path) {
- if (path == nullptr || strlen(path) < 1) {
- LOG(ERROR) << "Invalid destination path: " << path;
+media_status_t MediaTranscoder::configureDestination(int fd) {
+ if (fd < 0) {
+ LOG(ERROR) << "Invalid destination fd: " << fd;
return AMEDIA_ERROR_INVALID_PARAMETER;
- } else if (mSampleWriter != nullptr) {
+ }
+
+ if (mSampleWriter != nullptr) {
LOG(ERROR) << "Destination is already configured.";
return AMEDIA_ERROR_INVALID_OPERATION;
}
- // Write-only, create file if non-existent, don't overwrite existing file.
- static constexpr int kOpenFlags = O_WRONLY | O_CREAT | O_EXCL;
- // User R+W permission.
- static constexpr int kFileMode = S_IRUSR | S_IWUSR;
-
- const int fd = open(path, kOpenFlags, kFileMode);
- if (fd < 0) {
- LOG(ERROR) << "Unable to open destination file \"" << path << "\" for writing: " << fd;
- return AMEDIA_ERROR_INVALID_PARAMETER;
- }
-
- mDestinationPath = std::string(path);
-
mSampleWriter = std::make_unique<MediaSampleWriter>();
const bool initOk = mSampleWriter->init(
fd, std::bind(&MediaTranscoder::onSampleWriterFinished, this, std::placeholders::_1));
- close(fd);
if (!initOk) {
- LOG(ERROR) << "Unable to initialize sample writer with destination path " << path;
+ LOG(ERROR) << "Unable to initialize sample writer with destination fd: " << fd;
mSampleWriter.reset();
return AMEDIA_ERROR_UNKNOWN;
}
@@ -305,7 +289,7 @@
started = transcoder->start();
if (!started) {
LOG(ERROR) << "Unable to start track transcoder.";
- cancel(true);
+ cancel();
return AMEDIA_ERROR_UNKNOWN;
}
}
@@ -323,7 +307,7 @@
return AMEDIA_ERROR_UNSUPPORTED;
}
-media_status_t MediaTranscoder::cancel(bool deleteDestinationFile) {
+media_status_t MediaTranscoder::cancel() {
bool expected = false;
if (!mCancelled.compare_exchange_strong(expected, true)) {
// Already cancelled.
@@ -335,15 +319,6 @@
transcoder->stop();
}
- // TODO(chz): file deletion should be done by upper level from the content URI.
- if (deleteDestinationFile && !mDestinationPath.empty()) {
- int error = unlink(mDestinationPath.c_str());
- if (error) {
- LOG(ERROR) << "Unable to delete destination file " << mDestinationPath.c_str() << ": "
- << error;
- return AMEDIA_ERROR_IO;
- }
- }
return AMEDIA_OK;
}
diff --git a/media/libmediatranscoding/transcoder/include/media/MediaTrackTranscoder.h b/media/libmediatranscoding/transcoder/include/media/MediaTrackTranscoder.h
index a71db67..0a35a57 100644
--- a/media/libmediatranscoding/transcoder/include/media/MediaTrackTranscoder.h
+++ b/media/libmediatranscoding/transcoder/include/media/MediaTrackTranscoder.h
@@ -30,28 +30,7 @@
namespace android {
-class MediaTrackTranscoder;
-
-/** Callback interface for MediaTrackTranscoder. */
-class MediaTrackTranscoderCallback {
-public:
- /**
- * Called when the MediaTrackTranscoder instance have finished transcoding all media samples
- * successfully.
- * @param transcoder The MediaTrackTranscoder that finished the transcoding.
- */
- virtual void onTrackFinished(const MediaTrackTranscoder* transcoder);
-
- /**
- * Called when the MediaTrackTranscoder instance encountered an error it could not recover from.
- * @param transcoder The MediaTrackTranscoder that encountered the error.
- * @param status The non-zero error code describing the encountered error.
- */
- virtual void onTrackError(const MediaTrackTranscoder* transcoder, media_status_t status);
-
-protected:
- virtual ~MediaTrackTranscoderCallback() = default;
-};
+class MediaTrackTranscoderCallback;
/**
* Base class for all track transcoders. MediaTrackTranscoder operates asynchronously on an internal
diff --git a/media/libmediatranscoding/transcoder/include/media/MediaTrackTranscoderCallback.h b/media/libmediatranscoding/transcoder/include/media/MediaTrackTranscoderCallback.h
new file mode 100644
index 0000000..d535ada
--- /dev/null
+++ b/media/libmediatranscoding/transcoder/include/media/MediaTrackTranscoderCallback.h
@@ -0,0 +1,48 @@
+/*
+ * Copyright (C) 2020 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef ANDROID_MEDIA_TRACK_TRANSCODER_CALLBACK_H
+#define ANDROID_MEDIA_TRACK_TRANSCODER_CALLBACK_H
+
+#include <media/NdkMediaError.h>
+
+namespace android {
+
+class MediaTrackTranscoder;
+
+/** Callback interface for MediaTrackTranscoder. */
+class MediaTrackTranscoderCallback {
+public:
+ /**
+ * Called when the MediaTrackTranscoder instance have finished transcoding all media samples
+ * successfully.
+ * @param transcoder The MediaTrackTranscoder that finished the transcoding.
+ */
+ virtual void onTrackFinished(const MediaTrackTranscoder* transcoder);
+
+ /**
+ * Called when the MediaTrackTranscoder instance encountered an error it could not recover from.
+ * @param transcoder The MediaTrackTranscoder that encountered the error.
+ * @param status The non-zero error code describing the encountered error.
+ */
+ virtual void onTrackError(const MediaTrackTranscoder* transcoder, media_status_t status);
+
+protected:
+ virtual ~MediaTrackTranscoderCallback() = default;
+};
+
+} // namespace android
+#endif // ANDROID_MEDIA_TRACK_TRANSCODER_CALLBACK_H
diff --git a/media/libmediatranscoding/transcoder/include/media/MediaTranscoder.h b/media/libmediatranscoding/transcoder/include/media/MediaTranscoder.h
index 2d18eea..72a5559 100644
--- a/media/libmediatranscoding/transcoder/include/media/MediaTranscoder.h
+++ b/media/libmediatranscoding/transcoder/include/media/MediaTranscoder.h
@@ -19,9 +19,7 @@
#include <binder/Parcel.h>
#include <binder/Parcelable.h>
-#include <media/MediaSampleReader.h>
-#include <media/MediaSampleWriter.h>
-#include <media/MediaTrackTranscoder.h>
+#include <media/MediaTrackTranscoderCallback.h>
#include <media/NdkMediaError.h>
#include <media/NdkMediaFormat.h>
@@ -31,6 +29,9 @@
namespace android {
+class MediaSampleReader;
+class MediaSampleWriter;
+
class MediaTranscoder : public std::enable_shared_from_this<MediaTranscoder>,
public MediaTrackTranscoderCallback {
public:
@@ -68,8 +69,8 @@
const std::shared_ptr<CallbackInterface>& callbacks,
const std::shared_ptr<Parcel>& pausedState = nullptr);
- /** Configures source from path. */
- media_status_t configureSource(const char* path);
+ /** Configures source from path fd. */
+ media_status_t configureSource(int fd);
/** Gets the media formats of all tracks in the file. */
std::vector<std::shared_ptr<AMediaFormat>> getTrackFormats() const;
@@ -83,8 +84,8 @@
*/
media_status_t configureTrackFormat(size_t trackIndex, AMediaFormat* trackFormat);
- /** Configures destination from path. */
- media_status_t configureDestination(const char* path);
+ /** Configures destination from fd. */
+ media_status_t configureDestination(int fd);
/** Starts transcoding. No configurations can be made once the transcoder has started. */
media_status_t start();
@@ -105,19 +106,14 @@
/** Resumes a paused transcoding. */
media_status_t resume();
- /** Cancels the transcoding. Once canceled the transcoding can not be restarted. returns error
- * if file could not be deleted. */
- media_status_t cancel(bool deleteDestinationFile = true);
+ /** Cancels the transcoding. Once canceled the transcoding can not be restarted. Client
+ * will be responsible for cleaning up the abandoned file. */
+ media_status_t cancel();
virtual ~MediaTranscoder() = default;
private:
- MediaTranscoder(const std::shared_ptr<CallbackInterface>& callbacks)
- : mCallbacks(callbacks),
- mSampleReader(nullptr),
- mSampleWriter(nullptr),
- mSourceTrackFormats(),
- mTrackTranscoders() {}
+ MediaTranscoder(const std::shared_ptr<CallbackInterface>& callbacks);
// MediaTrackTranscoderCallback
virtual void onTrackFinished(const MediaTrackTranscoder* transcoder) override;
@@ -133,7 +129,6 @@
std::vector<std::shared_ptr<AMediaFormat>> mSourceTrackFormats;
std::vector<std::unique_ptr<MediaTrackTranscoder>> mTrackTranscoders;
- std::string mDestinationPath;
std::atomic_bool mCallbackSent = false;
std::atomic_bool mCancelled = false;
};
diff --git a/media/libmediatranscoding/transcoder/tests/MediaTranscoderTests.cpp b/media/libmediatranscoding/transcoder/tests/MediaTranscoderTests.cpp
index c4a67bb..09c24b5 100644
--- a/media/libmediatranscoding/transcoder/tests/MediaTranscoderTests.cpp
+++ b/media/libmediatranscoding/transcoder/tests/MediaTranscoderTests.cpp
@@ -68,6 +68,10 @@
static const char* SOURCE_PATH =
"/data/local/tmp/TranscoderTestAssets/cubicle_avc_480x240_aac_24KHz.mp4";
+// Write-only, create file if non-existent, don't overwrite existing file.
+static constexpr int kOpenFlags = O_WRONLY | O_CREAT | O_EXCL;
+// User R+W permission.
+static constexpr int kFileMode = S_IRUSR | S_IWUSR;
class MediaTranscoderTests : public ::testing::Test {
public:
@@ -92,7 +96,8 @@
auto transcoder = MediaTranscoder::create(mCallbacks, nullptr);
EXPECT_NE(transcoder, nullptr);
- EXPECT_EQ(transcoder->configureSource(SOURCE_PATH), AMEDIA_OK);
+ const int srcFd = open(SOURCE_PATH, O_RDONLY);
+ EXPECT_EQ(transcoder->configureSource(srcFd), AMEDIA_OK);
std::vector<std::shared_ptr<AMediaFormat>> trackFormats = transcoder->getTrackFormats();
EXPECT_GT(trackFormats.size(), 0);
@@ -105,13 +110,16 @@
}
}
deleteFile(destPath);
- EXPECT_EQ(transcoder->configureDestination(destPath), AMEDIA_OK);
+ const int dstFd = open(destPath, kOpenFlags, kFileMode);
+ EXPECT_EQ(transcoder->configureDestination(dstFd), AMEDIA_OK);
media_status_t startStatus = transcoder->start();
EXPECT_EQ(startStatus, AMEDIA_OK);
if (startStatus == AMEDIA_OK) {
mCallbacks->waitForTranscodingFinished();
}
+ close(srcFd);
+ close(dstFd);
return mCallbacks->mStatus;
}
diff --git a/media/libmediatranscoding/transcoder/tests/TrackTranscoderTestUtils.h b/media/libmediatranscoding/transcoder/tests/TrackTranscoderTestUtils.h
index 79c227b..32a1a0a 100644
--- a/media/libmediatranscoding/transcoder/tests/TrackTranscoderTestUtils.h
+++ b/media/libmediatranscoding/transcoder/tests/TrackTranscoderTestUtils.h
@@ -15,6 +15,7 @@
*/
#include <media/MediaTrackTranscoder.h>
+#include <media/MediaTrackTranscoderCallback.h>
#include <condition_variable>
#include <memory>