[media] Add runtime flag to disable StarboardRenderer

[borongc]

Currently, when use_starboard_media=true, Cobalt.apk uses StarboardRenderer by default. This PR allows Cobalt.apk to select either StarboardRenderer or default Chromium renderer at runtime.

Chromium on android uses Surface Control to manage SurfaceView for overlay video mode, which is different from StarboardRenderer. This enables Cobalt team to profile the performance of different renderers, and develop/debug StarboardRenderer. This will be revisited to support secondary videos.

1. Allow to disable StarboardRenderer via adb shell am start --esa commandLineArgs "--disable-starboard-renderer".
2. DRM contents are only supported iif using StarboardRenderer.

b/380300976

[yell0wd0g]

A few nits. Is there a strong reason to have this as a
runtime switch and not just as a build flag (i.e. build
and use StarboardRenderer, or not)? I'm saying this
because if we give this option, we should test it, which
in turn means more configuration runs. If there's no
strong reason I'd vote for built-time only, which is
easier to debug by non-cobalt developers.

[niranjanyardi]

A few nits. Is there a strong reason to have this as a runtime switch and not just as a build flag (i.e. build and use StarboardRenderer, or not)? I'm saying this because if we give this option, we should test it, which in turn means more configuration runs. If there's no strong reason I'd vote for built-time only, which is easier to debug by non-cobalt developers.

I thought runtime switching is preferred over build time by default.

I'm saying this because if we give this option, we should test it, which in turn means more configuration runs.

testing shouldn't be very expensive on CI, IMO - we were running blackbox tests earlier and this case can be added to that

If there's no strong reason I'd vote for built-time only, which is easier to debug by non-cobalt developers.

This is interesting, IMO having a runtime flag is much easier for non-cobalt developers to test. Devs/ testers just have to launch cobalt with the correct runtime arguments. Building cobalt requires a lot of setup - which might not be feasible for testers .

Note that the way to implement this as you mentioned in anohter thread will be via //base , discussed here: go/cobalt-naming#creating-a-new-build-variable , https://chromium.googlesource.com/chromium/src/+/HEAD/docs/how_to_add_your_feature_flag.md

[yell0wd0g]

@niranjanyardi

I thought runtime switching is preferred over build time by default.

Depends what for. Having a runtime-switch allows users to toggle back and forth, but builds more code, has the risk of running unforeseen code paths, and usually adds friction (because e.g. crash logs/user reports will have to include the flag, and we devs will have to think about it being used or not); moreover IIUC the expectation is to use SbPlayer in production, and this secondary code path is intended as debugging help (comparison), never to be shipped.

I'm saying this because if we give this option, we should test it, which in turn means more configuration runs.

testing shouldn't be very expensive on CI, IMO - we were running blackbox tests earlier and this case can be added to that

It might not be much, but it adds up. E.g. how much time are we spending in fixing CI journeys/tests for e.g. AOSP? How important would be to fix a CI bot failing the test path using this flag? What if the Chromium media path fails unit tests (soon to be run, I hope) but only on some obscure platform? IOW death by a thousand cuts.

If there's no strong reason I'd vote for built-time only, which is easier to debug by non-cobalt developers.

This is interesting, IMO having a runtime flag is much easier for non-cobalt developers to test. Devs/ testers just have to launch cobalt with the correct runtime arguments. Building cobalt requires a lot of setup - which might not be feasible for testers .

Note that the way to implement this as you mentioned in anohter thread will be via //base , discussed here: go/cobalt-naming#creating-a-new-build-variable , https://chromium.googlesource.com/chromium/src/+/HEAD/docs/how_to_add_your_feature_flag.md

Chromium prefers base::Feature to command line switch for a number of practical reasons indeed. But since this flag is only intended for debugging/development (not e.g. for experiments in the field), switch might be more familiar to the devs/testers involved, so I don't feel particularly strong about it.

[xiaomings]

nit: Can we just use

#if BUILDFLAG(USE_STARBOARD_MEDIA)
    // TODO(b/326827007): Revisit renderer to support secondary videos.
    if (media::IsStarboardRendererEnabled()) {
      factory_selector->AddFactory(
          RendererType::kStarboard,
          std::make_unique<media::StarboardRendererFactory>(media_log));
      factory_selector->SetBaseRendererType(RendererType::kStarboard);
    }
#endif // BUILDFLAG(USE_STARBOARD_MEDIA)

and then fallback to the block below (and remove the #else)?

[xiaomings]

Just wanted to double check, do we want to always pass false here when StarboardRenderer is disabled, or shall we use the same logic as the one used below?

[xiaomings]

nit: Can we use

#if BUILDFLAG(USE_STARBOARD_MEDIA)
  if (media::IsStarboardRendererEnabled()) {
    strategies_.push_back(
        std::make_unique<OverlayStrategyUnderlayStarboard>(this));
  }
#endif   // BUILDFLAG(USE_STARBOARD_MEDIA)

and then fallback to the block below (and remove the #else)?