Use use_linux_video_acceleration flag in BUILD.gn to simplify files [chromium/src : main]
Miguel Casas-Sanchez (Gerrit)
Miguel Casas-Sanchez voted and added 1 comment![]( "Open in Gerrit")
Votes added by Miguel Casas-Sanchez
| Code-Review | +1 |
1 comment
Low risk and clarifies the build. LGTM!
Related details
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nico Weber
| Code-Review | +1 |
2 comments
This LG, but does use_linux_video_acceleration have to be an arg in https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/args.gni;l=59?q=use_linux_video_acceleration%5C%20%3D%20case:yes ? That adds yet another state since that can be toggled independently of the other two.
Could we just put `use_linux_video_acceleration = use_vaapi || use_v4l2_codec` at the toplevel in that gni file outside of declare_args(), or do you really want people to be able to set that independent of the other two? If so, could we remove the other two args?
We have too many gn args and should be wary of adding more.
if (enable_av1_decoder && ((is_chromeos && (use_linux_video_acceleration)) ||nit: drop now-needless parens around use_linux_video_acc
Related details
- Pilar Molina Lopez
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber voted Owners-Override+1![]( "Open in Gerrit")
| Owners-Override | +1 |
Related details
- Pilar Molina Lopez
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vikram Pasupathy (Gerrit)
Vikram Pasupathy added 2 comments![]( "Open in Gerrit")
This LG, but does use_linux_video_acceleration have to be an arg in https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/args.gni;l=59?q=use_linux_video_acceleration%5C%20%3D%20case:yes ? That adds yet another state since that can be toggled independently of the other two.
Could we just put `use_linux_video_acceleration = use_vaapi || use_v4l2_codec` at the toplevel in that gni file outside of declare_args(), or do you really want people to be able to set that independent of the other two? If so, could we remove the other two args?
We have too many gn args and should be wary of adding more.
@Miguel you're the Owner for this, do we allow people to be able to set that independent of the other two? Nico's suggestion makes sense to me, but just want to double check.
If so, would the same suggestion apply to `use_vaapi_image_codecs`?
if (enable_av1_decoder && ((is_chromeos && (use_linux_video_acceleration)) ||nit: drop now-needless parens around use_linux_video_acc
- Miguel Casas-Sanchez
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Miguel Casas-Sanchez (Gerrit)
Miguel Casas-Sanchez voted and added 2 comments![]( "Open in Gerrit")
Votes added by Miguel Casas-Sanchez
| Code-Review | +1 |
Vikram PasupathyThis LG, but does use_linux_video_acceleration have to be an arg in https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/args.gni;l=59?q=use_linux_video_acceleration%5C%20%3D%20case:yes ? That adds yet another state since that can be toggled independently of the other two.
Could we just put `use_linux_video_acceleration = use_vaapi || use_v4l2_codec` at the toplevel in that gni file outside of declare_args(), or do you really want people to be able to set that independent of the other two? If so, could we remove the other two args?
We have too many gn args and should be wary of adding more.
@Miguel you're the Owner for this, do we allow people to be able to set that independent of the other two? Nico's suggestion makes sense to me, but just want to double check.
If so, would the same suggestion apply to `use_vaapi_image_codecs`?
I don't think it would make sense to enable `use_linux_video_acceleration`
without either the vaapi or v4l2 gn args. If the question is whether the
former can be moved to the same declare_args() as the latter two (i.e.
into [1]) then the answer is - likely yes, it might simply not have
been considered before.
Related details
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nico Weber
| Code-Review | +1 |
| Owners-Override | +1 |
Vikram PasupathyThis LG, but does use_linux_video_acceleration have to be an arg in https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/args.gni;l=59?q=use_linux_video_acceleration%5C%20%3D%20case:yes ? That adds yet another state since that can be toggled independently of the other two.
Could we just put `use_linux_video_acceleration = use_vaapi || use_v4l2_codec` at the toplevel in that gni file outside of declare_args(), or do you really want people to be able to set that independent of the other two? If so, could we remove the other two args?
We have too many gn args and should be wary of adding more.
Miguel Casas-Sanchez@Miguel you're the Owner for this, do we allow people to be able to set that independent of the other two? Nico's suggestion makes sense to me, but just want to double check.
If so, would the same suggestion apply to `use_vaapi_image_codecs`?
I don't think it would make sense to enable `use_linux_video_acceleration`
without either the vaapi or v4l2 gn args. If the question is whether the
former can be moved to the same declare_args() as the latter two (i.e.
into [1]) then the answer is - likely yes, it might simply not have
been considered before.
The question is "does it have to be an arg at all". Concretely, would this work:
```diff
diff --git a/media/gpu/args.gni b/media/gpu/args.gni
index 1cf51555d3348..22ecee80b5cc2 100644
--- a/media/gpu/args.gni
+++ b/media/gpu/args.gni
@@ -55,9 +55,7 @@ declare_args() {
use_vaapi_image_codecs = use_vaapi && is_chromeos
}
-declare_args() {
- use_linux_video_acceleration = use_vaapi || use_v4l2_codec
-}
+use_linux_video_acceleration = use_vaapi || use_v4l2_codec
declare_args() {
# A platform that is capable of hardware av1 decoding.
```
Related details
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber added 1 comment![]( "Open in Gerrit")
(as in, "would it work for you". On a GN level, it does work.)
Related details
- Vikram Pasupathy
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |