[Video Capture Service] Keep service running indefinitely on Android

When shutting down the service after an idle timeout, cached information
about supported device formats is lost. Re-querying these from the OS can
take several seconds on certain Android devices. To avoid this, this CL
keeps the service alive indefinitely on Android.

Bug: 878304, 792621
Change-Id: Ia031319d2f471e6359f248ae5c0b76208df77c54
Reviewed-on: https://chromium-review.googlesource.com/c/1231894
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596086}

services/service_manager/public/cpp/service_keepalive.cc

@@ -11,7 +11,7 @@
 namespace service_manager {
 
 ServiceKeepalive::ServiceKeepalive(ServiceContext* context,
-                                   base::TimeDelta idle_timeout,
+                                   base::Optional<base::TimeDelta> idle_timeout,
                                    TimeoutObserver* timeout_observer)
     : context_(context),
       idle_timeout_(idle_timeout),
@@ -40,7 +40,9 @@ void ServiceKeepalive::OnRefAdded() {
 }
 
 void ServiceKeepalive::OnRefCountZero() {
-  idle_timer_.Start(FROM_HERE, idle_timeout_,
+  if (!idle_timeout_.has_value())
+    return;
+  idle_timer_.Start(FROM_HERE, idle_timeout_.value(),
                     base::BindRepeating(&ServiceKeepalive::OnTimerExpired,
                                         weak_ptr_factory_.GetWeakPtr()));
 }

services/service_manager/public/cpp/service_keepalive.h

@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "base/timer/timer.h"
 #include "services/service_manager/public/cpp/service_context_ref.h"
 
@@ -39,10 +40,12 @@ class SERVICE_MANAGER_PUBLIC_CPP_EXPORT ServiceKeepalive {
   };
 
   // Creates a keepalive which allows the service to be idle for |idle_timeout|
-  // before requesting termination. Both |context| and |timeout_observer| are
-  // not owned and must outlive the ServiceKeepalive instance.
+  // before requesting termination. If |idle_timeout| is not given, the
+  // ServiceKeepalive will never request termination, i.e. the service will
+  // stay alive indefinitely. Both |context| and |timeout_observer| are not
+  // owned and must outlive the ServiceKeepalive instance.
   ServiceKeepalive(ServiceContext* context,
-                   base::TimeDelta idle_timeout,
+                   base::Optional<base::TimeDelta> idle_timeout,
                    TimeoutObserver* timeout_observer = nullptr);
   ~ServiceKeepalive();
 
@@ -55,7 +58,7 @@ class SERVICE_MANAGER_PUBLIC_CPP_EXPORT ServiceKeepalive {
   void OnTimerExpired();
 
   ServiceContext* const context_;
-  const base::TimeDelta idle_timeout_;
+  const base::Optional<base::TimeDelta> idle_timeout_;
   TimeoutObserver* const timeout_observer_;
   base::OneShotTimer idle_timer_;
   ServiceContextRefFactory ref_factory_;

services/video_capture/service_impl.cc

@@ -4,6 +4,7 @@
 
 #include "services/video_capture/service_impl.h"
 
+#include "build/build_config.h"
 #include "mojo/public/cpp/bindings/strong_binding.h"
 #include "services/service_manager/public/cpp/service_context.h"
 #include "services/video_capture/device_factory_provider_impl.h"
@@ -13,9 +14,8 @@
 
 namespace video_capture {
 
-ServiceImpl::ServiceImpl(float shutdown_delay_in_seconds)
-    : shutdown_delay_in_seconds_(shutdown_delay_in_seconds),
-      weak_factory_(this) {}
+ServiceImpl::ServiceImpl(base::Optional<base::TimeDelta> shutdown_delay)
+    : shutdown_delay_(shutdown_delay), weak_factory_(this) {}
 
 ServiceImpl::~ServiceImpl() {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -25,7 +25,14 @@ ServiceImpl::~ServiceImpl() {
 
 // static
 std::unique_ptr<service_manager::Service> ServiceImpl::Create() {
-  return std::make_unique<ServiceImpl>();
+#if defined(OS_ANDROID)
+  // On Android, we do not use automatic service shutdown, because when shutting
+  // down the service, we lose caching of the supported formats, and re-querying
+  // these can take several seconds on certain Android devices.
+  return std::make_unique<ServiceImpl>(base::Optional<base::TimeDelta>());
+#else
+  return std::make_unique<ServiceImpl>(base::TimeDelta::FromSeconds(5));
+#endif
 }
 
 void ServiceImpl::SetDestructionObserver(base::OnceClosure observer_cb) {
@@ -66,8 +73,7 @@ void ServiceImpl::OnStart() {
   // SetServiceContextRefProviderForTesting().
   if (!ref_factory_) {
     ref_factory_ = std::make_unique<service_manager::ServiceKeepalive>(
-        context(), base::TimeDelta::FromSecondsD(shutdown_delay_in_seconds_),
-        this);
+        context(), shutdown_delay_, this);
   }
 
   registry_.AddInterface<mojom::DeviceFactoryProvider>(

services/video_capture/service_impl.h

@@ -25,7 +25,9 @@ namespace video_capture {
 class ServiceImpl : public service_manager::Service,
                     public service_manager::ServiceKeepalive::TimeoutObserver {
  public:
-  ServiceImpl(float shutdown_delay_in_seconds = 5.0f);
+  // If |shutdown_delay| is provided, the service will shut itself down as soon
+  // as no client was connect for the corresponding duration.
+  explicit ServiceImpl(base::Optional<base::TimeDelta> shutdown_delay);
   ~ServiceImpl() override;
 
   static std::unique_ptr<service_manager::Service> Create();
@@ -58,7 +60,7 @@ class ServiceImpl : public service_manager::Service,
   void LazyInitializeDeviceFactoryProvider();
   void OnProviderClientDisconnected();
 
-  const float shutdown_delay_in_seconds_;
+  const base::Optional<base::TimeDelta> shutdown_delay_;
 #if defined(OS_WIN)
   // COM must be initialized in order to access the video capture devices.
   base::win::ScopedCOMInitializer com_initializer_;

services/video_capture/service_main.cc

@@ -7,6 +7,7 @@
 #include "services/video_capture/service_impl.h"
 
 MojoResult ServiceMain(MojoHandle service_request_handle) {
-  return service_manager::ServiceRunner(new video_capture::ServiceImpl())
+  return service_manager::ServiceRunner(
+             video_capture::ServiceImpl::Create().release())
       .Run(service_request_handle);
 }

services/video_capture/test/device_factory_provider_connectortest.cc

@@ -24,6 +24,7 @@ using testing::InvokeWithoutArgs;
 // Test fixture that creates a video_capture::ServiceImpl and sets up a
 // local service_manager::Connector through which client code can connect to
 // it.
+template <class DeviceFactoryProviderConnectorTestTraits>
 class DeviceFactoryProviderConnectorTest : public ::testing::Test {
  public:
   DeviceFactoryProviderConnectorTest() {}
@@ -32,13 +33,8 @@ class DeviceFactoryProviderConnectorTest : public ::testing::Test {
   void SetUp() override {
     base::CommandLine::ForCurrentProcess()->AppendSwitch(
         switches::kUseFakeDeviceForMediaStream);
-    // We need to set the shutdown delay to at least some epsilon > 0 in order
-    // to avoid the service shutting down synchronously which would prevent
-    // test case ServiceIncreasesRefCountOnNewConnectionAfterDisconnect from
-    // being able to reconnect before the timer expires.
-    static const float kShutdownDelayInSeconds = 0.0001f;
-    std::unique_ptr<ServiceImpl> service_impl =
-        std::make_unique<ServiceImpl>(kShutdownDelayInSeconds);
+    std::unique_ptr<ServiceImpl> service_impl = std::make_unique<ServiceImpl>(
+        DeviceFactoryProviderConnectorTestTraits::shutdown_delay());
     service_impl->SetDestructionObserver(base::BindOnce(
         [](base::RunLoop* service_destroyed_wait_loop) {
           service_destroyed_wait_loop->Quit();
@@ -59,7 +55,9 @@ class DeviceFactoryProviderConnectorTest : public ::testing::Test {
   }
 
   void TearDown() override {
-    if (factory_provider_.is_bound()) {
+    if (factory_provider_.is_bound() &&
+        DeviceFactoryProviderConnectorTestTraits::shutdown_delay()
+            .has_value()) {
       factory_provider_.reset();
       service_destroyed_wait_loop_.Run();
     }
@@ -78,9 +76,22 @@ class DeviceFactoryProviderConnectorTest : public ::testing::Test {
   std::unique_ptr<service_manager::TestConnectorFactory> connector_factory_;
 };
 
+// We need to set the shutdown delay to at least some epsilon > 0 in order
+// to avoid the service shutting down synchronously which would prevent
+// test case ServiceIncreasesRefCountOnNewConnectionAfterDisconnect from
+// being able to reconnect before the timer expires.
+struct ShortShutdownDelayDeviceFactoryProviderConnectorTestTraits {
+  static base::Optional<base::TimeDelta> shutdown_delay() {
+    return base::TimeDelta::FromMicroseconds(100);
+  }
+};
+using ShortShutdownDelayDeviceFactoryProviderConnectorTest =
+    DeviceFactoryProviderConnectorTest<
+        ShortShutdownDelayDeviceFactoryProviderConnectorTestTraits>;
+
 // Tests that the service does not quit when a client connects
 // while a second client stays connected.
-TEST_F(DeviceFactoryProviderConnectorTest,
+TEST_F(ShortShutdownDelayDeviceFactoryProviderConnectorTest,
        ServiceDoesNotQuitWhenOneOfTwoClientsDisconnects) {
   // Establish second connection
   mojom::DeviceFactoryProviderPtr second_connection;
@@ -109,7 +120,7 @@ TEST_F(DeviceFactoryProviderConnectorTest,
 // Tests that the service quits when the only client disconnects after not
 // having done anything other than obtaining a connection to the fake device
 // factory.
-TEST_F(DeviceFactoryProviderConnectorTest,
+TEST_F(ShortShutdownDelayDeviceFactoryProviderConnectorTest,
        ServiceQuitsWhenSingleClientDisconnected) {
   mojom::DeviceFactoryPtr factory;
   factory_provider_->ConnectToDeviceFactory(mojo::MakeRequest(&factory));
@@ -120,7 +131,7 @@ TEST_F(DeviceFactoryProviderConnectorTest,
 }
 
 // Tests that the service quits when both of two clients disconnect.
-TEST_F(DeviceFactoryProviderConnectorTest,
+TEST_F(ShortShutdownDelayDeviceFactoryProviderConnectorTest,
        ServiceQuitsWhenAllClientsDisconnected) {
   // Bind another client to the DeviceFactoryProvider interface.
   mojom::DeviceFactoryProviderPtr second_connection;
@@ -137,7 +148,7 @@ TEST_F(DeviceFactoryProviderConnectorTest,
 
 // Tests that the service increase the context ref count when a new connection
 // comes in after all previous connections have been released.
-TEST_F(DeviceFactoryProviderConnectorTest,
+TEST_F(ShortShutdownDelayDeviceFactoryProviderConnectorTest,
        ServiceIncreasesRefCountOnNewConnectionAfterDisconnect) {
   base::RunLoop wait_loop;
   service_impl_->SetShutdownTimeoutCancelledObserver(base::BindRepeating(
@@ -152,7 +163,7 @@ TEST_F(DeviceFactoryProviderConnectorTest,
 
 // Tests that the service quits when the last client disconnects while using a
 // device.
-TEST_F(DeviceFactoryProviderConnectorTest,
+TEST_F(ShortShutdownDelayDeviceFactoryProviderConnectorTest,
        ServiceQuitsWhenClientDisconnectsWhileUsingDevice) {
   mojom::DeviceFactoryPtr factory;
   factory_provider_->ConnectToDeviceFactory(mojo::MakeRequest(&factory));
@@ -201,4 +212,41 @@ TEST_F(DeviceFactoryProviderConnectorTest,
   service_destroyed_wait_loop_.Run();
 }
 
+struct NoAutomaticShutdownDeviceFactoryProviderConnectorTestTraits {
+  static base::Optional<base::TimeDelta> shutdown_delay() {
+    return base::Optional<base::TimeDelta>();
+  }
+};
+using NoAutomaticShutdownDeviceFactoryProviderConnectorTest =
+    DeviceFactoryProviderConnectorTest<
+        NoAutomaticShutdownDeviceFactoryProviderConnectorTestTraits>;
+
+// Tests that the service does not shut down after disconnecting.
+TEST_F(NoAutomaticShutdownDeviceFactoryProviderConnectorTest,
+       ServiceDoesNotShutDownOnDisconnect) {
+  {
+    base::RunLoop wait_loop;
+    service_impl_->SetFactoryProviderClientDisconnectedObserver(
+        wait_loop.QuitClosure());
+    factory_provider_.reset();
+    wait_loop.Run();
+  }
+
+  base::MockCallback<base::OnceClosure> service_impl_destructor_cb;
+  service_impl_->SetDestructionObserver(service_impl_destructor_cb.Get());
+  EXPECT_CALL(service_impl_destructor_cb, Run()).Times(0);
+
+  // Wait for an arbitrary short extra time after which we are convinced that
+  // there is enough evidence that service is not going to shut down.
+  {
+    base::RunLoop wait_loop;
+    base::OneShotTimer wait_timer;
+    wait_timer.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(10),
+                     wait_loop.QuitClosure());
+    wait_loop.Run();
+  }
+
+  service_impl_->SetDestructionObserver(base::DoNothing());
+}
+
 }  // namespace video_capture