I found several command-line flags that affect hardware video encode/decode:
* kDisableGpu 
* kDisableAcceleratedVideoDecode
* kDisableAcceleratedMjpegDecode
* kDisableWebRtcHWDecoding
* kDisableWebRtcHWEncoding
* kEnableWebRtcHWH264Encoding
* kEnableAcceleratedVpxDecode         (Windows only)
* kEnableWin7WebRtcHWH264Decoding     (Windows only)
* kDisableVaapiAcceleratedVideoEncode (ChromeOS only)

Even though the flags can all be set independently, clearly there are 
dependencies between them. For example, when |kDisableGpu| is set, all hardware-
accelerated video encoding and decoding is probably expected to be disabled 
regardless of what the other flags are set to. It is not immediately clear 
which flags take precedence over which other flags. I am assuming that the more 
general flags take higher precedence, but to be sure one would have to dig through all usage sites.

As already pointed out, |kDisableAcceleratedVideoDecode| is currently affecting 
both encode and decode, contrary to its name. A possible solution would be to 
rename it to |kDisableAcceleratedVideo|. Then, in order to enable flag-based 
control over encode and decode separately, we could introduce two new flags 
called |kDisableHWVideoDecoding| and |kDisableHWVideoEncoding|.

However, adding these two new flags makes the dependency structure between the 
flags even more complex than it already is. As an example, in order to check if 
WebRTC should use HW video decoding on would have to check all of the following 4 flags: 
  * kDisableGpu 
  * kDisableAcceleratedVideo
  * kDisableHWVideoDecoding
  * kDisableWebRtcHWDecoding

In order to keep the dependencies between flags manageable and consistent, I am 
proposing to decouple the code that implements switchable features from the 
logic that determines how multiple flags may interact. This can be done as 
follows:
  1.) Create an interface RuntimeSwitchSet with methods such as 
         bool IsGpuEnabled()
         bool IsAcceleratedVideoEnabled()
         bool IsHwVideoDecodingEnabled()
         bool IsWebRtcHwDecodingEnabled()
  2.) Break dependency between code that checks runtime switches and the 
      global CommandLine singleton. Have code that checks runtime switches talk 
      to an instance of RuntimeSwitchSet instead.
  3.) Provide an implementation of RuntimeSwitchSet that knows the dependencies 
      between the command-line flags. It internally obtains the status of 
      the individual flags from the global CommandLine singleton and then 
      translates that information to a set of switches such that each 
      switchable feature only needs to ask for exactly one switch in order to 
      determine whether or not it is enabled.

This should eventually be done for all switchable features, but it would be 
possible to do this step-by-step and only start out with the video encoding and 
decoding features.

Please let me know your thoughts on this.

It seems a mechanism similar to the idea proposed in #3 already exists in form of base::FeatureList [1]. I will take a good look at that and figure out how it would be used here.

[1] https://cs.chromium.org/chromium/src/base/feature_list.h?q=FeatureList&sq=package:chromium&l=77&ct=rc&cd=1&dr=CSs

 Issue 616640  has been merged into this issue.

I experimented a bit with trying to convert the command-line flag 
|kDisableAcceleratedVideo| to a feature switch using base::FeatureList [1].
A major difficulty I ran into is that there are multiple mechanisms at work 
for deciding whether or not a switchable feature is enabled. Also they are 
backed by global (process-local) state where each process has its own copy.

The mechanisms I found include command-line flags [2], base::FeatureList [1], 
preference structs such as GpuPreferences, and potentially others, such as 
user preferences and GPU feature blacklists for certain devices. These mechanisms 
have varying support for things like changing switches during runtime, exposing 
switches in chrome://flags, logging in UMA telemetry, as well as performing 
field trials [3]. Propagation of the process-local state to child-processes 
is typically done manually by pushing a potentially modified copy of the state 
to the child process, e.g. at [4].

Code that implements switchable features uses a mix of all of the above mechanisms 
across all different processes to decide whether or not a feature is enabled. For 
example at [5], the decision about enabling WebUsb depends on both a command-line 
switch and a FeatureList entry, and the logic for how the decision is made is 
duplicated at [6].
Considering that switches can form non-trivial dependency structures, it becomes 
very difficult to see if, when, and why any particular feature is enabled or disabled.
It is also highly non-obvious how to keep the process-local copies consistent between 
processes.

