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

Commit 15c192ad authored by Chong Zhang's avatar Chong Zhang
Browse files

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
parent 3fa408f8
Loading
Loading
Loading
Loading
+61 −19
Original line number Diff line number Diff line
@@ -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 @@ struct TranscodingClientManager::ClientImpl : public BnTranscodingClient {
    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 @@ struct TranscodingClientManager::ClientImpl : public BnTranscodingClient {
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 @@ TranscodingClientManager::ClientImpl::ClientImpl(
        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 @@ Status TranscodingClientManager::ClientImpl::submitRequest(
        *(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::getJobWithId(int32_t in_jobId,
}

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 @@ status_t TranscodingClientManager::addClient(
    // 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 @@ status_t TranscodingClientManager::addClient(

    // 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 @@ status_t TranscodingClientManager::removeClient(ClientIdType clientId) {
    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;
+25 −9
Original line number Diff line number Diff line
@@ -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 @@ bool TranscodingJobScheduler::cancel(ClientIdType clientId, JobIdType jobId) {

    ALOGV("%s: job %s", __FUNCTION__, jobToString(jobKey).c_str());

    std::list<JobKeyType> jobsToRemove;

    std::scoped_lock lock{mLock};

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

    for (auto it = jobsToRemove.begin(); it != jobsToRemove.end(); ++it) {
        // If the job is running, pause it first.
    if (mJobMap[jobKey].state == Job::RUNNING) {
        if (mJobMap[*it].state == Job::RUNNING) {
            mTranscoder->pause(clientId, jobId);
        }

        // Remove the job.
    removeJob_l(jobKey);
        removeJob_l(*it);
    }

    // Start next job.
    updateCurrentJob_l();
+21 −0
Original line number Diff line number Diff line
@@ -30,12 +30,33 @@ using ::aidl::android::media::TranscodingRequestParcel;
// 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;

+1 −1
Original line number Diff line number Diff line
@@ -46,7 +46,7 @@ using ::aidl::android::media::ITranscodingClientCallback;
 * 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();

+89 −23
Original line number Diff line number Diff line
@@ -39,6 +39,7 @@ using ::aidl::android::media::BnTranscodingClientCallback;
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 uid_t kClientUid = 3;
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 @@ TEST_F(TranscodingClientManagerTest, TestAddingWithInvalidClientCallback) {
    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 @@ TEST_F(TranscodingClientManagerTest, TestAddingWithInvalidClientPid) {
    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 @@ TEST_F(TranscodingClientManagerTest, TestAddingWithInvalidClientName) {
    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 @@ TEST_F(TranscodingClientManagerTest, TestAddingWithInvalidClientPackageName) {
    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 @@ TEST_F(TranscodingClientManagerTest, TestAddingDupliacteClient) {
    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 @@ TEST_F(TranscodingClientManagerTest, TestSubmitCancelGetJobs) {
    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 @@ TEST_F(TranscodingClientManagerTest, TestSubmitCancelGetJobs) {
    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 @@ TEST_F(TranscodingClientManagerTest, TestClientCallback) {
    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 @@ TEST_F(TranscodingClientManagerTest, TestClientCallback) {
    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
Loading