Correct MediaCodec + Surface behavior

Assorted tweaks:
- Allow signalEndOfInputStream() before ACodec is in Executing
  state (added message to two more states).
- Return an error if signalEndOfInputStream() is called a second
  time on the same stream.
- Require AndroidOpaque color format in createInputSurface().
- Disallow dequeueInputBuffer() after an input surface has been
  created (boolean flag in MediaCodec tracks it).
- Discard input surface when encoder is re-configure()ed (drop
  OMXNodeInstance's ref when we go back to Loaded).

Bug 7991062

Change-Id: Iff30f3036e14eb5a2f6536910dcf11aba33031ee

7 files changed, 74 insertions, 41 deletions

diff --git a/include/media/stagefright/ACodec.h b/include/media/stagefright/ACodec.h
index 097ec5f..96baf34 100644
--- a/include/media/stagefright/ACodec.h
+++ b/include/media/stagefright/ACodec.h
@@ -281,6 +281,9 @@ private:
     status_t requestIDRFrame();
     status_t setParameters(const sp<AMessage> &params);
 
+    // Send EOS on input stream.
+    void onSignalEndOfInputStream();
+
     DISALLOW_EVIL_CONSTRUCTORS(ACodec);
 };
 
diff --git a/include/media/stagefright/MediaCodec.h b/include/media/stagefright/MediaCodec.h
index ef695a7..35f46dc 100644
--- a/include/media/stagefright/MediaCodec.h
+++ b/include/media/stagefright/MediaCodec.h
@@ -212,6 +212,8 @@ private:
 
     sp<AMessage> mActivityNotify;
 
+    bool mHaveInputSurface;
+
     MediaCodec(const sp<ALooper> &looper);
 
     static status_t PostAndAwaitResponse(
diff --git a/media/libstagefright/ACodec.cpp b/media/libstagefright/ACodec.cpp
index 59fc45e..1a2eeb1 100644
--- a/media/libstagefright/ACodec.cpp
+++ b/media/libstagefright/ACodec.cpp
@@ -241,9 +241,6 @@ struct ACodec::ExecutingState : public ACodec::BaseState {
     // to fill with data.
     void resume();
 
-    // Send EOS on input stream.
-    void onSignalEndOfInputStream();
-
     // Returns true iff input and output buffers are in play.
     bool active() const { return mActive; }
 
@@ -3413,6 +3410,12 @@ bool ACodec::LoadedToIdleState::onMessageReceived(const sp<AMessage> &msg) {
             return true;
         }
 
+        case kWhatSignalEndOfInputStream:
+        {
+            mCodec->onSignalEndOfInputStream();
+            return true;
+        }
+
         default:
             return BaseState::onMessageReceived(msg);
     }
@@ -3458,6 +3461,12 @@ bool ACodec::IdleToExecutingState::onMessageReceived(const sp<AMessage> &msg) {
             return true;
         }
 
+        case kWhatSignalEndOfInputStream:
+        {
+            mCodec->onSignalEndOfInputStream();
+            return true;
+        }
+
         default:
             return BaseState::onMessageReceived(msg);
     }
@@ -3538,17 +3547,6 @@ void ACodec::ExecutingState::resume() {
     mActive = true;
 }
 
