Syed AbuTalib (Gerrit)

unread,

Feb 11, 2026, 6:42:56 PM (9 hours ago) Feb 11

to Thomas Guilbert, chromium...@chromium.org, feature-me...@chromium.org

Attention needed from Thomas Guilbert

Syed AbuTalib voted Commit-Queue+1

Commit-Queue

+1

Open in Gerrit

Related details

Attention is currently required from:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Thomas Guilbert (Gerrit)

unread,

Feb 11, 2026, 6:52:13 PM (9 hours ago) Feb 11

to Syed AbuTalib, Chromium LUCI CQ, Thomas Guilbert, chromium...@chromium.org, feature-me...@chromium.org

Attention needed from Syed AbuTalib

Thomas Guilbert added 3 comments

Commit Message

Line 7, Patchset 2 (Latest):

Add runtime version of ChannelLayoutConfig::FromLayout()

Bug: 483461268
Change-Id: Ib37b8e0ec24926b8eb0bba026bfc1b0f6b7a893c

Thomas Guilbert . unresolved

Adding a tiny description about how this will help reduce `{foo_layout, ChannelLayoutToChannelCount(foo_layout)}` would be helpful.

File media/base/channel_layout_unittest.cc

Line 167, Patchset 2 (Latest): auto quad_layout = CHANNEL_LAYOUT_QUAD;

Thomas Guilbert . unresolved

Throughout, I'm not sure that using `auto foo_layout = CHANNEL_LAYOUT_FOO` is adding to the readability or maintainability.

Inlining all the layouts seems fine to me.

Line 168, Patchset 2 (Latest): auto quad_layout_config = ChannelLayoutConfig::FromLayout(quad_layout);

Thomas Guilbert . unresolved

NIT: Add const throughout.

Open in Gerrit

Related details

Attention is currently required from:

Syed AbuTalib

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Syed AbuTalib (Gerrit)

unread,

Feb 11, 2026, 7:14:22 PM (8 hours ago) Feb 11

to Chromium LUCI CQ, Thomas Guilbert, chromium...@chromium.org, feature-me...@chromium.org

Attention needed from Thomas Guilbert

Syed AbuTalib voted and added 3 comments

Votes added by Syed AbuTalib

Commit-Queue

+1

3 comments

Commit Message

Line 7, Patchset 2:

Add runtime version of ChannelLayoutConfig::FromLayout()

Bug: 483461268
Change-Id: Ib37b8e0ec24926b8eb0bba026bfc1b0f6b7a893c

Thomas Guilbert . resolved

Adding a tiny description about how this will help reduce `{foo_layout, ChannelLayoutToChannelCount(foo_layout)}` would be helpful.

Syed AbuTalib

Done

File media/base/channel_layout_unittest.cc

Line 167, Patchset 2: auto quad_layout = CHANNEL_LAYOUT_QUAD;

Thomas Guilbert . resolved

Throughout, I'm not sure that using `auto foo_layout = CHANNEL_LAYOUT_FOO` is adding to the readability or maintainability.
Inlining all the layouts seems fine to me.

Syed AbuTalib

Done

Line 168, Patchset 2: auto quad_layout_config = ChannelLayoutConfig::FromLayout(quad_layout);

Thomas Guilbert . unresolved

NIT: Add const throughout.

Syed AbuTalib

Ended up inlining the two-check layouts (e.g. quad) as well, since ChannelLayoutConfig should be cheap to make. Let me know if I should change it back (+ add const).

Open in Gerrit

Related details

Attention is currently required from:

Thomas Guilbert

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Thomas Guilbert (Gerrit)

unread,

Feb 11, 2026, 7:40:14 PM (8 hours ago) Feb 11

to Syed AbuTalib, Thomas Guilbert, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org

Attention needed from Syed AbuTalib

Thomas Guilbert voted and added 2 comments

Votes added by Thomas Guilbert

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Thomas Guilbert . resolved

LGTM % comment.

File media/base/channel_layout_unittest.cc

Line 168, Patchset 2: auto quad_layout_config = ChannelLayoutConfig::FromLayout(quad_layout);

Thomas Guilbert . unresolved

NIT: Add const throughout.

Syed AbuTalib

Ended up inlining the two-check layouts (e.g. quad) as well, since ChannelLayoutConfig should be cheap to make. Let me know if I should change it back (+ add const).

Thomas Guilbert

I think the following is cleanest:
```
const auto quad_layout =
  ChannelLayougConfig::FromLayout(CHANNEL_LAYOUT_QUAD);
EXPECT_EQ(CHANNEL_LAYOUT_QUAD, quad_layout.channel_layout());
EXPECT_EQ(4, quad_layout.channels());
```

Open in Gerrit

Related details

Attention is currently required from:

Syed AbuTalib

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Syed AbuTalib (Gerrit)

unread,

Feb 11, 2026, 7:50:40 PM (8 hours ago) Feb 11

to Thomas Guilbert, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org

Syed AbuTalib voted and added 1 comment

Votes added by Syed AbuTalib

Commit-Queue

+2

1 comment

File media/base/channel_layout_unittest.cc