This situation makes it much more difficult than it needs to be to make changes 
to feature switches. Rather than adding to the complexity by introducing more command-line 
flags, I feel it would be better to first agree on how we want to handle feature 
switches in Chromium going forward.

I feel the following design would make sense:
* Have an interface FeatureSwitches that exposes a method 
    bool IsFeatureEnabled(const std::string& feature_key).
* Have all code that implements switchable features use the FeatureSwitches interface 
to query whether or not the feature is enabled.
* In the main process (Browser) use an implementation of FeatureSwitches that makes 
its decisions based on process-local state, which may include command-line flags, 
base::FeatureList, and others. This implementation is to be the single location where the 
dependencies between switches are specified.
* In all child processes, use an implementation that obtains its results by delegating to 
the implementation hosted in the Browser process via IPC. For performance-critical code, 
we may use process-local caching.
* For switches that can change during runtime, use the observer pattern to notify 
interested clients when a switch changes.

To transition from the current state to the new design, we could make the new design 
mandatory for new feature switches and convert old switches over step-by-step whenever 
they get touched.

If we can get the buy-in, I would be happy to sketch this out in an experimental CL.

[1] https://cs.chromium.org/chromium/src/base/feature_list.h?q=FeatureList&ct=rc&cd=1&dr=CSs
[2] https://cs.chromium.org/chromium/src/base/command_line.h?q=HasSwitch()&sq=package:chromium&l=162&ct=rc&cd=1&dr=C
[3] https://cs.chromium.org/chromium/src/base/metrics/field_trial.h?l=78&ct=xref_jump_to_def&cl=GROK&gsn=FieldTrial
[4] https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_process_host_impl.cc?dr&q=render_process_host_impl.cc&sq=package:chromium&l=1367
[5] https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main.cc?q=chrome_browser_main.cc&dr&l=1366
[6] https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_client.cc?q=chrome_content_browser_client.cc&dr&l=2813

Let's just focus on the initial problem described, and leave the refactoring of existing flags to another bug/CL. I realize the links in the initial post is referring to the wrong lines now, so here are updated ones.

https://cs.chromium.org/chromium/src/content/renderer/media/webrtc/peer_connection_dependency_factory.cc?rcl=1459438281&l=249
https://cs.chromium.org/chromium/src/content/renderer/render_thread_impl.cc?rcl=1468927100&l=1463

I think we need to also keep in mind that these duplicated switches serve different purposes:
- base::FeatureList switches are used for Finch experiments 
https://g3doc.corp.google.com/analysis/uma/g3doc/finch/feature-api.md?cl=head
- Some command line switches would be public -exposed in about:config- and some would be not-exposed for devs/testers usage.
One of them enabling a feature whereas the other disables is a possibility. For instance, base::FeatureList disables H264 encoding as a part of Finch experiment, but you want to enable it via command line flags to test.

