transcoding: remove realtime jobs when a client is removed

Also disallow usage of the client after unregister.

bug: 154734285
test: unit testing
Change-Id: Ib5d54a897c7e56d42d27645fa55ab6f21f435b5e
diff --git a/media/libmediatranscoding/TranscodingClientManager.cpp b/media/libmediatranscoding/TranscodingClientManager.cpp
index 1060791..de9dd76 100644
--- a/media/libmediatranscoding/TranscodingClientManager.cpp
+++ b/media/libmediatranscoding/TranscodingClientManager.cpp
@@ -18,15 +18,18 @@
 #define LOG_TAG "TranscodingClientManager"
 
 #include <aidl/android/media/BnTranscodingClient.h>
+#include <aidl/android/media/IMediaTranscodingService.h>
 #include <android/binder_ibinder.h>
 #include <inttypes.h>
 #include <media/TranscodingClientManager.h>
 #include <media/TranscodingRequest.h>
 #include <utils/Log.h>
-
 namespace android {
 
+static_assert(sizeof(ClientIdType) == sizeof(void*), "ClientIdType should be pointer-sized");
+
 using ::aidl::android::media::BnTranscodingClient;
+using ::aidl::android::media::IMediaTranscodingService;  // For service error codes
 using ::aidl::android::media::TranscodingJobParcel;
 using ::aidl::android::media::TranscodingRequestParcel;
 using Status = ::ndk::ScopedAStatus;
@@ -62,14 +65,16 @@
     std::string mClientName;
     std::string mClientOpPackageName;
 
-    // Next jobId to assign
+    // Next jobId to assign.
     std::atomic<int32_t> mNextJobId;
-    // Pointer to the client manager for this client
-    TranscodingClientManager* mOwner;
+    // Whether this client has been unregistered already.
+    std::atomic<bool> mAbandoned;
+    // Weak pointer to the client manager for this client.
+    std::weak_ptr<TranscodingClientManager> mOwner;
 
     ClientImpl(const std::shared_ptr<ITranscodingClientCallback>& callback, pid_t pid, uid_t uid,
                const std::string& clientName, const std::string& opPackageName,
-               TranscodingClientManager* owner);
+               const std::weak_ptr<TranscodingClientManager>& owner);
 
     Status submitRequest(const TranscodingRequestParcel& /*in_request*/,
                          TranscodingJobParcel* /*out_job*/, bool* /*_aidl_return*/) override;
@@ -85,7 +90,7 @@
 TranscodingClientManager::ClientImpl::ClientImpl(
         const std::shared_ptr<ITranscodingClientCallback>& callback, pid_t pid, uid_t uid,
         const std::string& clientName, const std::string& opPackageName,
-        TranscodingClientManager* owner)
+        const std::weak_ptr<TranscodingClientManager>& owner)
       : mClientBinder((callback != nullptr) ? callback->asBinder() : nullptr),
         mClientCallback(callback),
         mClientId(sCookieCounter.fetch_add(1, std::memory_order_relaxed)),
@@ -94,21 +99,28 @@
         mClientName(clientName),
         mClientOpPackageName(opPackageName),
         mNextJobId(0),
+        mAbandoned(false),
         mOwner(owner) {}
 
 Status TranscodingClientManager::ClientImpl::submitRequest(
         const TranscodingRequestParcel& in_request, TranscodingJobParcel* out_job,
         bool* _aidl_return) {
+    *_aidl_return = false;
+
+    std::shared_ptr<TranscodingClientManager> owner;
+    if (mAbandoned || (owner = mOwner.lock()) == nullptr) {
+        return Status::fromServiceSpecificError(IMediaTranscodingService::ERROR_DISCONNECTED);
+    }
+
     if (in_request.fileName.empty()) {
         // This is the only error we check for now.
-        *_aidl_return = false;
         return Status::ok();
     }
 
     int32_t jobId = mNextJobId.fetch_add(1);
 
-    *_aidl_return = mOwner->mJobScheduler->submit(mClientId, jobId, mClientUid, in_request,
-                                                  mClientCallback);
+    *_aidl_return =
+            owner->mJobScheduler->submit(mClientId, jobId, mClientUid, in_request, mClientCallback);
 
     if (*_aidl_return) {
         out_job->jobId = jobId;
@@ -117,18 +129,41 @@
         *(TranscodingRequest*)&out_job->request = in_request;
         out_job->awaitNumberOfJobs = 0;
     }
+
     return Status::ok();
 }
 
 Status TranscodingClientManager::ClientImpl::cancelJob(int32_t in_jobId, bool* _aidl_return) {
-    *_aidl_return = mOwner->mJobScheduler->cancel(mClientId, in_jobId);
+    *_aidl_return = false;
+
+    std::shared_ptr<TranscodingClientManager> owner;
+    if (mAbandoned || (owner = mOwner.lock()) == nullptr) {
+        return Status::fromServiceSpecificError(IMediaTranscodingService::ERROR_DISCONNECTED);
+    }
+
+    if (in_jobId < 0) {
+        return Status::ok();
+    }
+
+    *_aidl_return = owner->mJobScheduler->cancel(mClientId, in_jobId);
     return Status::ok();
 }
 
 Status TranscodingClientManager::ClientImpl::getJobWithId(int32_t in_jobId,
                                                           TranscodingJobParcel* out_job,
                                                           bool* _aidl_return) {
-    *_aidl_return = mOwner->mJobScheduler->getJob(mClientId, in_jobId, &out_job->request);
+    *_aidl_return = false;
+
+    std::shared_ptr<TranscodingClientManager> owner;
+    if (mAbandoned || (owner = mOwner.lock()) == nullptr) {
+        return Status::fromServiceSpecificError(IMediaTranscodingService::ERROR_DISCONNECTED);
+    }
+
+    if (in_jobId < 0) {
+        return Status::ok();
+    }
+
+    *_aidl_return = owner->mJobScheduler->getJob(mClientId, in_jobId, &out_job->request);
 
     if (*_aidl_return) {
         out_job->jobId = in_jobId;
@@ -138,10 +173,17 @@
 }
 
 Status TranscodingClientManager::ClientImpl::unregister() {
-    // TODO(chz): Decide what to do about this client's jobs.
-    // If app crashed, it could be relaunched later. Do we want to keep the
-    // jobs around for that?
-    mOwner->removeClient(mClientId);
+    bool abandoned = mAbandoned.exchange(true);
+
+    std::shared_ptr<TranscodingClientManager> owner;
+    if (abandoned || (owner = mOwner.lock()) == nullptr) {
+        return Status::fromServiceSpecificError(IMediaTranscodingService::ERROR_DISCONNECTED);
+    }
+
+    // Use jobId == -1 to cancel all realtime jobs for this client with the scheduler.
+    owner->mJobScheduler->cancel(mClientId, -1);
+    owner->removeClient(mClientId);
+
     return Status::ok();
 }
 
@@ -210,7 +252,7 @@
     // Validate the client.
     if (callback == nullptr || pid < 0 || clientName.empty() || opPackageName.empty()) {
         ALOGE("Invalid client");
-        return BAD_VALUE;
+        return IMediaTranscodingService::ERROR_ILLEGAL_ARGUMENT;
     }
 
     SpAIBinder binder = callback->asBinder();
@@ -219,12 +261,12 @@
 
     // Checks if the client already registers.
     if (mRegisteredCallbacks.count((uintptr_t)binder.get()) > 0) {
-        return ALREADY_EXISTS;
+        return IMediaTranscodingService::ERROR_ALREADY_EXISTS;
     }
 
     // Creates the client and uses its process id as client id.
     std::shared_ptr<ClientImpl> client = ::ndk::SharedRefBase::make<ClientImpl>(
-            callback, pid, uid, clientName, opPackageName, this);
+            callback, pid, uid, clientName, opPackageName, shared_from_this());
 
     ALOGD("Adding client id %lld, pid %d, uid %d, name %s, package %s",
           (long long)client->mClientId, client->mClientPid, client->mClientUid,
@@ -255,7 +297,7 @@
     auto it = mClientIdToClientMap.find(clientId);
     if (it == mClientIdToClientMap.end()) {
         ALOGE("Client id %lld does not exist", (long long)clientId);
-        return INVALID_OPERATION;
+        return IMediaTranscodingService::ERROR_INVALID_OPERATION;
     }
 
     SpAIBinder binder = it->second->mClientBinder;
diff --git a/media/libmediatranscoding/TranscodingJobScheduler.cpp b/media/libmediatranscoding/TranscodingJobScheduler.cpp
index cf9d3c8..1b36d4c 100644
--- a/media/libmediatranscoding/TranscodingJobScheduler.cpp
+++ b/media/libmediatranscoding/TranscodingJobScheduler.cpp
@@ -27,6 +27,8 @@
 
 namespace android {
 
+static_assert((JobIdType)-1 < 0, "JobIdType should be signed");
+
 constexpr static uid_t OFFLINE_UID = -1;
 
 //static
@@ -236,19 +238,33 @@
 
     ALOGV("%s: job %s", __FUNCTION__, jobToString(jobKey).c_str());
 
+    std::list<JobKeyType> jobsToRemove;
+
     std::scoped_lock lock{mLock};
 
-    if (mJobMap.count(jobKey) == 0) {
-        ALOGE("job %s doesn't exist", jobToString(jobKey).c_str());
-        return false;
-    }
-    // If the job is running, pause it first.
-    if (mJobMap[jobKey].state == Job::RUNNING) {
-        mTranscoder->pause(clientId, jobId);
+    if (jobId < 0) {
+        for (auto it = mJobMap.begin(); it != mJobMap.end(); ++it) {
+            if (it->first.first == clientId && it->second.uid != OFFLINE_UID) {
+                jobsToRemove.push_back(it->first);
+            }
+        }
+    } else {
+        if (mJobMap.count(jobKey) == 0) {
+            ALOGE("job %s doesn't exist", jobToString(jobKey).c_str());
+            return false;
+        }
+        jobsToRemove.push_back(jobKey);
     }
 
-    // Remove the job.
-    removeJob_l(jobKey);
+    for (auto it = jobsToRemove.begin(); it != jobsToRemove.end(); ++it) {
+        // If the job is running, pause it first.
+        if (mJobMap[*it].state == Job::RUNNING) {
+            mTranscoder->pause(clientId, jobId);
+        }
+
+        // Remove the job.
+        removeJob_l(*it);
+    }
 
     // Start next job.
     updateCurrentJob_l();
diff --git a/media/libmediatranscoding/include/media/SchedulerClientInterface.h b/media/libmediatranscoding/include/media/SchedulerClientInterface.h
index dc7648c..e00cfb2 100644
--- a/media/libmediatranscoding/include/media/SchedulerClientInterface.h
+++ b/media/libmediatranscoding/include/media/SchedulerClientInterface.h
@@ -30,12 +30,33 @@
 // the status of a job.
 class SchedulerClientInterface {
 public:
+    /**
+     * Submits one request to the scheduler.
+     *
+     * Returns true on success and false on failure. This call will fail is a job identified
+     * by <clientId, jobId> already exists.
+     */
     virtual bool submit(ClientIdType clientId, JobIdType jobId, uid_t uid,
                         const TranscodingRequestParcel& request,
                         const std::weak_ptr<ITranscodingClientCallback>& clientCallback) = 0;
 
+    /**
+     * Cancels a job identified by <clientId, jobId>.
+     *
+     * If jobId is negative (<0), all jobs with a specified priority (that's not
+     * TranscodingJobPriority::kUnspecified) will be cancelled. Otherwise, only the single job
+     * <clientId, jobId> will be cancelled.
+     *
+     * Returns false if a single job is being cancelled but it doesn't exist. Returns
+     * true otherwise.
+     */
     virtual bool cancel(ClientIdType clientId, JobIdType jobId) = 0;
 
+    /**
+     * Retrieves information about a job.
+     *
+     * Returns true and the job if it exists, and false otherwise.
+     */
     virtual bool getJob(ClientIdType clientId, JobIdType jobId,
                         TranscodingRequestParcel* request) = 0;
 
diff --git a/media/libmediatranscoding/include/media/TranscodingClientManager.h b/media/libmediatranscoding/include/media/TranscodingClientManager.h
index 00b2e7f..a62ad8c 100644
--- a/media/libmediatranscoding/include/media/TranscodingClientManager.h
+++ b/media/libmediatranscoding/include/media/TranscodingClientManager.h
@@ -46,7 +46,7 @@
  * TODO(hkuang): Hook up with ResourceManager for resource management.
  * TODO(hkuang): Hook up with MediaMetrics to log all the transactions.
  */
-class TranscodingClientManager {
+class TranscodingClientManager : public std::enable_shared_from_this<TranscodingClientManager> {
 public:
     virtual ~TranscodingClientManager();
 
diff --git a/media/libmediatranscoding/tests/TranscodingClientManager_tests.cpp b/media/libmediatranscoding/tests/TranscodingClientManager_tests.cpp
index 6204f47..d9504ca 100644
--- a/media/libmediatranscoding/tests/TranscodingClientManager_tests.cpp
+++ b/media/libmediatranscoding/tests/TranscodingClientManager_tests.cpp
@@ -39,6 +39,7 @@
 using ::aidl::android::media::IMediaTranscodingService;
 using ::aidl::android::media::TranscodingErrorCode;
 using ::aidl::android::media::TranscodingJobParcel;
+using ::aidl::android::media::TranscodingJobPriority;
 using ::aidl::android::media::TranscodingRequestParcel;
 using ::aidl::android::media::TranscodingResultParcel;
 
@@ -51,6 +52,8 @@
 constexpr const char* kClientName = "TestClientName";
 constexpr const char* kClientPackage = "TestClientPackage";
 
+#define JOB(n) (n)
+
 struct TestClientCallback : public BnTranscodingClientCallback {
     TestClientCallback() { ALOGI("TestClientCallback Created"); }
 
@@ -261,7 +264,7 @@
     std::shared_ptr<ITranscodingClient> client;
     status_t err = mClientManager->addClient(nullptr, kClientPid, kClientUid, kClientName,
                                              kClientPackage, &client);
-    EXPECT_EQ(err, BAD_VALUE);
+    EXPECT_EQ(err, IMediaTranscodingService::ERROR_ILLEGAL_ARGUMENT);
 }
 
 TEST_F(TranscodingClientManagerTest, TestAddingWithInvalidClientPid) {
@@ -269,7 +272,7 @@
     std::shared_ptr<ITranscodingClient> client;
     status_t err = mClientManager->addClient(mClientCallback1, kInvalidClientPid, kClientUid,
                                              kClientName, kClientPackage, &client);
-    EXPECT_EQ(err, BAD_VALUE);
+    EXPECT_EQ(err, IMediaTranscodingService::ERROR_ILLEGAL_ARGUMENT);
 }
 
 TEST_F(TranscodingClientManagerTest, TestAddingWithInvalidClientName) {
@@ -277,7 +280,7 @@
     std::shared_ptr<ITranscodingClient> client;
     status_t err = mClientManager->addClient(mClientCallback1, kClientPid, kClientUid,
                                              kInvalidClientName, kClientPackage, &client);
-    EXPECT_EQ(err, BAD_VALUE);
+    EXPECT_EQ(err, IMediaTranscodingService::ERROR_ILLEGAL_ARGUMENT);
 }
 
 TEST_F(TranscodingClientManagerTest, TestAddingWithInvalidClientPackageName) {
@@ -285,7 +288,7 @@
     std::shared_ptr<ITranscodingClient> client;
     status_t err = mClientManager->addClient(mClientCallback1, kClientPid, kClientUid, kClientName,
                                              kInvalidClientPackage, &client);
-    EXPECT_EQ(err, BAD_VALUE);
+    EXPECT_EQ(err, IMediaTranscodingService::ERROR_ILLEGAL_ARGUMENT);
 }
 
 TEST_F(TranscodingClientManagerTest, TestAddingValidClient) {
@@ -314,7 +317,7 @@
     std::shared_ptr<ITranscodingClient> dupClient;
     err = mClientManager->addClient(mClientCallback1, kClientPid, kClientUid, "dupClient",
                                     "dupPackage", &dupClient);
-    EXPECT_EQ(err, ALREADY_EXISTS);
+    EXPECT_EQ(err, IMediaTranscodingService::ERROR_ALREADY_EXISTS);
     EXPECT_EQ(dupClient.get(), nullptr);
     EXPECT_EQ(mClientManager->getNumOfClients(), 1);
 
@@ -348,17 +351,17 @@
     bool result;
     EXPECT_TRUE(mClient1->submitRequest(request, &job, &result).isOk());
     EXPECT_TRUE(result);
-    EXPECT_EQ(job.jobId, 0);
+    EXPECT_EQ(job.jobId, JOB(0));
 
     request.fileName = "test_file_1";
     EXPECT_TRUE(mClient1->submitRequest(request, &job, &result).isOk());
     EXPECT_TRUE(result);
-    EXPECT_EQ(job.jobId, 1);
+    EXPECT_EQ(job.jobId, JOB(1));
 
     request.fileName = "test_file_2";
     EXPECT_TRUE(mClient1->submitRequest(request, &job, &result).isOk());
     EXPECT_TRUE(result);
-    EXPECT_EQ(job.jobId, 2);
+    EXPECT_EQ(job.jobId, JOB(2));
 
     // Test submit bad request (no valid fileName) fails.
     TranscodingRequestParcel badRequest;
@@ -367,45 +370,45 @@
     EXPECT_FALSE(result);
 
     // Test get jobs by id.
-    EXPECT_TRUE(mClient1->getJobWithId(2, &job, &result).isOk());
-    EXPECT_EQ(job.jobId, 2);
+    EXPECT_TRUE(mClient1->getJobWithId(JOB(2), &job, &result).isOk());
+    EXPECT_EQ(job.jobId, JOB(2));
     EXPECT_EQ(job.request.fileName, "test_file_2");
     EXPECT_TRUE(result);
 
     // Test get jobs by invalid id fails.
-    EXPECT_TRUE(mClient1->getJobWithId(100, &job, &result).isOk());
+    EXPECT_TRUE(mClient1->getJobWithId(JOB(100), &job, &result).isOk());
     EXPECT_FALSE(result);
 
     // Test cancel non-existent job fail.
-    EXPECT_TRUE(mClient2->cancelJob(100, &result).isOk());
+    EXPECT_TRUE(mClient2->cancelJob(JOB(100), &result).isOk());
     EXPECT_FALSE(result);
 
     // Test cancel valid jobId in arbitrary order.
-    EXPECT_TRUE(mClient1->cancelJob(2, &result).isOk());
+    EXPECT_TRUE(mClient1->cancelJob(JOB(2), &result).isOk());
     EXPECT_TRUE(result);
 
-    EXPECT_TRUE(mClient1->cancelJob(0, &result).isOk());
+    EXPECT_TRUE(mClient1->cancelJob(JOB(0), &result).isOk());
     EXPECT_TRUE(result);
 
-    EXPECT_TRUE(mClient1->cancelJob(1, &result).isOk());
+    EXPECT_TRUE(mClient1->cancelJob(JOB(1), &result).isOk());
     EXPECT_TRUE(result);
 
     // Test cancel job again fails.
-    EXPECT_TRUE(mClient1->cancelJob(1, &result).isOk());
+    EXPECT_TRUE(mClient1->cancelJob(JOB(1), &result).isOk());
     EXPECT_FALSE(result);
 
     // Test get job after cancel fails.
-    EXPECT_TRUE(mClient1->getJobWithId(2, &job, &result).isOk());
+    EXPECT_TRUE(mClient1->getJobWithId(JOB(2), &job, &result).isOk());
     EXPECT_FALSE(result);
 
     // Test jobId independence for each client.
     EXPECT_TRUE(mClient2->submitRequest(request, &job, &result).isOk());
     EXPECT_TRUE(result);
-    EXPECT_EQ(job.jobId, 0);
+    EXPECT_EQ(job.jobId, JOB(0));
 
     EXPECT_TRUE(mClient2->submitRequest(request, &job, &result).isOk());
     EXPECT_TRUE(result);
-    EXPECT_EQ(job.jobId, 1);
+    EXPECT_EQ(job.jobId, JOB(1));
 
     unregisterMultipleClients();
 }
@@ -419,25 +422,25 @@
     bool result;
     EXPECT_TRUE(mClient1->submitRequest(request, &job, &result).isOk());
     EXPECT_TRUE(result);
-    EXPECT_EQ(job.jobId, 0);
+    EXPECT_EQ(job.jobId, JOB(0));
 
     mScheduler->finishLastJob();
     EXPECT_EQ(mClientCallback1->popEvent(), TestClientCallback::Finished(job.jobId));
 
     EXPECT_TRUE(mClient1->submitRequest(request, &job, &result).isOk());
     EXPECT_TRUE(result);
-    EXPECT_EQ(job.jobId, 1);
+    EXPECT_EQ(job.jobId, JOB(1));
 
     mScheduler->abortLastJob();
     EXPECT_EQ(mClientCallback1->popEvent(), TestClientCallback::Failed(job.jobId));
 
     EXPECT_TRUE(mClient1->submitRequest(request, &job, &result).isOk());
     EXPECT_TRUE(result);
-    EXPECT_EQ(job.jobId, 2);
+    EXPECT_EQ(job.jobId, JOB(2));
 
     EXPECT_TRUE(mClient2->submitRequest(request, &job, &result).isOk());
     EXPECT_TRUE(result);
-    EXPECT_EQ(job.jobId, 0);
+    EXPECT_EQ(job.jobId, JOB(0));
 
     mScheduler->finishLastJob();
     EXPECT_EQ(mClientCallback2->popEvent(), TestClientCallback::Finished(job.jobId));
@@ -445,4 +448,67 @@
     unregisterMultipleClients();
 }
 
+TEST_F(TranscodingClientManagerTest, TestUseAfterUnregister) {
+    // Add a client.
+    std::shared_ptr<ITranscodingClient> client;
+    status_t err = mClientManager->addClient(mClientCallback1, kClientPid, kClientUid, kClientName,
+                                             kClientPackage, &client);
+    EXPECT_EQ(err, OK);
+    EXPECT_NE(client.get(), nullptr);
+
+    // Submit 2 requests, 1 offline and 1 realtime.
+    TranscodingRequestParcel request;
+    TranscodingJobParcel job;
+    bool result;
+
+    request.fileName = "test_file_0";
+    request.priority = TranscodingJobPriority::kUnspecified;
+    EXPECT_TRUE(client->submitRequest(request, &job, &result).isOk() && result);
+    EXPECT_EQ(job.jobId, JOB(0));
+
+    request.fileName = "test_file_1";
+    request.priority = TranscodingJobPriority::kNormal;
+    EXPECT_TRUE(client->submitRequest(request, &job, &result).isOk() && result);
+    EXPECT_EQ(job.jobId, JOB(1));
+
+    // Unregister client, should succeed.
+    Status status = client->unregister();
+    EXPECT_TRUE(status.isOk());
+
+    // Test submit new request after unregister, should fail with ERROR_DISCONNECTED.
+    request.fileName = "test_file_2";
+    request.priority = TranscodingJobPriority::kNormal;
+    status = client->submitRequest(request, &job, &result);
+    EXPECT_FALSE(status.isOk());
+    EXPECT_EQ(status.getServiceSpecificError(), IMediaTranscodingService::ERROR_DISCONNECTED);
+
+    // Test cancel jobs after unregister, should fail with ERROR_DISCONNECTED
+    // regardless of realtime or offline job, or whether the jobId is valid.
+    status = client->cancelJob(JOB(0), &result);
+    EXPECT_FALSE(status.isOk());
+    EXPECT_EQ(status.getServiceSpecificError(), IMediaTranscodingService::ERROR_DISCONNECTED);
+
+    status = client->cancelJob(JOB(1), &result);
+    EXPECT_FALSE(status.isOk());
+    EXPECT_EQ(status.getServiceSpecificError(), IMediaTranscodingService::ERROR_DISCONNECTED);
+
+    status = client->cancelJob(JOB(2), &result);
+    EXPECT_FALSE(status.isOk());
+    EXPECT_EQ(status.getServiceSpecificError(), IMediaTranscodingService::ERROR_DISCONNECTED);
+
+    // Test get jobs, should fail with ERROR_DISCONNECTED regardless of realtime
+    // or offline job, or whether the jobId is valid.
+    status = client->getJobWithId(JOB(0), &job, &result);
+    EXPECT_FALSE(status.isOk());
+    EXPECT_EQ(status.getServiceSpecificError(), IMediaTranscodingService::ERROR_DISCONNECTED);
+
+    status = client->getJobWithId(JOB(1), &job, &result);
+    EXPECT_FALSE(status.isOk());
+    EXPECT_EQ(status.getServiceSpecificError(), IMediaTranscodingService::ERROR_DISCONNECTED);
+
+    status = client->getJobWithId(JOB(2), &job, &result);
+    EXPECT_FALSE(status.isOk());
+    EXPECT_EQ(status.getServiceSpecificError(), IMediaTranscodingService::ERROR_DISCONNECTED);
+}
+
 }  // namespace android
diff --git a/services/mediatranscoding/MediaTranscodingService.cpp b/services/mediatranscoding/MediaTranscodingService.cpp
index 9e62b97..b843967 100644
--- a/services/mediatranscoding/MediaTranscodingService.cpp
+++ b/services/mediatranscoding/MediaTranscodingService.cpp
@@ -100,9 +100,8 @@
         const std::string& in_clientName, const std::string& in_opPackageName, int32_t in_clientUid,
         int32_t in_clientPid, std::shared_ptr<ITranscodingClient>* _aidl_return) {
     if (in_callback == nullptr) {
-        ALOGE("Client callback can not be null");
         *_aidl_return = nullptr;
-        return Status::fromServiceSpecificError(ERROR_ILLEGAL_ARGUMENT);
+        return STATUS_ERROR_FMT(ERROR_ILLEGAL_ARGUMENT, "Client callback cannot be null!");
     }
 
     int32_t callingPid = AIBinder_getCallingPid();
diff --git a/services/mediatranscoding/tests/mediatranscodingservice_tests.cpp b/services/mediatranscoding/tests/mediatranscodingservice_tests.cpp
index b20ce09..a8d4241 100644
--- a/services/mediatranscoding/tests/mediatranscodingservice_tests.cpp
+++ b/services/mediatranscoding/tests/mediatranscodingservice_tests.cpp
@@ -611,6 +611,31 @@
     unregisterMultipleClients();
 }
 
+TEST_F(MediaTranscodingServiceTest, TestClientUseAfterUnregister) {
+    std::shared_ptr<ITranscodingClient> client;
+
+    // Register a client, then unregister.
+    Status status = mService->registerClient(mClientCallback1, kClientName, kClientOpPackageName,
+                                             kClientUseCallingUid, kClientUseCallingPid, &client);
+    EXPECT_TRUE(status.isOk());
+
+    status = client->unregister();
+    EXPECT_TRUE(status.isOk());
+
+    // Test various operations on the client, should fail with ERROR_DISCONNECTED.
+    TranscodingJobParcel job;
+    bool result;
+    status = client->getJobWithId(0, &job, &result);
+    EXPECT_EQ(status.getServiceSpecificError(), IMediaTranscodingService::ERROR_DISCONNECTED);
+
+    status = client->cancelJob(0, &result);
+    EXPECT_EQ(status.getServiceSpecificError(), IMediaTranscodingService::ERROR_DISCONNECTED);
+
+    TranscodingRequestParcel request;
+    status = client->submitRequest(request, &job, &result);
+    EXPECT_EQ(status.getServiceSpecificError(), IMediaTranscodingService::ERROR_DISCONNECTED);
+}
+
 TEST_F(MediaTranscodingServiceTest, TestTranscodingUidPolicy) {
     ALOGD("TestTranscodingUidPolicy starting...");