Optimize IsDecoderConfigSupported for frequent calls [chromium/src : main]
0 views
Skip to first unread message
Ambareesh Balaji (Gerrit)
Nov 7, 2025, 9:14:44 PMNov 7
to Eugene Zemtsov, AyeAye, chromeos-gfx-...@google.com, media-cro...@chromium.org, feature-me...@chromium.org
Attention needed from Eugene Zemtsov
New activity on the change![]( "Open in Gerrit")
Related details
Attention is currently required from:
- Eugene Zemtsov
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Nov 11, 2025, 1:10:38 PMNov 11
to Ambareesh Balaji, Eugene Zemtsov, AyeAye, chromeos-gfx-...@google.com, media-cro...@chromium.org, feature-me...@chromium.org
Attention needed from Ambareesh Balaji
Dale Curtis voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
2 comments
File media/mojo/clients/mojo_codec_factory.h
Line 89, Patchset 3 (Latest):
media::GpuVideoAcceleratorFactories::SupportedDale Curtis . unresolved
Drop media:: on this new function -- it should be dropped in the whole file if you want to do that while you're here.
File media/mojo/clients/mojo_codec_factory.cc
Line 102, Patchset 3 (Latest):
for (const auto& supported : *supported_decoder_configs_) {Dale Curtis . unresolved
```
return std::ranges::any_of(supported_config, [&](const auto& c) {
return supported.Matches(c);
}) ? kTrue : kFalse
```
If you want.
Related details
Attention is currently required from:
- Ambareesh Balaji
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ambareesh Balaji (Gerrit)
Nov 11, 2025, 8:01:27 PMNov 11
to Dale Curtis, Eugene Zemtsov, AyeAye, chromeos-gfx-...@google.com, media-cro...@chromium.org, feature-me...@chromium.org
Attention needed from Dale Curtis
Ambareesh Balaji added 1 comment![]( "Open in Gerrit")
File media/mojo/clients/mojo_codec_factory.h
Line 89, Patchset 3 (Latest):
media::GpuVideoAcceleratorFactories::SupportedDale Curtis . unresolved
Drop media:: on this new function -- it should be dropped in the whole file if you want to do that while you're here.
Ambareesh Balaji
Should dropping media:: from the file be done in this CL or a different one?
Related details
Attention is currently required from:
- Dale Curtis
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Nov 12, 2025, 1:29:18 PMNov 12
to Ambareesh Balaji, Eugene Zemtsov, AyeAye, chromeos-gfx-...@google.com, media-cro...@chromium.org, feature-me...@chromium.org
Attention needed from Ambareesh Balaji
Dale Curtis added 1 comment![]( "Open in Gerrit")
File media/mojo/clients/mojo_codec_factory.h
Line 89, Patchset 3 (Latest):
media::GpuVideoAcceleratorFactories::SupportedDale Curtis . unresolved
Ambareesh BalajiDrop media:: on this new function -- it should be dropped in the whole file if you want to do that while you're here.
Should dropping media:: from the file be done in this CL or a different one?
Attention is currently required from:
- Ambareesh Balaji
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ambareesh Balaji (Gerrit)
Nov 12, 2025, 6:51:01 PMNov 12
to Dale Curtis, Eugene Zemtsov, AyeAye, chromeos-gfx-...@google.com, media-cro...@chromium.org, feature-me...@chromium.org
Attention needed from Dale Curtis
Ambareesh Balaji added 2 comments![]( "Open in Gerrit")
File media/mojo/clients/mojo_codec_factory.h
Ambareesh BalajiDrop media:: on this new function -- it should be dropped in the whole file if you want to do that while you're here.
Dale CurtisShould dropping media:: from the file be done in this CL or a different one?
Either is fine.
Ambareesh Balaji
Done
File media/mojo/clients/mojo_codec_factory.cc
Line 102, Patchset 3:
for (const auto& supported : *supported_decoder_configs_) {Dale Curtis . resolved
```
return std::ranges::any_of(supported_config, [&](const auto& c) {
return supported.Matches(c);
}) ? kTrue : kFalse
```If you want.
Attention is currently required from:
- Dale Curtis
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Nov 12, 2025, 7:04:06 PMNov 12
to Ambareesh Balaji, Eugene Zemtsov, AyeAye, chromeos-gfx-...@google.com, media-cro...@chromium.org, feature-me...@chromium.org
Attention needed from Ambareesh Balaji
Dale Curtis voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Ambareesh Balaji
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Nov 13, 2025, 12:39:27 AMNov 13
to Ambareesh Balaji, Thomas Guilbert, Dale Curtis, Eugene Zemtsov, AyeAye, chromeos-gfx-...@google.com, media-cro...@chromium.org, feature-me...@chromium.org
Attention needed from Ambareesh Balaji
Thomas Guilbert voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Guilbert
| Code-Review | +1 |
1 comment
File media/mojo/clients/mojo_codec_factory.h
Line 88, Patchset 5 (Latest):
GpuVideoAcceleratorFactories::Supported IsDecoderConfigSupported(Thomas Guilbert . resolved
TINY NIT: This could be a `std::optional<bool>` instead, if ever this was reused in a context that didn't care about the `GpuVideoAcceleratorFactories::Supported` type. Since there is one use, I won't hold up the review for this.
Related details
Attention is currently required from:
- Ambareesh Balaji
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Nov 13, 2025, 1:05:36 PMNov 13
to Ambareesh Balaji, Chromium LUCI CQ, Thomas Guilbert, Eugene Zemtsov, AyeAye, chromeos-gfx-...@google.com, media-cro...@chromium.org, feature-me...@chromium.org
Attention needed from Ambareesh Balaji
Dale Curtis added 1 comment![]( "Open in Gerrit")
File media/mojo/clients/mojo_codec_factory.h
Line 88, Patchset 5 (Latest):
GpuVideoAcceleratorFactories::Supported IsDecoderConfigSupported(Thomas Guilbert . resolved
TINY NIT: This could be a `std::optional<bool>` instead, if ever this was reused in a context that didn't care about the `GpuVideoAcceleratorFactories::Supported` type. Since there is one use, I won't hold up the review for this.
Dale Curtis
There's some debate on if `std::optional<bool>` is an improvement in readability over an enum. E.g., https://abseil.io/tips/141 -- so definitely something to do in a followup to see if replacing this enum is reasonable.
Related details
Attention is currently required from:
- Ambareesh Balaji
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ambareesh Balaji (Gerrit)
Nov 13, 2025, 3:08:04 PMNov 13
to Chromium LUCI CQ, Thomas Guilbert, Dale Curtis, Eugene Zemtsov, AyeAye, chromeos-gfx-...@google.com, media-cro...@chromium.org, feature-me...@chromium.org
Ambareesh Balaji added 1 comment![]( "Open in Gerrit")
File media/mojo/clients/mojo_codec_factory.h
Line 88, Patchset 5 (Latest):
GpuVideoAcceleratorFactories::Supported IsDecoderConfigSupported(Thomas Guilbert . resolved
Dale CurtisTINY NIT: This could be a `std::optional<bool>` instead, if ever this was reused in a context that didn't care about the `GpuVideoAcceleratorFactories::Supported` type. Since there is one use, I won't hold up the review for this.
There's some debate on if `std::optional<bool>` is an improvement in readability over an enum. E.g., https://abseil.io/tips/141 -- so definitely something to do in a followup to see if replacing this enum is reasonable.
Ambareesh Balaji
BTW there is already a TODO to have this turned into a non-optional `bool` in the parent class of `MojoGpuVideoAcceleratorFactories`: https://source.chromium.org/chromium/chromium/src/+/main:media/video/gpu_video_accelerator_factories.h;l=107
I think in a separate CL this can be turned into a `bool` after all the call sites preemptively check for the case that results in `kUnknown`.
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ambareesh Balaji (Gerrit)
Nov 20, 2025, 3:47:57 AMNov 20
to Chromium LUCI CQ, Thomas Guilbert, Dale Curtis, Eugene Zemtsov, AyeAye, chromeos-gfx-...@google.com, media-cro...@chromium.org, feature-me...@chromium.org
Attention needed from Dale Curtis and Thomas Guilbert
Ambareesh Balaji added 1 comment![]( "Open in Gerrit")
Attention is currently required from:
- Dale Curtis
- Thomas Guilbert
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Nov 20, 2025, 1:25:09 PMNov 20
to Ambareesh Balaji, Chromium LUCI CQ, Thomas Guilbert, Eugene Zemtsov, AyeAye, chromeos-gfx-...@google.com, media-cro...@chromium.org, feature-me...@chromium.org
Attention needed from Ambareesh Balaji and Thomas Guilbert
Dale Curtis added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Ambareesh Balaji . resolved
Is this good to submit now?
Attention is currently required from:
- Ambareesh Balaji
- Thomas Guilbert
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ambareesh Balaji (Gerrit)
Nov 20, 2025, 4:23:48 PMNov 20
to Chromium LUCI CQ, Thomas Guilbert, Dale Curtis, Eugene Zemtsov, AyeAye, chromeos-gfx-...@google.com, media-cro...@chromium.org, feature-me...@chromium.org
Attention needed from Dale Curtis
Ambareesh Balaji added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Ambareesh Balaji . resolved
Dale CurtisIs this good to submit now?
Yup!
Attention is currently required from:
- Dale Curtis
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eugene Zemtsov (Gerrit)
Nov 20, 2025, 4:26:11 PMNov 20
to Ambareesh Balaji, Eugene Zemtsov, Chromium LUCI CQ, Thomas Guilbert, Dale Curtis, AyeAye, chromeos-gfx-...@google.com, media-cro...@chromium.org, feature-me...@chromium.org
Attention needed from Ambareesh Balaji and Dale Curtis
Eugene Zemtsov voted Commit-Queue+2![]( "Open in Gerrit")
Attention is currently required from:
- Ambareesh Balaji
- Dale Curtis
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Nov 20, 2025, 5:39:39 PMNov 20
to Ambareesh Balaji, Eugene Zemtsov, Thomas Guilbert, Dale Curtis, AyeAye, chromeos-gfx-...@google.com, media-cro...@chromium.org, feature-me...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Optimize IsDecoderConfigSupported for frequent calls
Currently, each call to IsDecoderConfigSupported copies
|supported_decoder_configs_| from MojoCodecFactory, hence move the check
inside to avoid copies.
Bug: 458768128
Change-Id: Ib8928d22cf393034673fda9ee151904339b64bba
Reviewed-by: Thomas Guilbert <tgui...@chromium.org>
Commit-Queue: Eugene Zemtsov <eug...@chromium.org>
Reviewed-by: Dale Curtis <dalec...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1548085}
Files:
- M media/mojo/clients/mojo_codec_factory.cc
- M media/mojo/clients/mojo_codec_factory.h
- M media/mojo/clients/mojo_gpu_video_accelerator_factories.cc
Change size: M
Delta: 3 files changed, 64 insertions(+), 61 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Dale Curtis, +1 by Thomas Guilbert
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages