MPEG4Writer:stop & notify,MediaMuxer:remove notify
1) MPEG4Writer.cpp, Whenever there are critical errors its better
that the threads are stopped before notifying its client.
2) Recent code changes(ag/9886712) in MediaMuxer.cpp in R to receive
notifications from writer threads, MIGHT lead to deadlock in a
scenario when the client thread was waiting for the
writeSampleData to complete while holding mLock, and a
notification was sent from track threads or the main writer
thread to MediaMuxer without stopping the source and/or
stopping the writer threads. Though tough to reproduce(if at all)
it's a bad design that should be avoided to keep the code simple
and less error prone. Now that MPEG4Writer either stops
the source or stop all writer threads based on the severity of
the errors, it would be reflected in the next writeSampleData
or stop call from the client. Hence, removed the code changes
for notifying error from writer to MediaMuxer class.
3) Added logs in MediaWriter to get details of notifications with
track id.
Bug: 157600876
Test: android.media.cts.MediaMuxerTest
Test: android.mediav2.cts.MuxerTest
Test: android.media.cts.MediaRecorderTest
Test: android.mediastress.cts.MediaMuxerStressTest
Change-Id: Idb8996134010669261fa638907b497c0cf70de6b
diff --git a/media/libstagefright/MPEG4Writer.cpp b/media/libstagefright/MPEG4Writer.cpp
index b902cbc..4c4d228 100644
--- a/media/libstagefright/MPEG4Writer.cpp
+++ b/media/libstagefright/MPEG4Writer.cpp
@@ -2401,8 +2401,8 @@
int32_t err;
CHECK(msg->findInt32("errno", &err));
// Stop tracks' threads and main writer thread.
- notify(MEDIA_RECORDER_EVENT_ERROR, MEDIA_RECORDER_ERROR_UNKNOWN, err);
stop();
+ notify(MEDIA_RECORDER_EVENT_ERROR, MEDIA_RECORDER_ERROR_UNKNOWN, err);
break;
}
// fallocate() failed, hence notify app about it and stop().
@@ -2410,9 +2410,10 @@
ALOGE("kWhatFallocateError");
int32_t err;
CHECK(msg->findInt32("errno", &err));
+ // Stop tracks' threads and main writer thread.
+ stop();
//TODO: introduce a suitable MEDIA_RECORDER_ERROR_* instead MEDIA_RECORDER_ERROR_UNKNOWN?
notify(MEDIA_RECORDER_EVENT_ERROR, MEDIA_RECORDER_ERROR_UNKNOWN, err);
- stop();
break;
}
default:
diff --git a/media/libstagefright/MediaMuxer.cpp b/media/libstagefright/MediaMuxer.cpp
index 809f298..cab4ebd 100644
--- a/media/libstagefright/MediaMuxer.cpp
+++ b/media/libstagefright/MediaMuxer.cpp
@@ -48,8 +48,7 @@
MediaMuxer::MediaMuxer(int fd, OutputFormat format)
: mFormat(format),
- mState(UNINITIALIZED),
- mError(OK) {
+ mState(UNINITIALIZED) {
if (isMp4Format(format)) {
mWriter = new MPEG4Writer(fd);
} else if (format == OUTPUT_FORMAT_WEBM) {
@@ -59,7 +58,6 @@
}
if (mWriter != NULL) {
- mWriter->setMuxerListener(this);
mFileMeta = new MetaData;
if (format == OUTPUT_FORMAT_HEIF) {
// Note that the key uses recorder file types.
@@ -157,26 +155,16 @@
status_t MediaMuxer::stop() {
Mutex::Autolock autoLock(mMuxerLock);
- if (mState == STARTED || mState == ERROR) {
+ if (mState == STARTED) {
mState = STOPPED;
for (size_t i = 0; i < mTrackList.size(); i++) {
if (mTrackList[i]->stop() != OK) {
return INVALID_OPERATION;
}
}
- // Unlock this mutex to allow notify to be called during stop process.
- mMuxerLock.unlock();
status_t err = mWriter->stop();
- mMuxerLock.lock();
- if (err != OK || mError != OK) {
- ALOGE("stop err: %d, mError:%d", err, mError);
- }
- /* Prioritize mError over err as writer would have got stopped on any
- * internal error and notified muxer already. Clients might issue
- * stop again later, and mWriter->stop() would return success.
- */
- if (mError != OK) {
- err = mError;
+ if (err != OK) {
+ ALOGE("stop() err: %d", err);
}
return err;
} else {
@@ -232,29 +220,4 @@
return currentTrack->pushBuffer(mediaBuffer);
}
-void MediaMuxer::notify(int msg, int ext1, int ext2) {
- switch (msg) {
- case MEDIA_RECORDER_EVENT_ERROR:
- case MEDIA_RECORDER_TRACK_EVENT_ERROR: {
- Mutex::Autolock autoLock(mMuxerLock);
- mState = ERROR;
- mError = ext2;
- ALOGW("message received msg=%d, ext1=%d, ext2=%d", msg, ext1, ext2);
- break;
- }
- case MEDIA_RECORDER_EVENT_INFO: {
- if (ext1 == MEDIA_RECORDER_INFO_UNKNOWN) {
- Mutex::Autolock autoLock(mMuxerLock);
- mState = ERROR;
- mError = ext2;
- ALOGW("message received msg=%d, ext1=%d, ext2=%d", msg, ext1, ext2);
- }
- break;
- }
- default:
- // Ignore INFO and other notifications for now.
- break;
- }
-}
-
} // namespace android
diff --git a/media/libstagefright/include/media/stagefright/MediaMuxer.h b/media/libstagefright/include/media/stagefright/MediaMuxer.h
index 7c75f74..a1b9465 100644
--- a/media/libstagefright/include/media/stagefright/MediaMuxer.h
+++ b/media/libstagefright/include/media/stagefright/MediaMuxer.h
@@ -117,8 +117,6 @@
status_t writeSampleData(const sp<ABuffer> &buffer, size_t trackIndex,
int64_t timeUs, uint32_t flags) ;
- void notify(int msg, int ext1, int ext2);
-
private:
const OutputFormat mFormat;
sp<MediaWriter> mWriter;
@@ -130,11 +128,9 @@
UNINITIALIZED,
INITIALIZED,
STARTED,
- STOPPED,
- ERROR
+ STOPPED
};
State mState;
- status_t mError;
DISALLOW_EVIL_CONSTRUCTORS(MediaMuxer);
};
diff --git a/media/libstagefright/include/media/stagefright/MediaWriter.h b/media/libstagefright/include/media/stagefright/MediaWriter.h
index 08e54b3..1f4fbcb 100644
--- a/media/libstagefright/include/media/stagefright/MediaWriter.h
+++ b/media/libstagefright/include/media/stagefright/MediaWriter.h
@@ -21,7 +21,7 @@
#include <utils/RefBase.h>
#include <media/stagefright/MediaSource.h>
#include <media/IMediaRecorderClient.h>
-#include <media/stagefright/MediaMuxer.h>
+#include <media/mediarecorder.h>
namespace android {
@@ -46,7 +46,6 @@
virtual void setListener(const sp<IMediaRecorderClient>& listener) {
mListener = listener;
}
- virtual void setMuxerListener(const wp<MediaMuxer>& muxer) { mMuxer = muxer; }
virtual status_t dump(int /*fd*/, const Vector<String16>& /*args*/) {
return OK;
@@ -61,17 +60,20 @@
int64_t mMaxFileSizeLimitBytes;
int64_t mMaxFileDurationLimitUs;
sp<IMediaRecorderClient> mListener;
- wp<MediaMuxer> mMuxer;
void notify(int msg, int ext1, int ext2) {
- ALOG(LOG_VERBOSE, "MediaWriter", "notify msg:%d, ext1:%d, ext2:%d", msg, ext1, ext2);
+ if (msg == MEDIA_RECORDER_TRACK_EVENT_INFO || msg == MEDIA_RECORDER_TRACK_EVENT_ERROR) {
+ uint32_t trackId = (ext1 >> 28) & 0xf;
+ int type = ext1 & 0xfffffff;
+ ALOG(LOG_VERBOSE, "MediaWriter", "Track event err/info msg:%d, trackId:%u, type:%d,"
+ "val:%d", msg, trackId, type, ext2);
+ } else {
+ ALOG(LOG_VERBOSE, "MediaWriter", "Recorder event msg:%d, ext1:%d, ext2:%d",
+ msg, ext1, ext2);
+ }
if (mListener != nullptr) {
mListener->notify(msg, ext1, ext2);
}
- sp<MediaMuxer> muxer = mMuxer.promote();
- if (muxer != nullptr) {
- muxer->notify(msg, ext1, ext2);
- }
}
private:
MediaWriter(const MediaWriter &);