[media] Remove UseSingleNV12ForSoftwareGMB feature [chromium/src : main]
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala voted and added 1 comment![]( "Open in Gerrit")
Votes added by Saifuddin Hitawala
| Commit-Queue | +1 |
1 comment
Related details
- Colin Blundell
- Kyle Charbonneau
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kyle Charbonneau (Gerrit)
Kyle Charbonneau added 1 comment![]( "Open in Gerrit")
return false;Does this only need to return false for Chrome OS? Something should document this function is only needed for Chrome OS and that when `GateNV12GMBVideoFramesOnHWSupport` is removed the function should also be removed.
Related details
- Colin Blundell
- Saifuddin Hitawala
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 1 comment![]( "Open in Gerrit")
return false;Does this only need to return false for Chrome OS? Something should document this function is only needed for Chrome OS and that when `GateNV12GMBVideoFramesOnHWSupport` is removed the function should also be removed.
Good question! The only other platforms I can think of is Android and Cast OS. Android shouldn't reach here for multiplanar. I'm not really sure about Cast OS (and its variations). Should we just return true for Cast OS here and use SINGLE_NV12? +vasilyt@ for inputs as well.
Also, I can definitely add a comment near the method stating we only need for ChromeOS and should be deleted.
Related details
- Colin Blundell
- Kyle Charbonneau
- Vasiliy Telezhnikov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell voted and added 2 comments![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
2 comments
Thanks!
return false;Saifuddin HitawalaDoes this only need to return false for Chrome OS? Something should document this function is only needed for Chrome OS and that when `GateNV12GMBVideoFramesOnHWSupport` is removed the function should also be removed.
Good question! The only other platforms I can think of is Android and Cast OS. Android shouldn't reach here for multiplanar. I'm not really sure about Cast OS (and its variations). Should we just return true for Cast OS here and use SINGLE_NV12? +vasilyt@ for inputs as well.
Also, I can definitely add a comment near the method stating we only need for ChromeOS and should be deleted.
CastOS [is a subset of Linux](https://source.chromium.org/chromium/chromium/src/+/main:build/config/cast.gni;l=117?q=build%2Fconfig%2Fcast.gni&ss=chromium), so we don't need to worry about that. I think that in an immediate followup to this CL we can eliminate this function and move the returning of NV12_DUAL_GMB on ChromeOS to be just above [here](https://source.chromium.org/chromium/chromium/src/+/main:content/renderer/media/gpu/gpu_video_accelerator_factories_impl.cc;l=395;drc=d311b843fa6d3cfd69f650d05cd44ace6aaa968d;bpv=1;bpt=1). Then everything will be set up for getting rid of NV12_DUAL_GMB once that killswitch can be removed.
Related details
- Kyle Charbonneau
- Saifuddin Hitawala
- Vasiliy Telezhnikov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kyle Charbonneau (Gerrit)
Kyle Charbonneau added 1 comment![]( "Open in Gerrit")
return false;Saifuddin HitawalaDoes this only need to return false for Chrome OS? Something should document this function is only needed for Chrome OS and that when `GateNV12GMBVideoFramesOnHWSupport` is removed the function should also be removed.
Colin BlundellGood question! The only other platforms I can think of is Android and Cast OS. Android shouldn't reach here for multiplanar. I'm not really sure about Cast OS (and its variations). Should we just return true for Cast OS here and use SINGLE_NV12? +vasilyt@ for inputs as well.
Also, I can definitely add a comment near the method stating we only need for ChromeOS and should be deleted.
CastOS [is a subset of Linux](https://source.chromium.org/chromium/chromium/src/+/main:build/config/cast.gni;l=117?q=build%2Fconfig%2Fcast.gni&ss=chromium), so we don't need to worry about that. I think that in an immediate followup to this CL we can eliminate this function and move the returning of NV12_DUAL_GMB on ChromeOS to be just above [here](https://source.chromium.org/chromium/chromium/src/+/main:content/renderer/media/gpu/gpu_video_accelerator_factories_impl.cc;l=395;drc=d311b843fa6d3cfd69f650d05cd44ace6aaa968d;bpv=1;bpt=1). Then everything will be set up for getting rid of NV12_DUAL_GMB once that killswitch can be removed.
I think that in an immediate followup to this CL we can eliminate this function and move the returning of NV12_DUAL_GMB on ChromeOS to be just above here. Then everything will be set up for getting rid of NV12_DUAL_GMB once that killswitch can be removed.
Any reason not to do it in this CL? Seems equivalent to me if this only applies to Chrome OS.
Related details
- Colin Blundell
- Saifuddin Hitawala
- Vasiliy Telezhnikov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
return false;Saifuddin HitawalaDoes this only need to return false for Chrome OS? Something should document this function is only needed for Chrome OS and that when `GateNV12GMBVideoFramesOnHWSupport` is removed the function should also be removed.
Colin BlundellGood question! The only other platforms I can think of is Android and Cast OS. Android shouldn't reach here for multiplanar. I'm not really sure about Cast OS (and its variations). Should we just return true for Cast OS here and use SINGLE_NV12? +vasilyt@ for inputs as well.
Also, I can definitely add a comment near the method stating we only need for ChromeOS and should be deleted.
Kyle CharbonneauCastOS [is a subset of Linux](https://source.chromium.org/chromium/chromium/src/+/main:build/config/cast.gni;l=117?q=build%2Fconfig%2Fcast.gni&ss=chromium), so we don't need to worry about that. I think that in an immediate followup to this CL we can eliminate this function and move the returning of NV12_DUAL_GMB on ChromeOS to be just above [here](https://source.chromium.org/chromium/chromium/src/+/main:content/renderer/media/gpu/gpu_video_accelerator_factories_impl.cc;l=395;drc=d311b843fa6d3cfd69f650d05cd44ace6aaa968d;bpv=1;bpt=1). Then everything will be set up for getting rid of NV12_DUAL_GMB once that killswitch can be removed.
I think that in an immediate followup to this CL we can eliminate this function and move the returning of NV12_DUAL_GMB on ChromeOS to be just above here. Then everything will be set up for getting rid of NV12_DUAL_GMB once that killswitch can be removed.
Any reason not to do it in this CL? Seems equivalent to me if this only applies to Chrome OS.
- Kyle Charbonneau
- Saifuddin Hitawala
- Vasiliy Telezhnikov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 2 comments![]( "Open in Gerrit")
Saifuddin HitawalaDoes this only need to return false for Chrome OS? Something should document this function is only needed for Chrome OS and that when `GateNV12GMBVideoFramesOnHWSupport` is removed the function should also be removed.
Colin BlundellGood question! The only other platforms I can think of is Android and Cast OS. Android shouldn't reach here for multiplanar. I'm not really sure about Cast OS (and its variations). Should we just return true for Cast OS here and use SINGLE_NV12? +vasilyt@ for inputs as well.
Also, I can definitely add a comment near the method stating we only need for ChromeOS and should be deleted.
Kyle CharbonneauCastOS [is a subset of Linux](https://source.chromium.org/chromium/chromium/src/+/main:build/config/cast.gni;l=117?q=build%2Fconfig%2Fcast.gni&ss=chromium), so we don't need to worry about that. I think that in an immediate followup to this CL we can eliminate this function and move the returning of NV12_DUAL_GMB on ChromeOS to be just above [here](https://source.chromium.org/chromium/chromium/src/+/main:content/renderer/media/gpu/gpu_video_accelerator_factories_impl.cc;l=395;drc=d311b843fa6d3cfd69f650d05cd44ace6aaa968d;bpv=1;bpt=1). Then everything will be set up for getting rid of NV12_DUAL_GMB once that killswitch can be removed.
I think that in an immediate followup to this CL we can eliminate this function and move the returning of NV12_DUAL_GMB on ChromeOS to be just above here. Then everything will be set up for getting rid of NV12_DUAL_GMB once that killswitch can be removed.
Any reason not to do it in this CL? Seems equivalent to me if this only applies to Chrome OS.
Thanks for clarifying! I didn't know CastOS was under Linux. I've updated to remove the method entirely and return DualNV12 for ChromeOS if feature is disabled.
Related details
- Kyle Charbonneau
- Vasiliy Telezhnikov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kyle Charbonneau (Gerrit)
Kyle Charbonneau voted Code-Review+1![]( "Open in Gerrit")
- Saifuddin Hitawala
- Vasiliy Telezhnikov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell voted and added 2 comments![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
2 comments
Thanks!
// Use NV12_SINGLE_GMB for Mac, Windows, Linux and CastOS platforms.nit: Looking at the structure of this code now, I think it's actually significantly more clear to just do another #if-#else here rather than adding the "else if" above:
#if BUILDFLAG(IS_CHROMEOS)
return OutputFormat::NV12_DUAL_GMB;
#else
return OutputFormat::NV12_SINGLE_GMB;
#endif
Apologies for the churn!
Related details
- Saifuddin Hitawala
- Vasiliy Telezhnikov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala voted and added 2 comments![]( "Open in Gerrit")
Votes added by Saifuddin Hitawala
| Commit-Queue | +2 |
2 comments
// Use NV12_SINGLE_GMB for Mac, Windows, Linux and CastOS platforms.nit: Looking at the structure of this code now, I think it's actually significantly more clear to just do another #if-#else here rather than adding the "else if" above:
#if BUILDFLAG(IS_CHROMEOS)
return OutputFormat::NV12_DUAL_GMB;
#else
return OutputFormat::NV12_SINGLE_GMB;
#endifApologies for the churn!
- Vasiliy Telezhnikov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: content/renderer/media/gpu/gpu_video_accelerator_factories_impl.cc
Insertions: 6, Deletions: 4.
@@ -362,16 +362,18 @@
if (base::FeatureList::IsEnabled(
features::kGateNV12GMBVideoFramesOnHWSupport)) {
return OutputFormat::UNDEFINED;
- } else if (capabilities.texture_rg) {
- // TODO(crbug.com/40283225): NV12_DUAL_GMB is used on ChromeOS only if above
- // feature is disabled. Remove this codepath once above feature is launched.
- return OutputFormat::NV12_DUAL_GMB;
}
#endif
if (capabilities.texture_rg) {
+#if BUILDFLAG(IS_CHROMEOS)
+ // TODO(crbug.com/40283225): NV12_DUAL_GMB is used on ChromeOS only if above
+ // feature is disabled. Remove this codepath once above feature is launched.
+ return OutputFormat::NV12_DUAL_GMB;
+#else
// Use NV12_SINGLE_GMB for Mac, Windows, Linux and CastOS platforms.
return OutputFormat::NV12_SINGLE_GMB;
+#endif
}
return OutputFormat::UNDEFINED;
#endif // BUILDFLAG(IS_FUCHSIA)
```
Change information
[media] Remove UseSingleNV12ForSoftwareGMB feature
This feature is really not needed as for ChromeOS we fallback to
VideoResourceUpdater with the GateNV12GMBVideoFramesOnHWSupport
feature.
Also remove the corresponding UseSingleNV12 method and simplify
to return the correct OutputFormat.
- M content/renderer/media/gpu/gpu_video_accelerator_factories_impl.cc
Code-Review: +1 by Colin Blundell, +1 by Kyle Charbonneau
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |