Merge "Skip transaction processing if the surface damage stays the same in Layer::setSurfaceDamageRegion." into udc-dev
diff --git a/libs/binder/RpcState.cpp b/libs/binder/RpcState.cpp
index ed3ce24..38bd081 100644
--- a/libs/binder/RpcState.cpp
+++ b/libs/binder/RpcState.cpp
@@ -262,10 +262,8 @@
 }
 
 void RpcState::clear() {
-    return clear(RpcMutexUniqueLock(mNodeMutex));
-}
+    RpcMutexUniqueLock _l(mNodeMutex);
 
-void RpcState::clear(RpcMutexUniqueLock nodeLock) {
     if (mTerminated) {
         LOG_ALWAYS_FATAL_IF(!mNodeForAddress.empty(),
                             "New state should be impossible after terminating!");
@@ -294,7 +292,7 @@
     auto temp = std::move(mNodeForAddress);
     mNodeForAddress.clear(); // RpcState isn't reusable, but for future/explicit
 
-    nodeLock.unlock();
+    _l.unlock();
     temp.clear(); // explicit
 }
 
@@ -706,7 +704,7 @@
     };
 
     {
-        RpcMutexUniqueLock _l(mNodeMutex);
+        RpcMutexLockGuard _l(mNodeMutex);
         if (mTerminated) return DEAD_OBJECT; // avoid fatal only, otherwise races
         auto it = mNodeForAddress.find(addr);
         LOG_ALWAYS_FATAL_IF(it == mNodeForAddress.end(),
@@ -722,9 +720,8 @@
         body.amount = it->second.timesRecd - target;
         it->second.timesRecd = target;
 
-        LOG_ALWAYS_FATAL_IF(nullptr != tryEraseNode(session, std::move(_l), it),
+        LOG_ALWAYS_FATAL_IF(nullptr != tryEraseNode(it),
                             "Bad state. RpcState shouldn't own received binder");
-        // LOCK ALREADY RELEASED
     }
 
     RpcWireHeader cmd = {
@@ -1167,8 +1164,8 @@
                    it->second.timesSent);
 
     it->second.timesSent -= body.amount;
-    sp<IBinder> tempHold = tryEraseNode(session, std::move(_l), it);
-    // LOCK ALREADY RELEASED
+    sp<IBinder> tempHold = tryEraseNode(it);
+    _l.unlock();
     tempHold = nullptr; // destructor may make binder calls on this session
 
     return OK;
@@ -1232,10 +1229,7 @@
     return OK;
 }
 
-sp<IBinder> RpcState::tryEraseNode(const sp<RpcSession>& session, RpcMutexUniqueLock nodeLock,
-                                   std::map<uint64_t, BinderNode>::iterator& it) {
-    bool shouldShutdown = false;
-
+sp<IBinder> RpcState::tryEraseNode(std::map<uint64_t, BinderNode>::iterator& it) {
     sp<IBinder> ref;
 
     if (it->second.timesSent == 0) {
@@ -1245,27 +1239,9 @@
             LOG_ALWAYS_FATAL_IF(!it->second.asyncTodo.empty(),
                                 "Can't delete binder w/ pending async transactions");
             mNodeForAddress.erase(it);
-
-            if (mNodeForAddress.size() == 0) {
-                shouldShutdown = true;
-            }
         }
     }
 
-    // If we shutdown, prevent RpcState from being re-used. This prevents another
-    // thread from getting the root object again.
-    if (shouldShutdown) {
-        clear(std::move(nodeLock));
-    } else {
-        nodeLock.unlock(); // explicit
-    }
-    // LOCK IS RELEASED
-
-    if (shouldShutdown) {
-        ALOGI("RpcState has no binders left, so triggering shutdown...");
-        (void)session->shutdownAndWait(false);
-    }
-
     return ref;
 }
 
diff --git a/libs/binder/RpcState.h b/libs/binder/RpcState.h
index 0e23ea7..ac86585 100644
--- a/libs/binder/RpcState.h
+++ b/libs/binder/RpcState.h
@@ -168,7 +168,6 @@
     void clear();
 
 private:
-    void clear(RpcMutexUniqueLock nodeLock);
     void dumpLocked();
 
     // Alternative to std::vector<uint8_t> that doesn't abort on allocation failure and caps
@@ -269,20 +268,11 @@
         std::string toString() const;
     };
 
-    // Checks if there is any reference left to a node and erases it. If this
-    // is the last node, shuts down the session.
-    //
-    // Node lock is passed here for convenience, so that we can release it
-    // and terminate the session, but we could leave it up to the caller
-    // by returning a continuation if we needed to erase multiple specific
-    // nodes. It may be tempting to allow the client to keep on holding the
-    // lock and instead just return whether or not we should shutdown, but
-    // this introduces the posssibility that another thread calls
-    // getRootBinder and thinks it is valid, rather than immediately getting
-    // an error.
-    sp<IBinder> tryEraseNode(const sp<RpcSession>& session, RpcMutexUniqueLock nodeLock,
-                             std::map<uint64_t, BinderNode>::iterator& it);
-
+    // checks if there is any reference left to a node and erases it. If erase
+    // happens, and there is a strong reference to the binder kept by
+    // binderNode, this returns that strong reference, so that it can be
+    // dropped after any locks are removed.
+    sp<IBinder> tryEraseNode(std::map<uint64_t, BinderNode>::iterator& it);
     // true - success
     // false - session shutdown, halt
     [[nodiscard]] bool nodeProgressAsyncNumber(BinderNode* node);
diff --git a/libs/binder/include/binder/RpcSession.h b/libs/binder/include/binder/RpcSession.h
index a323feb..0750ccf 100644
--- a/libs/binder/include/binder/RpcSession.h
+++ b/libs/binder/include/binder/RpcSession.h
@@ -51,9 +51,6 @@
  * This represents a session (group of connections) between a client
  * and a server. Multiple connections are needed for multiple parallel "binder"
  * calls which may also have nested calls.
- *
- * Once a binder exists in the session, if all references to all binders are dropped,
- * the session shuts down.
  */
 class RpcSession final : public virtual RefBase {
 public:
diff --git a/libs/binder/ndk/include_platform/android/binder_process.h b/libs/binder/ndk/include_platform/android/binder_process.h
index ffcad55..3fbe90d 100644
--- a/libs/binder/ndk/include_platform/android/binder_process.h
+++ b/libs/binder/ndk/include_platform/android/binder_process.h
@@ -32,7 +32,7 @@
  * Do not use this from a library. Apps setup their own threadpools, and otherwise, the main
  * function should be responsible for configuring the threadpool for the entire application.
  */
-void ABinderProcess_startThreadPool();
+void ABinderProcess_startThreadPool(void);
 /**
  * This sets the maximum number of threads that can be started in the threadpool. By default, after
  * startThreadPool is called, this is 15. If it is called additional times, it will only prevent
@@ -48,7 +48,7 @@
  * you should use this in a library to abort if the threadpool is not started.
  * Programs should configure binder threadpools once at the beginning.
  */
-bool ABinderProcess_isThreadPoolStarted();
+bool ABinderProcess_isThreadPoolStarted(void);
 /**
  * This adds the current thread to the threadpool. This may cause the threadpool to exceed the
  * maximum size.
@@ -56,7 +56,7 @@
  * Do not use this from a library. Apps setup their own threadpools, and otherwise, the main
  * function should be responsible for configuring the threadpool for the entire application.
  */
-void ABinderProcess_joinThreadPool();
+void ABinderProcess_joinThreadPool(void);
 
 /**
  * This gives you an fd to wait on. Whenever data is available on the fd,
@@ -79,6 +79,6 @@
  *
  * \return STATUS_OK on success
  */
-__attribute__((weak)) binder_status_t ABinderProcess_handlePolledCommands() __INTRODUCED_IN(31);
+__attribute__((weak)) binder_status_t ABinderProcess_handlePolledCommands(void) __INTRODUCED_IN(31);
 
 __END_DECLS
diff --git a/libs/binder/ndk/process.cpp b/libs/binder/ndk/process.cpp
index bc6610e..0fea57b 100644
--- a/libs/binder/ndk/process.cpp
+++ b/libs/binder/ndk/process.cpp
@@ -24,17 +24,17 @@
 using ::android::IPCThreadState;
 using ::android::ProcessState;
 
-void ABinderProcess_startThreadPool() {
+void ABinderProcess_startThreadPool(void) {
     ProcessState::self()->startThreadPool();
     ProcessState::self()->giveThreadPoolName();
 }
 bool ABinderProcess_setThreadPoolMaxThreadCount(uint32_t numThreads) {
     return ProcessState::self()->setThreadPoolMaxThreadCount(numThreads) == 0;
 }
-bool ABinderProcess_isThreadPoolStarted() {
+bool ABinderProcess_isThreadPoolStarted(void) {
     return ProcessState::self()->isThreadPoolStarted();
 }
-void ABinderProcess_joinThreadPool() {
+void ABinderProcess_joinThreadPool(void) {
     IPCThreadState::self()->joinThreadPool();
 }
 
@@ -42,6 +42,6 @@
     return IPCThreadState::self()->setupPolling(fd);
 }
 
-binder_status_t ABinderProcess_handlePolledCommands() {
+binder_status_t ABinderProcess_handlePolledCommands(void) {
     return IPCThreadState::self()->handlePolledCommands();
 }
diff --git a/libs/binder/tests/binderRpcBenchmark.cpp b/libs/binder/tests/binderRpcBenchmark.cpp
index 5939273..52ba9b0 100644
--- a/libs/binder/tests/binderRpcBenchmark.cpp
+++ b/libs/binder/tests/binderRpcBenchmark.cpp
@@ -102,11 +102,9 @@
 }
 
 static sp<RpcSession> gSession = RpcSession::make();
-static sp<IBinder> gRpcBinder;
 // Certificate validation happens during handshake and does not affect the result of benchmarks.
 // Skip certificate validation to simplify the setup process.
 static sp<RpcSession> gSessionTls = RpcSession::make(makeFactoryTls());
-static sp<IBinder> gRpcTlsBinder;
 #ifdef __BIONIC__
 static const String16 kKernelBinderInstance = String16(u"binderRpcBenchmark-control");
 static sp<IBinder> gKernelBinder;
@@ -120,9 +118,9 @@
             return gKernelBinder;
 #endif
         case RPC:
-            return gRpcBinder;
+            return gSession->getRootObject();
         case RPC_TLS:
-            return gRpcTlsBinder;
+            return gSessionTls->getRootObject();
         default:
             LOG(FATAL) << "Unknown transport value: " << transport;
             return nullptr;
@@ -256,13 +254,11 @@
     (void)unlink(addr.c_str());
     forkRpcServer(addr.c_str(), RpcServer::make(RpcTransportCtxFactoryRaw::make()));
     setupClient(gSession, addr.c_str());
-    gRpcBinder = gSession->getRootObject();
 
     std::string tlsAddr = tmp + "/binderRpcTlsBenchmark";
     (void)unlink(tlsAddr.c_str());
     forkRpcServer(tlsAddr.c_str(), RpcServer::make(makeFactoryTls()));
     setupClient(gSessionTls, tlsAddr.c_str());
-    gRpcTlsBinder = gSessionTls->getRootObject();
 
     ::benchmark::RunSpecifiedBenchmarks();
     return 0;
diff --git a/libs/binder/tests/binderRpcTest.cpp b/libs/binder/tests/binderRpcTest.cpp
index efc314b..0dc2028 100644
--- a/libs/binder/tests/binderRpcTest.cpp
+++ b/libs/binder/tests/binderRpcTest.cpp
@@ -163,8 +163,7 @@
             session.root = nullptr;
         }
 
-        for (size_t sessionNum = 0; sessionNum < sessions.size(); sessionNum++) {
-            auto& info = sessions.at(sessionNum);
+        for (auto& info : sessions) {
             sp<RpcSession>& session = info.session;
 
             EXPECT_NE(nullptr, session);
@@ -180,7 +179,6 @@
             for (size_t i = 0; i < 3; i++) {
                 sp<RpcSession> strongSession = weakSession.promote();
                 EXPECT_EQ(nullptr, strongSession)
-                        << "For session " << sessionNum << ". "
                         << (debugBacktrace(host.getPid()), debugBacktrace(getpid()),
                             "Leaked sess: ")
                         << strongSession->getStrongCount() << " checked time " << i;
@@ -233,21 +231,8 @@
     return std::move(sockClient);
 }
 
-std::string BinderRpc::PrintParamInfo(const testing::TestParamInfo<ParamType>& info) {
-    auto [type, security, clientVersion, serverVersion, singleThreaded, noKernel] = info.param;
-    auto ret = PrintToString(type) + "_" + newFactory(security)->toCString() + "_clientV" +
-            std::to_string(clientVersion) + "_serverV" + std::to_string(serverVersion);
-    if (singleThreaded) {
-        ret += "_single_threaded";
-    } else {
-        ret += "_multi_threaded";
-    }
-    if (noKernel) {
-        ret += "_no_kernel";
-    } else {
-        ret += "_with_kernel";
-    }
-    return ret;
+std::unique_ptr<RpcTransportCtxFactory> BinderRpc::newFactory(RpcSecurity rpcSecurity) {
+    return newTlsFactory(rpcSecurity);
 }
 
 // This creates a new process serving an interface on a certain number of
@@ -256,10 +241,6 @@
         const BinderRpcOptions& options) {
     CHECK_GE(options.numSessions, 1) << "Must have at least one session to a server";
 
-    if (options.numIncomingConnectionsBySession.size() != 0) {
-        CHECK_EQ(options.numIncomingConnectionsBySession.size(), options.numSessions);
-    }
-
     SocketType socketType = std::get<0>(GetParam());
     RpcSecurity rpcSecurity = std::get<1>(GetParam());
     uint32_t clientVersion = std::get<2>(GetParam());
@@ -327,7 +308,7 @@
             LOG_ALWAYS_FATAL("TIPC socket type only supported on vendor");
 #endif
         } else {
-            factory = newFactory(rpcSecurity, certVerifier);
+            factory = newTlsFactory(rpcSecurity, certVerifier);
         }
         sessions.emplace_back(RpcSession::make(std::move(factory)));
     }
@@ -357,15 +338,9 @@
 
     status_t status;
 
-    for (size_t i = 0; i < sessions.size(); i++) {
-        const auto& session = sessions.at(i);
-
-        size_t numIncoming = options.numIncomingConnectionsBySession.size() > 0
-                ? options.numIncomingConnectionsBySession.at(i)
-                : 0;
-
+    for (const auto& session : sessions) {
         CHECK(session->setProtocolVersion(clientVersion));
-        session->setMaxIncomingThreads(numIncoming);
+        session->setMaxIncomingThreads(options.numIncomingConnections);
         session->setMaxOutgoingConnections(options.numOutgoingConnections);
         session->setFileDescriptorTransportMode(options.clientFileDescriptorTransportMode);
 
@@ -663,32 +638,6 @@
     proc.proc->sessions.erase(proc.proc->sessions.begin() + 1);
 }
 
-TEST_P(BinderRpc, SessionWithIncomingThreadpoolDoesntLeak) {
-    if (clientOrServerSingleThreaded()) {
-        GTEST_SKIP() << "This test requires multiple threads";
-    }
-
-    // session 0 - will check for leaks in destrutor of proc
-    // session 1 - we want to make sure it gets deleted when we drop all references to it
-    auto proc = createRpcTestSocketServerProcess(
-            {.numThreads = 1, .numIncomingConnectionsBySession = {0, 1}, .numSessions = 2});
-
-    wp<RpcSession> session = proc.proc->sessions.at(1).session;
-
-    // remove all references to the second session
-    proc.proc->sessions.at(1).root = nullptr;
-    proc.proc->sessions.erase(proc.proc->sessions.begin() + 1);
-
-    // TODO(b/271830568) more efficient way to wait for other incoming threadpool
-    // to drain commands.
-    for (size_t i = 0; i < 100; i++) {
-        usleep(10 * 1000);
-        if (session.promote() == nullptr) break;
-    }
-
-    EXPECT_EQ(nullptr, session.promote());
-}
-
 TEST_P(BinderRpc, SingleDeathRecipient) {
     if (clientOrServerSingleThreaded()) {
         GTEST_SKIP() << "This test requires multiple threads";
@@ -706,7 +655,7 @@
 
     // Death recipient needs to have an incoming connection to be called
     auto proc = createRpcTestSocketServerProcess(
-            {.numThreads = 1, .numSessions = 1, .numIncomingConnectionsBySession = {1}});
+            {.numThreads = 1, .numSessions = 1, .numIncomingConnections = 1});
 
     auto dr = sp<MyDeathRec>::make();
     ASSERT_EQ(OK, proc.rootBinder->linkToDeath(dr, (void*)1, 0));
@@ -719,10 +668,6 @@
     ASSERT_TRUE(dr->mCv.wait_for(lock, 100ms, [&]() { return dr->dead; }));
 
     // need to wait for the session to shutdown so we don't "Leak session"
-    // can't do this before checking the death recipient by calling
-    // forceShutdown earlier, because shutdownAndWait will also trigger
-    // a death recipient, but if we had a way to wait for the service
-    // to gracefully shutdown, we could use that here.
     EXPECT_TRUE(proc.proc->sessions.at(0).session->shutdownAndWait(true));
     proc.expectAlreadyShutdown = true;
 }
@@ -744,7 +689,7 @@
 
     // Death recipient needs to have an incoming connection to be called
     auto proc = createRpcTestSocketServerProcess(
-            {.numThreads = 1, .numSessions = 1, .numIncomingConnectionsBySession = {1}});
+            {.numThreads = 1, .numSessions = 1, .numIncomingConnections = 1});
 
     auto dr = sp<MyDeathRec>::make();
     EXPECT_EQ(OK, proc.rootBinder->linkToDeath(dr, (void*)1, 0));
@@ -777,7 +722,8 @@
         void binderDied(const wp<IBinder>& /* who */) override {}
     };
 
-    auto proc = createRpcTestSocketServerProcess({.numThreads = 1, .numSessions = 1});
+    auto proc = createRpcTestSocketServerProcess(
+            {.numThreads = 1, .numSessions = 1, .numIncomingConnections = 0});
 
     auto dr = sp<MyDeathRec>::make();
     EXPECT_EQ(INVALID_OPERATION, proc.rootBinder->linkToDeath(dr, (void*)1, 0));
@@ -796,13 +742,19 @@
 
     // Death recipient needs to have an incoming connection to be called
     auto proc = createRpcTestSocketServerProcess(
-            {.numThreads = 1, .numSessions = 1, .numIncomingConnectionsBySession = {1}});
+            {.numThreads = 1, .numSessions = 1, .numIncomingConnections = 1});
 
     auto dr = sp<MyDeathRec>::make();
     ASSERT_EQ(OK, proc.rootBinder->linkToDeath(dr, (void*)1, 0));
     ASSERT_EQ(OK, proc.rootBinder->unlinkToDeath(dr, (void*)1, 0, nullptr));
 
-    proc.forceShutdown();
+    if (auto status = proc.rootIface->scheduleShutdown(); !status.isOk()) {
+        EXPECT_EQ(DEAD_OBJECT, status.transactionError()) << status;
+    }
+
+    // need to wait for the session to shutdown so we don't "Leak session"
+    EXPECT_TRUE(proc.proc->sessions.at(0).session->shutdownAndWait(true));
+    proc.expectAlreadyShutdown = true;
 }
 
 TEST_P(BinderRpc, Die) {
@@ -1257,7 +1209,7 @@
     };
 
     auto [isStrong1, isStrong2, rpcSecurity] = GetParam();
-    auto server = RpcServer::make(newFactory(rpcSecurity));
+    auto server = RpcServer::make(newTlsFactory(rpcSecurity));
     auto binder1 = sp<BBinder>::make();
     IBinder* binderRaw1 = binder1.get();
     setRootObject(isStrong1)(server.get(), binder1);
@@ -1353,7 +1305,7 @@
 class BinderRpcServerOnly : public ::testing::TestWithParam<std::tuple<RpcSecurity, uint32_t>> {
 public:
     static std::string PrintTestParam(const ::testing::TestParamInfo<ParamType>& info) {
-        return std::string(newFactory(std::get<0>(info.param))->toCString()) + "_serverV" +
+        return std::string(newTlsFactory(std::get<0>(info.param))->toCString()) + "_serverV" +
                 std::to_string(std::get<1>(info.param));
     }
 };
@@ -1361,7 +1313,7 @@
 TEST_P(BinderRpcServerOnly, SetExternalServerTest) {
     base::unique_fd sink(TEMP_FAILURE_RETRY(open("/dev/null", O_RDWR)));
     int sinkFd = sink.get();
-    auto server = RpcServer::make(newFactory(std::get<0>(GetParam())));
+    auto server = RpcServer::make(newTlsFactory(std::get<0>(GetParam())));
     server->setProtocolVersion(std::get<1>(GetParam()));
     ASSERT_FALSE(server->hasServer());
     ASSERT_EQ(OK, server->setupExternalServer(std::move(sink)));
@@ -1377,7 +1329,7 @@
     }
 
     auto addr = allocateSocketAddress();
-    auto server = RpcServer::make(newFactory(std::get<0>(GetParam())));
+    auto server = RpcServer::make(newTlsFactory(std::get<0>(GetParam())));
     server->setProtocolVersion(std::get<1>(GetParam()));
     ASSERT_EQ(OK, server->setupUnixDomainServer(addr.c_str()));
     auto joinEnds = std::make_shared<OneOffSignal>();
@@ -1426,7 +1378,7 @@
                 const Param& param,
                 std::unique_ptr<RpcAuth> auth = std::make_unique<RpcAuthSelfSigned>()) {
             auto [socketType, rpcSecurity, certificateFormat, serverVersion] = param;
-            auto rpcServer = RpcServer::make(newFactory(rpcSecurity));
+            auto rpcServer = RpcServer::make(newTlsFactory(rpcSecurity));
             rpcServer->setProtocolVersion(serverVersion);
             switch (socketType) {
                 case SocketType::PRECONNECTED: {
@@ -1505,7 +1457,7 @@
             }
             mFd = rpcServer->releaseServer();
             if (!mFd.fd.ok()) return AssertionFailure() << "releaseServer returns invalid fd";
-            mCtx = newFactory(rpcSecurity, mCertVerifier, std::move(auth))->newServerCtx();
+            mCtx = newTlsFactory(rpcSecurity, mCertVerifier, std::move(auth))->newServerCtx();
             if (mCtx == nullptr) return AssertionFailure() << "newServerCtx";
             mSetup = true;
             return AssertionSuccess();
@@ -1610,7 +1562,7 @@
             auto [socketType, rpcSecurity, certificateFormat, serverVersion] = param;
             (void)serverVersion;
             mFdTrigger = FdTrigger::make();
-            mCtx = newFactory(rpcSecurity, mCertVerifier)->newClientCtx();
+            mCtx = newTlsFactory(rpcSecurity, mCertVerifier)->newClientCtx();
             if (mCtx == nullptr) return AssertionFailure() << "newClientCtx";
             return AssertionSuccess();
         }
@@ -1682,7 +1634,7 @@
     using Client = RpcTransportTestUtils::Client;
     static inline std::string PrintParamInfo(const testing::TestParamInfo<ParamType>& info) {
         auto [socketType, rpcSecurity, certificateFormat, serverVersion] = info.param;
-        auto ret = PrintToString(socketType) + "_" + newFactory(rpcSecurity)->toCString();
+        auto ret = PrintToString(socketType) + "_" + newTlsFactory(rpcSecurity)->toCString();
         if (certificateFormat.has_value()) ret += "_" + PrintToString(*certificateFormat);
         ret += "_serverV" + std::to_string(serverVersion);
         return ret;
diff --git a/libs/binder/tests/binderRpcTestCommon.h b/libs/binder/tests/binderRpcTestCommon.h
index a9f0df3..1918cf4 100644
--- a/libs/binder/tests/binderRpcTestCommon.h
+++ b/libs/binder/tests/binderRpcTestCommon.h
@@ -126,11 +126,7 @@
 struct BinderRpcOptions {
     size_t numThreads = 1;
     size_t numSessions = 1;
-    // right now, this can be empty, or length numSessions, where each value
-    // represents the info for the corresponding session, but we should
-    // probably switch this to be a list of sessions options so that other
-    // options can all be specified per session
-    std::vector<size_t> numIncomingConnectionsBySession = {};
+    size_t numIncomingConnections = 0;
     size_t numOutgoingConnections = SIZE_MAX;
     RpcSession::FileDescriptorTransportMode clientFileDescriptorTransportMode =
             RpcSession::FileDescriptorTransportMode::NONE;
@@ -174,7 +170,7 @@
     return object;
 }
 
-static inline std::unique_ptr<RpcTransportCtxFactory> newFactory(
+static inline std::unique_ptr<RpcTransportCtxFactory> newTlsFactory(
         RpcSecurity rpcSecurity, std::shared_ptr<RpcCertificateVerifier> verifier = nullptr,
         std::unique_ptr<RpcAuth> auth = nullptr) {
     switch (rpcSecurity) {
diff --git a/libs/binder/tests/binderRpcTestFixture.h b/libs/binder/tests/binderRpcTestFixture.h
index 20fb6bf..d4d5968 100644
--- a/libs/binder/tests/binderRpcTestFixture.h
+++ b/libs/binder/tests/binderRpcTestFixture.h
@@ -64,21 +64,6 @@
     // whether session should be invalidated by end of run
     bool expectAlreadyShutdown = false;
 
-    // TODO(b/271830568): fix this in binderRpcTest, we always use the first session to cause the
-    // remote process to shutdown. Normally, when we shutdown, the default in the destructor is to
-    // check that there are no leaks and shutdown. However, when there are incoming threadpools,
-    // there will be a few extra binder threads there, so we can't shutdown the server. We should
-    // consider an alternative way of doing the test so that we don't need this, some ideas, such as
-    // program in understanding of incoming threadpool into the destructor so that (e.g.
-    // intelligently wait for sessions to shutdown now that they will do this)
-    void forceShutdown() {
-        if (auto status = rootIface->scheduleShutdown(); !status.isOk()) {
-            EXPECT_EQ(DEAD_OBJECT, status.transactionError()) << status;
-        }
-        EXPECT_TRUE(proc->sessions.at(0).session->shutdownAndWait(true));
-        expectAlreadyShutdown = true;
-    }
-
     BinderRpcTestProcessSession(BinderRpcTestProcessSession&&) = default;
     ~BinderRpcTestProcessSession() {
         if (!expectAlreadyShutdown) {
@@ -148,9 +133,26 @@
         return ret;
     }
 
-    static std::string PrintParamInfo(const testing::TestParamInfo<ParamType>& info);
+    static std::string PrintParamInfo(const testing::TestParamInfo<ParamType>& info) {
+        auto [type, security, clientVersion, serverVersion, singleThreaded, noKernel] = info.param;
+        auto ret = PrintToString(type) + "_" + newFactory(security)->toCString() + "_clientV" +
+                std::to_string(clientVersion) + "_serverV" + std::to_string(serverVersion);
+        if (singleThreaded) {
+            ret += "_single_threaded";
+        } else {
+            ret += "_multi_threaded";
+        }
+        if (noKernel) {
+            ret += "_no_kernel";
+        } else {
+            ret += "_with_kernel";
+        }
+        return ret;
+    }
 
 protected:
+    static std::unique_ptr<RpcTransportCtxFactory> newFactory(RpcSecurity rpcSecurity);
+
     std::unique_ptr<ProcessSession> createRpcTestSocketServerProcessEtc(
             const BinderRpcOptions& options);
 };
diff --git a/libs/binder/tests/binderRpcTestService.cpp b/libs/binder/tests/binderRpcTestService.cpp
index ca5a117..a9736d5 100644
--- a/libs/binder/tests/binderRpcTestService.cpp
+++ b/libs/binder/tests/binderRpcTestService.cpp
@@ -116,7 +116,7 @@
     }
 
     auto certVerifier = std::make_shared<RpcCertificateVerifierSimple>();
-    sp<RpcServer> server = RpcServer::make(newFactory(rpcSecurity, certVerifier));
+    sp<RpcServer> server = RpcServer::make(newTlsFactory(rpcSecurity, certVerifier));
 
     server->setProtocolVersion(serverConfig.serverVersion);
     server->setMaxThreads(serverConfig.numThreads);
diff --git a/libs/binder/tests/binderRpcTestTrusty.cpp b/libs/binder/tests/binderRpcTestTrusty.cpp
index 85794bd..56e1d64 100644
--- a/libs/binder/tests/binderRpcTestTrusty.cpp
+++ b/libs/binder/tests/binderRpcTestTrusty.cpp
@@ -39,31 +39,24 @@
     void terminate() override { LOG_ALWAYS_FATAL("terminate() not supported"); }
 };
 
-std::string BinderRpc::PrintParamInfo(const testing::TestParamInfo<ParamType>& info) {
-    auto [type, security, clientVersion, serverVersion, singleThreaded, noKernel] = info.param;
-    auto ret = PrintToString(type) + "_clientV" + std::to_string(clientVersion) + "_serverV" +
-            std::to_string(serverVersion);
-    if (singleThreaded) {
-        ret += "_single_threaded";
-    } else {
-        ret += "_multi_threaded";
+std::unique_ptr<RpcTransportCtxFactory> BinderRpc::newFactory(RpcSecurity rpcSecurity) {
+    switch (rpcSecurity) {
+        case RpcSecurity::RAW:
+            return RpcTransportCtxFactoryTipcTrusty::make();
+        default:
+            LOG_ALWAYS_FATAL("Unknown RpcSecurity %d", rpcSecurity);
     }
-    if (noKernel) {
-        ret += "_no_kernel";
-    } else {
-        ret += "_with_kernel";
-    }
-    return ret;
 }
 
 // This creates a new process serving an interface on a certain number of
 // threads.
 std::unique_ptr<ProcessSession> BinderRpc::createRpcTestSocketServerProcessEtc(
         const BinderRpcOptions& options) {
-    LOG_ALWAYS_FATAL_IF(options.numIncomingConnectionsBySession.size() != 0,
+    LOG_ALWAYS_FATAL_IF(options.numIncomingConnections != 0,
                         "Non-zero incoming connections %zu on Trusty",
-                        options.numIncomingConnectionsBySession.size());
+                        options.numIncomingConnections);
 
+    RpcSecurity rpcSecurity = std::get<1>(GetParam());
     uint32_t clientVersion = std::get<2>(GetParam());
     uint32_t serverVersion = std::get<3>(GetParam());
 
@@ -71,8 +64,7 @@
 
     status_t status;
     for (size_t i = 0; i < options.numSessions; i++) {
-        auto factory = android::RpcTransportCtxFactoryTipcTrusty::make();
-        auto session = android::RpcSession::make(std::move(factory));
+        auto session = android::RpcSession::make(newFactory(rpcSecurity));
 
         EXPECT_TRUE(session->setProtocolVersion(clientVersion));
         session->setMaxOutgoingConnections(options.numOutgoingConnections);
diff --git a/libs/binder/tests/binderRpcUniversalTests.cpp b/libs/binder/tests/binderRpcUniversalTests.cpp
index 1f46010..11a22b0 100644
--- a/libs/binder/tests/binderRpcUniversalTests.cpp
+++ b/libs/binder/tests/binderRpcUniversalTests.cpp
@@ -463,7 +463,7 @@
                 auto proc = createRpcTestSocketServerProcess(
                         {.numThreads = 1,
                          .numSessions = 1,
-                         .numIncomingConnectionsBySession = {numIncomingConnections}});
+                         .numIncomingConnections = numIncomingConnections});
                 auto cb = sp<MyBinderRpcCallback>::make();
 
                 if (callIsOneway) {
@@ -491,7 +491,16 @@
                         << "callIsOneway: " << callIsOneway
                         << " callbackIsOneway: " << callbackIsOneway << " delayed: " << delayed;
 
-                proc.forceShutdown();
+                // since we are severing the connection, we need to go ahead and
+                // tell the server to shutdown and exit so that waitpid won't hang
+                if (auto status = proc.rootIface->scheduleShutdown(); !status.isOk()) {
+                    EXPECT_EQ(DEAD_OBJECT, status.transactionError()) << status;
+                }
+
+                // since this session has an incoming connection w/ a threadpool, we
+                // need to manually shut it down
+                EXPECT_TRUE(proc.proc->sessions.at(0).session->shutdownAndWait(true));
+                proc.expectAlreadyShutdown = true;
             }
         }
     }
diff --git a/libs/binder/tests/unit_fuzzers/RecordedTransactionFileFuzz.cpp b/libs/binder/tests/unit_fuzzers/RecordedTransactionFileFuzz.cpp
index 73790fa..e494366 100644
--- a/libs/binder/tests/unit_fuzzers/RecordedTransactionFileFuzz.cpp
+++ b/libs/binder/tests/unit_fuzzers/RecordedTransactionFileFuzz.cpp
@@ -26,7 +26,7 @@
     rewind(intermediateFile);
     int fileNumber = fileno(intermediateFile);
 
-    android::base::unique_fd fd(fileNumber);
+    android::base::unique_fd fd(dup(fileNumber));
 
     auto transaction = android::binder::debug::RecordedTransaction::fromFile(fd);
 
diff --git a/libs/jpegrecoverymap/include/jpegrecoverymap/jpegr.h b/libs/jpegrecoverymap/include/jpegrecoverymap/jpegr.h
index 9b2dde7..e2023a6 100644
--- a/libs/jpegrecoverymap/include/jpegrecoverymap/jpegr.h
+++ b/libs/jpegrecoverymap/include/jpegrecoverymap/jpegr.h
@@ -91,7 +91,10 @@
     int length;
 };
 
-struct jpegr_metadata {
+/*
+ * Holds information for recovery map related metadata.
+ */
+struct jpegr_metadata_struct {
   // JPEG/R version
   uint32_t version;
   // Max Content Boost for the map
@@ -103,12 +106,14 @@
 typedef struct jpegr_uncompressed_struct* jr_uncompressed_ptr;
 typedef struct jpegr_compressed_struct* jr_compressed_ptr;
 typedef struct jpegr_exif_struct* jr_exif_ptr;
-typedef struct jpegr_metadata* jr_metadata_ptr;
+typedef struct jpegr_metadata_struct* jr_metadata_ptr;
 typedef struct jpegr_info_struct* jr_info_ptr;
 
 class JpegR {
 public:
     /*
+     * Experimental only
+     *
      * Encode API-0
      * Compress JPEGR image from 10-bit HDR YUV.
      *
@@ -199,21 +204,40 @@
      * Decode API
      * Decompress JPEGR image.
      *
-     * The output JPEGR image is in RGBA_1010102 data format if decoding to HDR.
-     * @param compressed_jpegr_image compressed JPEGR image
-     * @param dest destination of the uncompressed JPEGR image
-     * @param exif destination of the decoded EXIF metadata.
-     * @param output_format flag for setting output color format. if set to
-     *                      {@code JPEGR_OUTPUT_SDR}, decoder will only decode the primary image
-     *                      which is SDR. Default value is JPEGR_OUTPUT_HDR_LINEAR.
-     * @param recovery_map destination of the decoded recovery map.
+     * @param compressed_jpegr_image compressed JPEGR image.
+     * @param dest destination of the uncompressed JPEGR image.
+     * @param exif destination of the decoded EXIF metadata. The default value is NULL where the
+                   decoder will do nothing about it. If configured not NULL the decoder will write
+                   EXIF data into this structure. The format is defined in {@code jpegr_exif_struct}
+     * @param output_format flag for setting output color format. Its value configures the output
+                            color format. The default value is {@code JPEGR_OUTPUT_HDR_LINEAR}.
+                            ----------------------------------------------------------------------
+                            |   output_format          |    decoded color format to be written   |
+                            ----------------------------------------------------------------------
+                            |     JPEGR_OUTPUT_SDR     |                RGBA_8888                |
+                            ----------------------------------------------------------------------
+                            | JPEGR_OUTPUT_HDR_LINEAR  |        (default)RGBA_F16 linear         |
+                            ----------------------------------------------------------------------
+                            |   JPEGR_OUTPUT_HDR_PQ    |             RGBA_1010102 PQ             |
+                            ----------------------------------------------------------------------
+                            |   JPEGR_OUTPUT_HDR_HLG   |            RGBA_1010102 HLG             |
+                            ----------------------------------------------------------------------
+     * @param recovery_map destination of the decoded recovery map. The default value is NULL where
+                           the decoder will do nothing about it. If configured not NULL the decoder
+                           will write the decoded recovery_map data into this structure. The format
+                           is defined in {@code jpegr_uncompressed_struct}.
+     * @param metadata destination of the decoded metadata. The default value is NULL where the
+                       decoder will do nothing about it. If configured not NULL the decoder will
+                       write metadata into this structure. the format of metadata is defined in
+                       {@code jpegr_metadata}.
      * @return NO_ERROR if decoding succeeds, error code if error occurs.
      */
     status_t decodeJPEGR(jr_compressed_ptr compressed_jpegr_image,
                          jr_uncompressed_ptr dest,
                          jr_exif_ptr exif = nullptr,
                          jpegr_output_format output_format = JPEGR_OUTPUT_HDR_LINEAR,
-                         jr_uncompressed_ptr recovery_map = nullptr);
+                         jr_uncompressed_ptr recovery_map = nullptr,
+                         jr_metadata_ptr metadata = nullptr);
 
     /*
     * Gets Info from JPEGR file without decoding it.
diff --git a/libs/jpegrecoverymap/include/jpegrecoverymap/jpegrutils.h b/libs/jpegrecoverymap/include/jpegrecoverymap/jpegrutils.h
index a381743..dd06fa2 100644
--- a/libs/jpegrecoverymap/include/jpegrecoverymap/jpegrutils.h
+++ b/libs/jpegrecoverymap/include/jpegrecoverymap/jpegrutils.h
@@ -45,7 +45,7 @@
     #define Endian_SwapBE16(n) (n)
 #endif
 
-struct jpegr_metadata;
+struct jpegr_metadata_struct;
 /*
  * Mutable data structure. Holds information for metadata.
  */
@@ -87,7 +87,7 @@
  * @param metadata place to store HDR metadata values
  * @return true if metadata is successfully retrieved, false otherwise
 */
-bool getMetadataFromXMP(uint8_t* xmp_data, size_t xmp_size, jpegr_metadata* metadata);
+bool getMetadataFromXMP(uint8_t* xmp_data, size_t xmp_size, jpegr_metadata_struct* metadata);
 
 /*
  * This method generates XMP metadata for the primary image.
@@ -156,7 +156,7 @@
  * @param metadata JPEG/R metadata to encode as XMP
  * @return XMP metadata in type of string
  */
- std::string generateXmpForSecondaryImage(jpegr_metadata& metadata);
+ std::string generateXmpForSecondaryImage(jpegr_metadata_struct& metadata);
 }  // namespace android::jpegrecoverymap
 
 #endif //ANDROID_JPEGRECOVERYMAP_JPEGRUTILS_H
diff --git a/libs/jpegrecoverymap/jpegr.cpp b/libs/jpegrecoverymap/jpegr.cpp
index f8763c6..e197bf0 100644
--- a/libs/jpegrecoverymap/jpegr.cpp
+++ b/libs/jpegrecoverymap/jpegr.cpp
@@ -107,7 +107,7 @@
     return ERROR_JPEGR_INVALID_INPUT_TYPE;
   }
 
-  jpegr_metadata metadata;
+  jpegr_metadata_struct metadata;
   metadata.version = kJpegrVersion;
 
   jpegr_uncompressed_struct uncompressed_yuv_420_image;
@@ -176,7 +176,7 @@
     return ERROR_JPEGR_INVALID_INPUT_TYPE;
   }
 
-  jpegr_metadata metadata;
+  jpegr_metadata_struct metadata;
   metadata.version = kJpegrVersion;
 
   jpegr_uncompressed_struct map;
@@ -235,7 +235,7 @@
     return ERROR_JPEGR_INVALID_INPUT_TYPE;
   }
 