Line 168, Patchset 2: auto quad_layout_config = ChannelLayoutConfig::FromLayout(quad_layout);

Thomas Guilbert . resolved

NIT: Add const throughout.

Syed AbuTalib

Ended up inlining the two-check layouts (e.g. quad) as well, since ChannelLayoutConfig should be cheap to make. Let me know if I should change it back (+ add const).

Thomas Guilbert

I think the following is cleanest:
```
const auto quad_layout =
  ChannelLayougConfig::FromLayout(CHANNEL_LAYOUT_QUAD);
EXPECT_EQ(CHANNEL_LAYOUT_QUAD, quad_layout.channel_layout());
EXPECT_EQ(4, quad_layout.channels());
```

Syed AbuTalib

Done

Open in Gerrit

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.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Feb 11, 2026, 9:38:08 PM (6 hours ago) Feb 11

to Syed AbuTalib, Thomas Guilbert, chromium...@chromium.org, feature-me...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: media/base/channel_layout_unittest.cc
Insertions: 11, Deletions: 15.

@@ -162,26 +162,22 @@
   EXPECT_EQ(ChannelLayoutConfig::Stereo(),
             ChannelLayoutConfig::FromLayout(CHANNEL_LAYOUT_STEREO));
 
-  EXPECT_EQ(
-      CHANNEL_LAYOUT_QUAD,
-      ChannelLayoutConfig::FromLayout(CHANNEL_LAYOUT_QUAD).channel_layout());
-  EXPECT_EQ(4, ChannelLayoutConfig::FromLayout(CHANNEL_LAYOUT_QUAD).channels());
+  const auto quad_layout = ChannelLayoutConfig::FromLayout(CHANNEL_LAYOUT_QUAD);
+  EXPECT_EQ(CHANNEL_LAYOUT_QUAD, quad_layout.channel_layout());
+  EXPECT_EQ(4, quad_layout.channels());
 
   EXPECT_EQ(ChannelLayoutConfig(),
             ChannelLayoutConfig::FromLayout(CHANNEL_LAYOUT_NONE));
 
-  EXPECT_EQ(CHANNEL_LAYOUT_UNSUPPORTED,
-            ChannelLayoutConfig::FromLayout(CHANNEL_LAYOUT_UNSUPPORTED)
-                .channel_layout());
-  EXPECT_EQ(
-      0,
-      ChannelLayoutConfig::FromLayout(CHANNEL_LAYOUT_UNSUPPORTED).channels());
+  const auto unsupported_layout =
+      ChannelLayoutConfig::FromLayout(CHANNEL_LAYOUT_UNSUPPORTED);
+  EXPECT_EQ(CHANNEL_LAYOUT_UNSUPPORTED, unsupported_layout.channel_layout());
+  EXPECT_EQ(0, unsupported_layout.channels());
 
-  EXPECT_EQ(CHANNEL_LAYOUT_BITSTREAM,
-            ChannelLayoutConfig::FromLayout(CHANNEL_LAYOUT_BITSTREAM)
-                .channel_layout());
-  EXPECT_EQ(
-      0, ChannelLayoutConfig::FromLayout(CHANNEL_LAYOUT_BITSTREAM).channels());
+  const auto bitstream_layout =
+      ChannelLayoutConfig::FromLayout(CHANNEL_LAYOUT_BITSTREAM);
+  EXPECT_EQ(CHANNEL_LAYOUT_BITSTREAM, bitstream_layout.channel_layout());
+  EXPECT_EQ(0, bitstream_layout.channels());
 }
 
 #if GTEST_HAS_DEATH_TEST
```

Change information

Commit message:

Add runtime version of ChannelLayoutConfig::FromLayout()

There are several cases where we create a ChannelLayoutConfig doing
`{foo_layout, ChannelLayoutToChannelCount(foo_layout)}`. Adding a
nontemplated FromLayout method will clean these cases to be a simple
call.

Bug: 483461268

Change-Id: Ib37b8e0ec24926b8eb0bba026bfc1b0f6b7a893c

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7569128

Reviewed-by: Thomas Guilbert <tgui...@chromium.org>

Commit-Queue: Syed AbuTalib <low...@google.com>

Cr-Commit-Position: refs/heads/main@{#1583683}

Files:

M media/base/channel_layout.h
M media/base/channel_layout_unittest.cc

Change size: S

Delta: 2 files changed, 29 insertions(+), 0 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Thomas Guilbert

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

Add runtime version of ChannelLayoutConfig::FromLayout() [chromium/src : main]

Syed AbuTalib (Gerrit)

Syed AbuTalib voted Commit-Queue+1

Related details

Thomas Guilbert (Gerrit)

Thomas Guilbert added 3 comments

Related details

Syed AbuTalib (Gerrit)

Syed AbuTalib voted and added 3 comments

Votes added by Syed AbuTalib

3 comments

Related details

Thomas Guilbert (Gerrit)

Thomas Guilbert voted and added 2 comments

Votes added by Thomas Guilbert

2 comments

Related details

Syed AbuTalib (Gerrit)

Syed AbuTalib voted and added 1 comment

Votes added by Syed AbuTalib

1 comment

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information