-void ACodec::ExecutingState::onSignalEndOfInputStream() {
-    sp<AMessage> notify = mCodec->mNotify->dup();
-    notify->setInt32("what", ACodec::kWhatSignaledInputEOS);
-
-    status_t err = mCodec->mOMX->signalEndOfInputStream(mCodec->mNode);
-    if (err != OK) {
-        notify->setInt32("err", err);
-    }
-    notify->post();
-}
-
 void ACodec::ExecutingState::stateEntered() {
     ALOGV("[%s] Now Executing", mCodec->mComponentName.c_str());
 
@@ -3640,7 +3638,7 @@ bool ACodec::ExecutingState::onMessageReceived(const sp<AMessage> &msg) {
 
         case ACodec::kWhatSignalEndOfInputStream:
         {
-            onSignalEndOfInputStream();
+            mCodec->onSignalEndOfInputStream();
             handled = true;
             break;
         }
@@ -3678,6 +3676,17 @@ status_t ACodec::setParameters(const sp<AMessage> &params) {
     return OK;
 }
 
+void ACodec::onSignalEndOfInputStream() {
+    sp<AMessage> notify = mNotify->dup();
+    notify->setInt32("what", ACodec::kWhatSignaledInputEOS);
+
+    status_t err = mOMX->signalEndOfInputStream(mNode);
+    if (err != OK) {
+        notify->setInt32("err", err);
+    }
+    notify->post();
+}
+
 bool ACodec::ExecutingState::onOMXEvent(
         OMX_EVENTTYPE event, OMX_U32 data1, OMX_U32 data2) {
     switch (event) {
diff --git a/media/libstagefright/MediaCodec.cpp b/media/libstagefright/MediaCodec.cpp
index 79ea04c..0d89c0f 100644
--- a/media/libstagefright/MediaCodec.cpp
+++ b/media/libstagefright/MediaCodec.cpp
@@ -69,7 +69,8 @@ MediaCodec::MediaCodec(const sp<ALooper> &looper)
       mDequeueInputTimeoutGeneration(0),
       mDequeueInputReplyID(0),
       mDequeueOutputTimeoutGeneration(0),
-      mDequeueOutputReplyID(0) {
+      mDequeueOutputReplyID(0),
+      mHaveInputSurface(false) {
 }
 
 MediaCodec::~MediaCodec() {
@@ -160,8 +161,6 @@ status_t MediaCodec::createInputSurface(
         sp<IGraphicBufferProducer>* bufferProducer) {
     sp<AMessage> msg = new AMessage(kWhatCreateInputSurface, id());
 
-    // TODO(fadden): require MediaFormat colorFormat == AndroidOpaque
-
     sp<AMessage> response;
     status_t err = PostAndAwaitResponse(msg, &response);
     if (err == NO_ERROR) {
@@ -256,8 +255,6 @@ status_t MediaCodec::queueSecureInputBuffer(
 }
 
 status_t MediaCodec::dequeueInputBuffer(size_t *index, int64_t timeoutUs) {
-    // TODO(fadden): fail if an input Surface has been configured
-
     sp<AMessage> msg = new AMessage(kWhatDequeueInputBuffer, id());
     msg->setInt64("timeoutUs", timeoutUs);
 
@@ -604,6 +601,9 @@ void MediaCodec::onMessageReceived(const sp<AMessage> &msg) {
                     CHECK_EQ(mState, CONFIGURING);
                     setState(CONFIGURED);
 
+                    // reset input surface flag
+                    mHaveInputSurface = false;
+
                     (new AMessage)->postReply(mReplyID);
                     break;
                 }
@@ -618,6 +618,7 @@ void MediaCodec::onMessageReceived(const sp<AMessage> &msg) {
                         msg->findObject("input-surface", &obj);
                         CHECK(obj != NULL);
                         response->setObject("input-surface", obj);
+                        mHaveInputSurface = true;
                     } else {
                         response->setInt32("err", err);
                     }
@@ -1029,10 +1030,17 @@ void MediaCodec::onMessageReceived(const sp<AMessage> &msg) {
 
         case kWhatDequeueInputBuffer:
         {
-            // TODO(fadden): make this fail if we're using an input Surface
             uint32_t replyID;
             CHECK(msg->senderAwaitsResponse(&replyID));
 
+            if (mHaveInputSurface) {
+                ALOGE("dequeueInputBuffer can't be used with input surface");
+                sp<AMessage> response = new AMessage;
+                response->setInt32("err", INVALID_OPERATION);
+                response->postReply(replyID);
+                break;
+            }
+
             if (handleDequeueInputBuffer(replyID, true /* new request */)) {
                 break;
             }
diff --git a/media/libstagefright/omx/GraphicBufferSource.cpp b/media/libstagefright/omx/GraphicBufferSource.cpp
index f207954..211e1d1 100644
--- a/media/libstagefright/omx/GraphicBufferSource.cpp
+++ b/media/libstagefright/omx/GraphicBufferSource.cpp
@@ -15,6 +15,7 @@
  */
 
 #define LOG_TAG "GraphicBufferSource"
+//#define LOG_NDEBUG 0
 #include <utils/Log.h>
 
 #include <GraphicBufferSource.h>
@@ -110,15 +111,12 @@ void GraphicBufferSource::omxExecuting() {
     }
 }
 
-void GraphicBufferSource::omxIdling(){
+void GraphicBufferSource::omxLoaded(){
     Mutex::Autolock autoLock(mMutex);
-    ALOGV("--> idling");
-    if (!mExecuting) {
-        // Transition from "loading" to "idling".  Nothing to do.
-        return;
-    }
+    ALOGV("--> loaded");
+    CHECK(mExecuting);
 
-    ALOGV("Dropped down to idle, avail=%d eos=%d eosSent=%d",
+    ALOGV("Dropped down to loaded, avail=%d eos=%d eosSent=%d",
             mNumFramesAvailable, mEndOfStream, mEndOfStreamSent);
 
     // Codec is no longer executing.  Discard all codec-related state.
@@ -282,10 +280,15 @@ status_t GraphicBufferSource::fillCodecBuffer_l() {
     return OK;
 }
 
-void GraphicBufferSource::signalEndOfInputStream() {
+status_t GraphicBufferSource::signalEndOfInputStream() {
     Mutex::Autolock autoLock(mMutex);
-    ALOGV("signalEndOfInputStream: exec=%d avail=%d",
-            mExecuting, mNumFramesAvailable);
+    ALOGV("signalEndOfInputStream: exec=%d avail=%d eos=%d",
+            mExecuting, mNumFramesAvailable, mEndOfStream);
+
+    if (mEndOfStream) {
+        ALOGE("EOS was already signaled");
+        return INVALID_OPERATION;
+    }
 
     // Set the end-of-stream flag.  If no frames are pending from the
     // BufferQueue, and a codec buffer is available, and we're executing,
@@ -300,6 +303,8 @@ void GraphicBufferSource::signalEndOfInputStream() {
     if (mExecuting && mNumFramesAvailable == 0) {
         submitEndOfInputStream_l();
     }
+
+    return OK;
 }
 
 status_t GraphicBufferSource::submitBuffer_l(sp<GraphicBuffer>& graphicBuffer,
diff --git a/media/libstagefright/omx/GraphicBufferSource.h b/media/libstagefright/omx/GraphicBufferSource.h
index 6d49f96..6a34bc5 100644
--- a/media/libstagefright/omx/GraphicBufferSource.h
+++ b/media/libstagefright/omx/GraphicBufferSource.h
@@ -67,10 +67,9 @@ public:
     // sitting in the BufferQueue, this will send them to the codec.
     void omxExecuting();
 
-    // This is called when OMX transitions to OMX_StateIdle.  If we were
-    // previously executing, this means we're about to be shut down.  (We
-    // also enter Idle on the way up.)
-    void omxIdling();
+    // This is called when OMX transitions to OMX_StateLoaded, indicating that
+    // we are shutting down.
+    void omxLoaded();
 
     // A "codec buffer", i.e. a buffer that can be used to pass data into
     // the encoder, has been allocated.  (This call does not call back into
@@ -84,7 +83,7 @@ public:
     // This is called after the last input frame has been submitted.  We
     // need to submit an empty buffer with the EOS flag set.  If we don't
     // have a codec buffer ready, we just set the mEndOfStream flag.
-    void signalEndOfInputStream();
+    status_t signalEndOfInputStream();
 
 protected:
     // BufferQueue::ConsumerListener interface, called when a new frame of
diff --git a/media/libstagefright/omx/OMXNodeInstance.cpp b/media/libstagefright/omx/OMXNodeInstance.cpp
index 6c2c33b..f3d8d14 100644
--- a/media/libstagefright/omx/OMXNodeInstance.cpp
+++ b/media/libstagefright/omx/OMXNodeInstance.cpp
@@ -584,6 +584,11 @@ status_t OMXNodeInstance::createInputSurface(
             mHandle, OMX_IndexParamPortDefinition, &def);
     CHECK(oerr == OMX_ErrorNone);
 
+    if (def.format.video.eColorFormat != OMX_COLOR_FormatAndroidOpaque) {
+        ALOGE("createInputSurface requires AndroidOpaque color format");
+        return INVALID_OPERATION;
+    }
+
     GraphicBufferSource* bufferSource = new GraphicBufferSource(
             this, def.format.video.nFrameWidth, def.format.video.nFrameHeight);
     if ((err = bufferSource->initCheck()) != OK) {
@@ -602,11 +607,10 @@ status_t OMXNodeInstance::signalEndOfInputStream() {
     // flag set).  Seems easier than doing the equivalent from here.
     sp<GraphicBufferSource> bufferSource(getGraphicBufferSource());
     if (bufferSource == NULL) {
-        ALOGW("signalEndOfInputStream should only be used with Surface input");
+        ALOGW("signalEndOfInputStream can only be used with Surface input");
         return INVALID_OPERATION;
     };
-    bufferSource->signalEndOfInputStream();
-    return OK;
+    return bufferSource->signalEndOfInputStream();
 }
 
 status_t OMXNodeInstance::allocateBuffer(
@@ -801,8 +805,11 @@ void OMXNodeInstance::onEvent(
             arg1 == OMX_CommandStateSet) {
         if (arg2 == OMX_StateExecuting) {
             bufferSource->omxExecuting();
-        } else if (arg2 == OMX_StateIdle) {
-            bufferSource->omxIdling();
+        } else if (arg2 == OMX_StateLoaded) {
+            // Must be shutting down -- won't have a GraphicBufferSource
+            // on the way up.
+            bufferSource->omxLoaded();
+            setGraphicBufferSource(NULL);
         }
     }
 }