Refactoring these switches into a common interface, such as FeatureSwitches you mention, is a long task that requires input from many parties. I think first step we can do would be refactoring the existing flags(listed on #3) to have clear purposes and making sure they do what they say.

Sounds fair enough. I will copy the proposal to consolidate the feature switching logic from Comment #7 into a separate issue tracker entry. Let us keep the discussion going there.

In the meantime I will work on sorting out the exiting flags listed on #3.

I created a CL for making the switch kDisableAcceleratedVideoDecode only affect decode and not encode.

https://codereview.chromium.org/2656183002/

With that CL, some webkit_tests are failing on mac. It looks like these tests were already relying on the fact that the flag would disable video encode as well. I'll check if we can get away with setting kDisableWebRtcHWEncoding for these tests. If that is not enough, we may have to add a new flag that allows disabling of the encoding.

Hmm, it appears the failing tests were just "regular" flakiness.
The failure messages from the log related to GPU encoding that I interpreted as a sign that it is related to my change are also found in logs of recent successful runs of the same tests.

Re-running the bot just worked.

Latest status: I abandoned that CL from #10, because webrtc_media_recorder_browsertests already 
relies on kDisableAcceleratedVideoDecode to disable encode acceleration [1]. We probably have to add a separate flag for encoding, and there is a separate effort working towards that, see  issue 448638 .

I feel that adding new flags without fixing the overall issue of it being hard to track and control what interrelated flags are doing is dangerous. A solution might be do explicitly model dependencies between flags. I wrote a small design doc outlining this idea, see [2]. Currently waiting for comments/input on this idea.

[1] https://cs.chromium.org/chromium/src/content/browser/webrtc/webrtc_media_recorder_browsertest.cc?dr=CSs&l=61
[2] https://docs.google.com/document/d/1kydw6u7UwPG0WNdhRqVzEWr3rvEvTtYshTvD5CY0Z5c/edit?usp=sharing

 Issue 448638  has been merged into this issue.

Ping. What's the plan here?

Sorry for the delayed response.
I am not currently planning to work on this within the next weeks, so I am changing the status back to Available.

If Available then no Owner

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ea4f3b4dec37c27db1a46ec7f7e7ff6ff876f905

commit ea4f3b4dec37c27db1a46ec7f7e7ff6ff876f905
Author: Miguel Casas <mcasas@chromium.org>
Date: Mon Feb 05 18:56:25 2018

Remove gpu::GpuPreferences.disable_web_rtc_hw_encoding flag

GpuPreferences.disable_web_rtc_hw_encoding is essentially a
pass-through of the command line flag kDisableWebRtcHWEncoding
to be used only in the Gpu process GpuVideoEncodeAcceleratorFactory
to decide if the AndroidVEA should be used or not.

This flag is redundant, because the check should be and it is done
on the Renderer side (i.e. it's a policy, not a mechanism), see e.g
WebRTC (PeerConnectionDependencyFactory), while other users such
as MediaRecorder do it differently [1].

Also worth mentioning that the flag includes "webrtc" in the name
erroneously, and that there's a number of other flags like
kWebRtcHWVP8Encoding and  kWebRtcHWH264Encoding doing a similar job.

TBR=kbr@chromium.org, sandersd@chromium.org for two last RSs needed
(somehow not covered by the other 4 reviewers :-P  )

[1] https://cs.chromium.org/chromium/src/content/renderer/media_recorder/video_track_recorder.cc?sq=package:chromium&dr=C&l=135

Bug: 599650
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I4ae540a6a58f355ce3d09067511c37a64f87ed2e
Reviewed-on: https://chromium-review.googlesource.com/900243
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534439}
[modify] https://crrev.com/ea4f3b4dec37c27db1a46ec7f7e7ff6ff876f905/content/public/browser/gpu_utils.cc
[modify] https://crrev.com/ea4f3b4dec37c27db1a46ec7f7e7ff6ff876f905/content/renderer/media/webrtc/peer_connection_dependency_factory.cc
[modify] https://crrev.com/ea4f3b4dec37c27db1a46ec7f7e7ff6ff876f905/gpu/command_buffer/service/gpu_preferences.h
[modify] https://crrev.com/ea4f3b4dec37c27db1a46ec7f7e7ff6ff876f905/gpu/ipc/common/gpu_preferences.mojom
[modify] https://crrev.com/ea4f3b4dec37c27db1a46ec7f7e7ff6ff876f905/gpu/ipc/common/gpu_preferences_struct_traits.h
[modify] https://crrev.com/ea4f3b4dec37c27db1a46ec7f7e7ff6ff876f905/gpu/ipc/common/gpu_preferences_util_unittest.cc
[modify] https://crrev.com/ea4f3b4dec37c27db1a46ec7f7e7ff6ff876f905/media/gpu/gpu_video_encode_accelerator_factory.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e7abcd96f921cdca85f2849bbb78b39cf8f73f43

commit e7abcd96f921cdca85f2849bbb78b39cf8f73f43
Author: Miguel Casas <mcasas@chromium.org>
Date: Mon Feb 05 23:06:46 2018

cleanup GpuVideoEncodeAcceleratorFactory

Various cleanups on GpuVideoEncodeAcceleratorFactory that I
stumbled upon while adding more info to the DLOG(ERROR) in
CreateVEA().

Bug: 599650
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I99bfad58b82b9e87bbf224290a27bdf15f1201b2
Reviewed-on: https://chromium-review.googlesource.com/899224
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Chrome Cunningham <chcunningham@chromium.org>
Reviewed-by: Emircan Uysaler <emircan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534524}
[modify] https://crrev.com/e7abcd96f921cdca85f2849bbb78b39cf8f73f43/media/gpu/gpu_video_encode_accelerator_factory.cc
[modify] https://crrev.com/e7abcd96f921cdca85f2849bbb78b39cf8f73f43/media/gpu/gpu_video_encode_accelerator_factory.h