-  jpegr_metadata metadata;
+  jpegr_metadata_struct metadata;
   metadata.version = kJpegrVersion;
 
   jpegr_uncompressed_struct map;
@@ -288,7 +288,7 @@
     return ERROR_JPEGR_RESOLUTION_MISMATCH;
   }
 
-  jpegr_metadata metadata;
+  jpegr_metadata_struct metadata;
   metadata.version = kJpegrVersion;
 
   jpegr_uncompressed_struct map;
@@ -332,7 +332,8 @@
                             jr_uncompressed_ptr dest,
                             jr_exif_ptr exif,
                             jpegr_output_format output_format,
-                            jr_uncompressed_ptr recovery_map) {
+                            jr_uncompressed_ptr recovery_map,
+                            jr_metadata_ptr metadata) {
   if (compressed_jpegr_image == nullptr || dest == nullptr) {
     return ERROR_JPEGR_INVALID_NULL_PTR;
   }
@@ -387,6 +388,18 @@
     memcpy(recovery_map->data, recovery_map_decoder.getDecompressedImagePtr(), size);
   }
 
+  jpegr_metadata_struct jr_metadata;
+  if (!getMetadataFromXMP(static_cast<uint8_t*>(recovery_map_decoder.getXMPPtr()),
+                          recovery_map_decoder.getXMPSize(), &jr_metadata)) {
+    return ERROR_JPEGR_DECODE_ERROR;
+  }
+
+  if (metadata != nullptr) {
+      metadata->version = jr_metadata.version;
+      metadata->minContentBoost = jr_metadata.minContentBoost;
+      metadata->maxContentBoost = jr_metadata.maxContentBoost;
+  }
+
   if (output_format == JPEGR_OUTPUT_SDR) {
     return NO_ERROR;
   }
@@ -417,13 +430,8 @@
   uncompressed_yuv_420_image.width = jpeg_decoder.getDecompressedImageWidth();
   uncompressed_yuv_420_image.height = jpeg_decoder.getDecompressedImageHeight();
 
-  jpegr_metadata metadata;
-  if (!getMetadataFromXMP(static_cast<uint8_t*>(recovery_map_decoder.getXMPPtr()),
-                          recovery_map_decoder.getXMPSize(), &metadata)) {
-    return ERROR_JPEGR_DECODE_ERROR;
-  }
-
-  JPEGR_CHECK(applyRecoveryMap(&uncompressed_yuv_420_image, &map, &metadata, output_format, dest));
+  JPEGR_CHECK(applyRecoveryMap(&uncompressed_yuv_420_image, &map, &jr_metadata, output_format,
+                               dest));
   return NO_ERROR;
 }
 
diff --git a/libs/jpegrecoverymap/jpegrutils.cpp b/libs/jpegrecoverymap/jpegrutils.cpp
index 38b78ad..ff96447 100644
--- a/libs/jpegrecoverymap/jpegrutils.cpp
+++ b/libs/jpegrecoverymap/jpegrutils.cpp
@@ -253,7 +253,7 @@
 const string XMPXmlHandler::minContentBoostAttrName = kMapGainMapMin;
 const string XMPXmlHandler::maxContentBoostAttrName = kMapGainMapMax;
 
-bool getMetadataFromXMP(uint8_t* xmp_data, size_t xmp_size, jpegr_metadata* metadata) {
+bool getMetadataFromXMP(uint8_t* xmp_data, size_t xmp_size, jpegr_metadata_struct* metadata) {
     string nameSpace = "http://ns.adobe.com/xap/1.0/\0";
 
     if (xmp_size < nameSpace.size()+2) {
@@ -327,7 +327,7 @@
   return ss.str();
 }
 
-string generateXmpForSecondaryImage(jpegr_metadata& metadata) {
+string generateXmpForSecondaryImage(jpegr_metadata_struct& metadata) {
   const vector<string> kConDirSeq({kConDirectory, string("rdf:Seq")});
   const vector<string> kLiItem({string("rdf:li"), kConItem});
 
diff --git a/libs/jpegrecoverymap/tests/jpegr_test.cpp b/libs/jpegrecoverymap/tests/jpegr_test.cpp
index 0a7d20a..be4b972 100644
--- a/libs/jpegrecoverymap/tests/jpegr_test.cpp
+++ b/libs/jpegrecoverymap/tests/jpegr_test.cpp
@@ -174,7 +174,7 @@
 }
 
 TEST_F(JpegRTest, writeXmpThenRead) {
-  jpegr_metadata metadata_expected;
+  jpegr_metadata_struct metadata_expected;
   metadata_expected.maxContentBoost = 1.25;
   metadata_expected.minContentBoost = 0.75;
   const std::string nameSpace = "http://ns.adobe.com/xap/1.0/\0";
@@ -189,7 +189,7 @@
   xmpData.insert(xmpData.end(), reinterpret_cast<const uint8_t*>(xmp.c_str()),
                   reinterpret_cast<const uint8_t*>(xmp.c_str()) + xmp.size());
 
-  jpegr_metadata metadata_read;
+  jpegr_metadata_struct metadata_read;
   EXPECT_TRUE(getMetadataFromXMP(xmpData.data(), xmpData.size(), &metadata_read));
   ASSERT_EQ(metadata_expected.maxContentBoost, metadata_read.maxContentBoost);
   ASSERT_EQ(metadata_expected.minContentBoost, metadata_read.minContentBoost);
@@ -476,7 +476,7 @@
 
   JpegRBenchmark benchmark;
 
-  jpegr_metadata metadata = { .version = 1,
+  jpegr_metadata_struct metadata = { .version = 1,
                               .maxContentBoost = 8.0f,
                               .minContentBoost = 1.0f / 8.0f };
 
diff --git a/libs/jpegrecoverymap/tests/recoverymapmath_test.cpp b/libs/jpegrecoverymap/tests/recoverymapmath_test.cpp
index 6c61ff1..cf6a034 100644
--- a/libs/jpegrecoverymap/tests/recoverymapmath_test.cpp
+++ b/libs/jpegrecoverymap/tests/recoverymapmath_test.cpp
@@ -554,8 +554,8 @@
 
 TEST_F(RecoveryMapMathTest, applyRecoveryLUT) {
   for (int boost = 1; boost <= 10; boost++) {
-    jpegr_metadata metadata = { .maxContentBoost = static_cast<float>(boost),
-                                .minContentBoost = 1.0f / static_cast<float>(boost) };
+    jpegr_metadata_struct metadata = { .maxContentBoost = static_cast<float>(boost),
+                                       .minContentBoost = 1.0f / static_cast<float>(boost) };
     RecoveryLUT recoveryLUT(&metadata);
     for (int idx = 0; idx < kRecoveryFactorNumEntries; idx++) {
       float value = static_cast<float>(idx) / static_cast<float>(kRecoveryFactorNumEntries - 1);
@@ -573,8 +573,8 @@
   }
 
   for (int boost = 1; boost <= 10; boost++) {
-    jpegr_metadata metadata = { .maxContentBoost = static_cast<float>(boost),
-                                .minContentBoost = 1.0f };
+    jpegr_metadata_struct metadata = { .maxContentBoost = static_cast<float>(boost),
+                                       .minContentBoost = 1.0f };
     RecoveryLUT recoveryLUT(&metadata);
     for (int idx = 0; idx < kRecoveryFactorNumEntries; idx++) {
       float value = static_cast<float>(idx) / static_cast<float>(kRecoveryFactorNumEntries - 1);
@@ -592,8 +592,8 @@
   }
 
   for (int boost = 1; boost <= 10; boost++) {
-    jpegr_metadata metadata = { .maxContentBoost = static_cast<float>(boost),
-                                .minContentBoost = 1.0f / pow(static_cast<float>(boost),
+    jpegr_metadata_struct metadata = { .maxContentBoost = static_cast<float>(boost),
+                                       .minContentBoost = 1.0f / pow(static_cast<float>(boost),
                                                               1.0f / 3.0f) };
     RecoveryLUT recoveryLUT(&metadata);
     for (int idx = 0; idx < kRecoveryFactorNumEntries; idx++) {
@@ -659,8 +659,8 @@
 }
 
 TEST_F(RecoveryMapMathTest, EncodeRecovery) {
-  jpegr_metadata metadata = { .maxContentBoost = 4.0f,
-                              .minContentBoost = 1.0f / 4.0f };
+  jpegr_metadata_struct metadata = { .maxContentBoost = 4.0f,
+                                     .minContentBoost = 1.0f / 4.0f };
 
   EXPECT_EQ(encodeRecovery(0.0f, 0.0f, &metadata), 127);
   EXPECT_EQ(encodeRecovery(0.0f, 1.0f, &metadata), 127);
@@ -717,8 +717,8 @@
 }
 
 TEST_F(RecoveryMapMathTest, ApplyRecovery) {
-  jpegr_metadata metadata = { .maxContentBoost = 4.0f,
-                              .minContentBoost = 1.0f / 4.0f };
+  jpegr_metadata_struct metadata = { .maxContentBoost = 4.0f,
+                                     .minContentBoost = 1.0f / 4.0f };
 
   EXPECT_RGB_NEAR(applyRecovery(RgbBlack(), 0.0f, &metadata), RgbBlack());
   EXPECT_RGB_NEAR(applyRecovery(RgbBlack(), 0.5f, &metadata), RgbBlack());
@@ -981,8 +981,8 @@
 }
 
 TEST_F(RecoveryMapMathTest, ApplyMap) {
-  jpegr_metadata metadata = { .maxContentBoost = 8.0f,
-                              .minContentBoost = 1.0f / 8.0f };
+  jpegr_metadata_struct metadata = { .maxContentBoost = 8.0f,
+                                     .minContentBoost = 1.0f / 8.0f };
 
   EXPECT_RGB_EQ(Recover(YuvWhite(), 1.0f, &metadata),
                 RgbWhite() * 8.0f);
diff --git a/services/surfaceflinger/FrameTimeline/FrameTimeline.cpp b/services/surfaceflinger/FrameTimeline/FrameTimeline.cpp
index 925f111..ded734e 100644
--- a/services/surfaceflinger/FrameTimeline/FrameTimeline.cpp
+++ b/services/surfaceflinger/FrameTimeline/FrameTimeline.cpp
@@ -885,15 +885,17 @@
 
 void FrameTimeline::DisplayFrame::classifyJank(nsecs_t& deadlineDelta, nsecs_t& deltaToVsync,
                                                nsecs_t previousPresentTime) {
-    if (mPredictionState == PredictionState::Expired ||
-        mSurfaceFlingerActuals.presentTime == Fence::SIGNAL_TIME_INVALID) {
+    const bool presentTimeValid =
+            mSurfaceFlingerActuals.presentTime >= mSurfaceFlingerActuals.startTime;
+    if (mPredictionState == PredictionState::Expired || !presentTimeValid) {
         // Cannot do jank classification with expired predictions or invalid signal times. Set the
         // deltas to 0 as both negative and positive deltas are used as real values.
         mJankType = JankType::Unknown;
         deadlineDelta = 0;
         deltaToVsync = 0;
-        if (mSurfaceFlingerActuals.presentTime == Fence::SIGNAL_TIME_INVALID) {
+        if (!presentTimeValid) {
             mSurfaceFlingerActuals.presentTime = mSurfaceFlingerActuals.endTime;
+            mJankType |= JankType::DisplayHAL;
         }
 
         return;
diff --git a/services/surfaceflinger/tests/unittests/FrameTimelineTest.cpp b/services/surfaceflinger/tests/unittests/FrameTimelineTest.cpp
index abd7789..d26ef3c 100644
--- a/services/surfaceflinger/tests/unittests/FrameTimelineTest.cpp
+++ b/services/surfaceflinger/tests/unittests/FrameTimelineTest.cpp
@@ -489,7 +489,7 @@
 
     auto displayFrame0 = getDisplayFrame(0);
     EXPECT_EQ(displayFrame0->getActuals().presentTime, 59);
-    EXPECT_EQ(displayFrame0->getJankType(), JankType::Unknown);
+    EXPECT_EQ(displayFrame0->getJankType(), JankType::Unknown | JankType::DisplayHAL);
     EXPECT_EQ(surfaceFrame1->getActuals().presentTime, -1);
     EXPECT_EQ(surfaceFrame1->getJankType(), JankType::Unknown);
 }
@@ -2259,6 +2259,7 @@
     mFrameTimeline->setSfWakeUp(sfToken3, 72, Fps::fromPeriodNsecs(11));
     mFrameTimeline->setSfPresent(80, validPresentFence);
 
+    erroneousPresentFence2->signalForTest(2);
     validPresentFence->signalForTest(80);
 
     addEmptyDisplayFrame();
@@ -2268,14 +2269,14 @@
         EXPECT_EQ(displayFrame->getActuals().presentTime, 26);
         EXPECT_EQ(displayFrame->getFramePresentMetadata(), FramePresentMetadata::UnknownPresent);
         EXPECT_EQ(displayFrame->getFrameReadyMetadata(), FrameReadyMetadata::UnknownFinish);
-        EXPECT_EQ(displayFrame->getJankType(), JankType::Unknown);
+        EXPECT_EQ(displayFrame->getJankType(), JankType::Unknown | JankType::DisplayHAL);
     }
     {
         auto displayFrame = getDisplayFrame(1);
         EXPECT_EQ(displayFrame->getActuals().presentTime, 60);
         EXPECT_EQ(displayFrame->getFramePresentMetadata(), FramePresentMetadata::UnknownPresent);
         EXPECT_EQ(displayFrame->getFrameReadyMetadata(), FrameReadyMetadata::UnknownFinish);
-        EXPECT_EQ(displayFrame->getJankType(), JankType::Unknown);
+        EXPECT_EQ(displayFrame->getJankType(), JankType::Unknown | JankType::DisplayHAL);
     }
     {
         auto displayFrame = getDisplayFrame(2);