Enable Channel Mixing for DISCRETE with differing counts [chromium/src : main]
Syed AbuTalib (Gerrit)
Syed AbuTalib voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Syed AbuTalib
- Thomas Guilbert
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)
Thomas Guilbert added 5 comments![]( "Open in Gerrit")
Also, the unittest file was optimized for testing out a max of stereo
(2) layout, had to do a substantial rewrite in order to account for
higher channel counts. The changes include:
* Use ChannelMixingMatrix and precompute the output channel values
* Added convenience method `IsOutputChannelSilent()` to loop through all input channels
* Updated the WhichFlow enum
* Moved to a generic `GetFrequencyForChannel()` rather than having a left and right channel methods
* Added convenience method `VerifyTonesAt()` to loop through all the output channels
* Added convenience method `VerifySilenceInRange()` to loop through all the output channels for silence
* Updated tests to handle more than Stereo
* Added more TestParams, notably including cases such as 5.1 and differing `DISCRETE` cases (i.e. 1 vs 7).This is too much detail for the CL description. Folks should look at the actual CL for this.
At a high level, list the coverage you improved, rather than the list of mechanical changes made in the CL.
if (GetExpectedGain(in_ch, out_ch) >
std::numeric_limits<double>::epsilon()) {Necessary to use this precision rather than `!= 0`?
There might be a clearer std::ranges::all_of or equivalent expression here too.
GetFrequencyForChannel(WhichFlow::kInput));This loses some test coverage. Before, there was a distinct frequency for L/R, to identify the mixing worked correctly. Now, there won't be any way to know whether the L channel was copied to L and R accidentally, or if the R channel is dropped when downmixing to mono.
I know this is harder with the multi-channel configs (having 8 distinct frequencies to track would be hard, and you'd have to choose those carefully).
Maybe the full mixing is covered (or should be covered) by other UTs. Maybe we should keep some basic mixing check (e.g. L channel versus all other channels).
if (expected_vol <= std::numeric_limits<double>::epsilon()) {Is this overkill vs `== 0.0`?
testing::Values(
MakeParams(media::ChannelLayoutConfig::Stereo(),
48000,
480,
media::ChannelLayoutConfig::Stereo(),
48000,
480),
MakeParams(media::ChannelLayoutConfig::Stereo(),
48000,
64,
media::ChannelLayoutConfig::Stereo(),
48000,
480),
MakeParams(media::ChannelLayoutConfig::Stereo(),
44100,
64,
media::ChannelLayoutConfig::Stereo(),
48000,
480),
MakeParams(media::ChannelLayoutConfig::Stereo(),
48000,
512,
media::ChannelLayoutConfig::Stereo(),
44100,
441),
MakeParams(media::ChannelLayoutConfig::Mono(),
8000,
64,
media::ChannelLayoutConfig::Stereo(),
48000,
480),
MakeParams(media::ChannelLayoutConfig::Stereo(),
48000,
480,
media::ChannelLayoutConfig::Mono(),
8000,
80),
MakeParams(
media::ChannelLayoutConfig::FromLayout(media::CHANNEL_LAYOUT_5_1),
48000,
480,
media::ChannelLayoutConfig::FromLayout(media::CHANNEL_LAYOUT_5_1),
48000,
480),
MakeParams(
media::ChannelLayoutConfig::FromLayout(media::CHANNEL_LAYOUT_5_1),
48000,
480,
media::ChannelLayoutConfig::Stereo(),
48000,
480),
MakeParams(
media::ChannelLayoutConfig(media::CHANNEL_LAYOUT_DISCRETE, 1),
48000,
480,
media::ChannelLayoutConfig(media::CHANNEL_LAYOUT_DISCRETE, 7),
48000,
480)));Any way to clean this up? Most of the params are identical. It might be worth creating a "base param" and more varied helpers.
Maybe a builder pattern... I will send an example offline.
Related details
- Syed AbuTalib
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. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 5 comments![]( "Open in Gerrit")
Also, the unittest file was optimized for testing out a max of stereo
(2) layout, had to do a substantial rewrite in order to account for
higher channel counts. The changes include:
* Use ChannelMixingMatrix and precompute the output channel values
* Added convenience method `IsOutputChannelSilent()` to loop through all input channels
* Updated the WhichFlow enum
* Moved to a generic `GetFrequencyForChannel()` rather than having a left and right channel methods
* Added convenience method `VerifyTonesAt()` to loop through all the output channels
* Added convenience method `VerifySilenceInRange()` to loop through all the output channels for silence
* Updated tests to handle more than Stereo
* Added more TestParams, notably including cases such as 5.1 and differing `DISCRETE` cases (i.e. 1 vs 7).This is too much detail for the CL description. Folks should look at the actual CL for this.
At a high level, list the coverage you improved, rather than the list of mechanical changes made in the CL.
Done
if (GetExpectedGain(in_ch, out_ch) >
std::numeric_limits<double>::epsilon()) {Necessary to use this precision rather than `!= 0`?
There might be a clearer std::ranges::all_of or equivalent expression here too.
Just wanted to try out epsilon, comparing to 0.0 still works. I think `none_of ch[] > 0.0` is cleaner than `all_of ch[] != 0`, so going to do that instead. Marking as resolved.
GetFrequencyForChannel(WhichFlow::kInput));This loses some test coverage. Before, there was a distinct frequency for L/R, to identify the mixing worked correctly. Now, there won't be any way to know whether the L channel was copied to L and R accidentally, or if the R channel is dropped when downmixing to mono.
I know this is harder with the multi-channel configs (having 8 distinct frequencies to track would be hard, and you'd have to choose those carefully).
Maybe the full mixing is covered (or should be covered) by other UTs. Maybe we should keep some basic mixing check (e.g. L channel versus all other channels).
In my head I wanted to have [ChannelMixingMatrix verify correct mixing values](https://source.chromium.org/chromium/chromium/src/+/main:media/base/channel_mixing_matrix_unittest.cc) and have SnooperNode not worry about it, which is why I was fine with losing this coverage (also I tried 8 distinct freq but ran into slightly off values with [ComputeAmplitudeAt](https://source.chromium.org/chromium/chromium/src/+/main:services/audio/test/fake_consumer.cc;l=90;drc=fd1c7384931d505896bf0f20eb0c02117a3ed851). I think to bring back the 'original coverage' we could check for L vs *all* other channels. wdyt?
if (expected_vol <= std::numeric_limits<double>::epsilon()) {Is this overkill vs `== 0.0`?
Done
testing::Values(Any way to clean this up? Most of the params are identical. It might be worth creating a "base param" and more varied helpers.
Maybe a builder pattern... I will send an example offline.
- Thomas Guilbert
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. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 2 comments![]( "Open in Gerrit")
GetFrequencyForChannel(WhichFlow::kInput));Syed AbuTalibThis loses some test coverage. Before, there was a distinct frequency for L/R, to identify the mixing worked correctly. Now, there won't be any way to know whether the L channel was copied to L and R accidentally, or if the R channel is dropped when downmixing to mono.
I know this is harder with the multi-channel configs (having 8 distinct frequencies to track would be hard, and you'd have to choose those carefully).
Maybe the full mixing is covered (or should be covered) by other UTs. Maybe we should keep some basic mixing check (e.g. L channel versus all other channels).
In my head I wanted to have [ChannelMixingMatrix verify correct mixing values](https://source.chromium.org/chromium/chromium/src/+/main:media/base/channel_mixing_matrix_unittest.cc) and have SnooperNode not worry about it, which is why I was fine with losing this coverage (also I tried 8 distinct freq but ran into slightly off values with [ComputeAmplitudeAt](https://source.chromium.org/chromium/chromium/src/+/main:services/audio/test/fake_consumer.cc;l=90;drc=fd1c7384931d505896bf0f20eb0c02117a3ed851). I think to bring back the 'original coverage' we could check for L vs *all* other channels. wdyt?
Having `ChannelMixingMatrix` verify correct mixing makes sense, as long as it does verify that correct mixing.
Maybe keeping 1 simple stereo test as a guardrail would be worth it.
I'm not surprised that `ComputeAmplitudeAt` runs into issues... The 8 frequencies would have to be relatively prime to each other (e.g. checking the amplitude of a 600hz could be affected by mixing in some 200hz from another channel), and there needs to be enough frames to contain the period of each frequency.
class TestParamsBuilder {Discussed offline: only build `AudioParameters` rather than `InputAndOutputParams`
Related details
- Syed AbuTalib
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. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 2 comments![]( "Open in Gerrit")
GetFrequencyForChannel(WhichFlow::kInput));Syed AbuTalibThis loses some test coverage. Before, there was a distinct frequency for L/R, to identify the mixing worked correctly. Now, there won't be any way to know whether the L channel was copied to L and R accidentally, or if the R channel is dropped when downmixing to mono.
I know this is harder with the multi-channel configs (having 8 distinct frequencies to track would be hard, and you'd have to choose those carefully).
Maybe the full mixing is covered (or should be covered) by other UTs. Maybe we should keep some basic mixing check (e.g. L channel versus all other channels).
Thomas GuilbertIn my head I wanted to have [ChannelMixingMatrix verify correct mixing values](https://source.chromium.org/chromium/chromium/src/+/main:media/base/channel_mixing_matrix_unittest.cc) and have SnooperNode not worry about it, which is why I was fine with losing this coverage (also I tried 8 distinct freq but ran into slightly off values with [ComputeAmplitudeAt](https://source.chromium.org/chromium/chromium/src/+/main:services/audio/test/fake_consumer.cc;l=90;drc=fd1c7384931d505896bf0f20eb0c02117a3ed851). I think to bring back the 'original coverage' we could check for L vs *all* other channels. wdyt?
Having `ChannelMixingMatrix` verify correct mixing makes sense, as long as it does verify that correct mixing.
Maybe keeping 1 simple stereo test as a guardrail would be worth it.
I'm not surprised that `ComputeAmplitudeAt` runs into issues... The 8 frequencies would have to be relatively prime to each other (e.g. checking the amplitude of a 600hz could be affected by mixing in some 200hz from another channel), and there needs to be enough frames to contain the period of each frequency.
Added a new unittest labeled `StereoIsMixedCorrectly`.
class TestParamsBuilder {Discussed offline: only build `AudioParameters` rather than `InputAndOutputParams`
- Thomas Guilbert
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)
Thomas Guilbert added 5 comments![]( "Open in Gerrit")
// Check that FeatureList is available as a protection against early startup
// crashes.
if (base::FeatureList::GetInstance() &&If you're seeing this crash in the wild, better to fix it in its own CL.
Is there something missing from the test environment otherwise?
enum class WhichFlow {
kInput,
kSwappedInput,
kOutput,
kSwappedOutput,
};
double GetFrequencyForChannel(WhichFlow flow) const {
switch (flow) {
case WhichFlow::kInput:
case WhichFlow::kOutput:
return 500.0;
case WhichFlow::kSwappedInput:
case WhichFlow::kSwappedOutput:
return 1200.0;
}
}Now that all channels contain the same frequency, you would just use two constants directly?
channels_with_tones_.push_back(ch);Will `channels_with_tones_` always contain `0 ... ch-1`? Could you use input channels directly instead?
int frames_;
};Please fix this WARNING reported by ClangTidy: check: modernize-use-default-member-init
use default member initializer for 'fr...
check: modernize-use-default-member-init
use default member initializer for 'frames_' (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-default-member-init` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
MakeParams(BaseParams(), BaseParams()),Optional: consider adding a string param and a [NameGenerator](https://google.github.io/googletest/reference/testing.html#TYPED_TEST_SUITE) which makes it easier to figure out which test case is failing.
Related details
- Syed AbuTalib
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. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 4 comments![]( "Open in Gerrit")
// Check that FeatureList is available as a protection against early startup
// crashes.
if (base::FeatureList::GetInstance() &&If you're seeing this crash in the wild, better to fix it in its own CL.
Is there something missing from the test environment otherwise?
This isn't seen in the wild. It's an issue where media/ is accessed before FeatureList is registered.
```
[2284348:2284348:FATAL:base/feature_list.cc:110] Check failed: !feature. Accessed feature EnableHighChannelLayouts before FeatureList registration.
```
enum class WhichFlow {
kInput,
kSwappedInput,
kOutput,
kSwappedOutput,
};
double GetFrequencyForChannel(WhichFlow flow) const {
switch (flow) {
case WhichFlow::kInput:
case WhichFlow::kOutput:
return 500.0;
case WhichFlow::kSwappedInput:
case WhichFlow::kSwappedOutput:
return 1200.0;
}
}Now that all channels contain the same frequency, you would just use two constants directly?
Done
Will `channels_with_tones_` always contain `0 ... ch-1`? Could you use input channels directly instead?
Done
Please fix this WARNING reported by ClangTidy: check: modernize-use-default-member-init
use default member initializer for 'fr...
check: modernize-use-default-member-init
use default member initializer for 'frames_' (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-default-member-init` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
- Thomas Guilbert
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. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 3 comments![]( "Open in Gerrit")
// Check that FeatureList is available as a protection against early startup
// crashes.
if (base::FeatureList::GetInstance() &&Syed AbuTalibIf you're seeing this crash in the wild, better to fix it in its own CL.
Is there something missing from the test environment otherwise?
This isn't seen in the wild. It's an issue where media/ is accessed before FeatureList is registered.
```
[2284348:2284348:FATAL:base/feature_list.cc:110] Check failed: !feature. Accessed feature EnableHighChannelLayouts before FeatureList registration.
```
See other comment. You might be missing something in the test fixture.
Otherwise, the crash might be avoided, but you might always use 8 channels.
std::vector<std::vector<double>> expected_mix_matrix_;Should this be private?
scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;Does adding a `base::test::TaskEnvironment task_environment_` or a `base::test::ScopedFeatureList feature_list_` here fix the `base::Feature` crash? Note that these should be the first thing declared to make sure they are constructed first.
Related details
- Syed AbuTalib
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. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 2 comments![]( "Open in Gerrit")
std::vector<std::vector<double>> expected_mix_matrix_;Syed AbuTalibShould this be private?
Done
scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;Does adding a `base::test::TaskEnvironment task_environment_` or a `base::test::ScopedFeatureList feature_list_` here fix the `base::Feature` crash? Note that these should be the first thing declared to make sure they are constructed first.
I've tried adding the `TaskEnvironment`, `ScopedFeatureList` with an `.InitAndEnableFeature()` call, and both at the same time (including swapping the order) as the first member variable (above task runner). Still hitting the same check. Maybe I have to configure something with `TaskEnvironment`?
Related details
- Thomas Guilbert
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. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 1 comment![]( "Open in Gerrit")
scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;Syed AbuTalibDoes adding a `base::test::TaskEnvironment task_environment_` or a `base::test::ScopedFeatureList feature_list_` here fix the `base::Feature` crash? Note that these should be the first thing declared to make sure they are constructed first.
I've tried adding the `TaskEnvironment`, `ScopedFeatureList` with an `.InitAndEnableFeature()` call, and both at the same time (including swapping the order) as the first member variable (above task runner). Still hitting the same check. Maybe I have to configure something with `TaskEnvironment`?
I wonder if it's called during the test param construction then...
I'm worried about the case where two calls to `GetConcurrentMaxChannels()` would yield different results. The first runs into `!base::FeatureList::GetInstance()` and returns 8 before the feature subsystem is initialized, and the next returns 12.
If it's just tests, that's fine, but it might have other implications in production... However, I don't think we would run into this issue in production.
You will need a second reviewer for this CL anyways (I don't own `services/audio/*`) so maybe they'll have an idea?
Related details
- Syed AbuTalib
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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
// Check that FeatureList is available as a protection against early startup
// crashes.
if (base::FeatureList::GetInstance() &&Syed AbuTalibIf you're seeing this crash in the wild, better to fix it in its own CL.
Is there something missing from the test environment otherwise?
Thomas GuilbertThis isn't seen in the wild. It's an issue where media/ is accessed before FeatureList is registered.
```
[2284348:2284348:FATAL:base/feature_list.cc:110] Check failed: !feature. Accessed feature EnableHighChannelLayouts before FeatureList registration.
```
See other comment. You might be missing something in the test fixture.
Otherwise, the crash might be avoided, but you might always use 8 channels.
This is because we have a static initializer/global constructor (which btw is banned in Chrome :)
We should fix it.
scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;Syed AbuTalibDoes adding a `base::test::TaskEnvironment task_environment_` or a `base::test::ScopedFeatureList feature_list_` here fix the `base::Feature` crash? Note that these should be the first thing declared to make sure they are constructed first.
Thomas GuilbertI've tried adding the `TaskEnvironment`, `ScopedFeatureList` with an `.InitAndEnableFeature()` call, and both at the same time (including swapping the order) as the first member variable (above task runner). Still hitting the same check. Maybe I have to configure something with `TaskEnvironment`?
I wonder if it's called during the test param construction then...
I'm worried about the case where two calls to `GetConcurrentMaxChannels()` would yield different results. The first runs into `!base::FeatureList::GetInstance()` and returns 8 before the feature subsystem is initialized, and the next returns 12.
If it's just tests, that's fine, but it might have other implications in production... However, I don't think we would run into this issue in production.
You will need a second reviewer for this CL anyways (I don't own `services/audio/*`) so maybe they'll have an idea?
This ended up being rather fascinating.
The root cause of this is SnnoperNodeTest is parameterized, and some of the test parameters are initialized by calling `media::ChannelLayoutConfig::FromLayout()`.
This ends up reaching `ChannelLayoutToChannelCount()`, which, of course, needs the feature flag.
This also happens quite early in gtest initialization:
```
1 services_unittests 0x0000000109aded60 base::debug::StackTrace::StackTrace(unsigned long) + 224
2 services_unittests 0x0000000106683d54 _ZZN5media24GetConcurrentMaxChannelsEvENK3$_0clEv + 64
3 services_unittests 0x0000000106683e90 media::ChannelLayoutToChannelCount(media::ChannelLayout) + 280
4 services_unittests 0x000000010573fe4c audio::(anonymous namespace)::gtest_AllSnooperNodeTest_EvalGenerator_() + 700
5 services_unittests 0x0000000105746968 testing::internal::ParameterizedTestSuiteInfo<audio::(anonymous namespace)::SnooperNodeTest>::RegisterTests() + 280
6 services_unittests 0x0000000106581758 testing::internal::UnitTestImpl::PostFlagParsingInit() + 156
7 services_unittests 0x0000000106583804 void testing::internal::InitGoogleTestImpl<char>(int*, char**) + 364
8 services_unittests 0x000000010af18788 base::(anonymous namespace)::InitGoogleTestChar(int*, char**) + 24
9 services_unittests 0x0000000104d8ede4 base::OnceCallback<void ()>::Run() && + 108
10 services_unittests 0x000000010af1855c base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, unsigned long, bool, base::RepeatingCallback<void ()>, base::OnceCallback<void ()>) + 812
11 services_unittests 0x000000010af1820c base::LaunchUnitTests(int, char**, base::OnceCallback<int ()>, unsigned long) + 236
12 services_unittests 0x0000000105678230 main + 444
13 dyld 0x00000001889b3da4 start + 6992
```
Feature flags actually aren't initialized until a bit later (for tests, this usually happens in https://source.chromium.org/chromium/chromium/src/+/main:base/test/test_suite.cc;l=650;drc=869d94f8d790d8e41aa248d18d83557853374dbf, I think). But the failing test in question is a **multiprocess** test, and its `main()` initializes a new feature list:
https://source.chromium.org/chromium/chromium/src/+/main:services/tracing/public/cpp/trace_startup_shared_memory_unittest.cc;l=67;drc=b1397794111b060e366651f989f1b4386cbed797
There is a lot of [complex interactions between `FeatureList` and tests](https://source.chromium.org/chromium/chromium/src/+/main:base/feature_list.cc;l=626;drc=9aa846e4cb63098be56ba090a1d7dd328287da71), but in short, I think in tests, there are probably edge cases where we don't warn on feature access where we should.
I think the best solution here is to avoid creating parameterized types that depend on feature list flags, directly or indirectly. So defer the creation of the audio input params/output params (and thus `ChannelLayoutConfig`) until the test fixture is constructed; at that point, all the feature list machinery is live.
Related details
- Syed AbuTalib
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. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 2 comments![]( "Open in Gerrit")
// Check that FeatureList is available as a protection against early startup
// crashes.
if (base::FeatureList::GetInstance() &&Syed AbuTalibIf you're seeing this crash in the wild, better to fix it in its own CL.
Is there something missing from the test environment otherwise?
Thomas GuilbertThis isn't seen in the wild. It's an issue where media/ is accessed before FeatureList is registered.
```
[2284348:2284348:FATAL:base/feature_list.cc:110] Check failed: !feature. Accessed feature EnableHighChannelLayouts before FeatureList registration.
```
Daniel ChengSee other comment. You might be missing something in the test fixture.
Otherwise, the crash might be avoided, but you might always use 8 channels.
This is because we have a static initializer/global constructor (which btw is banned in Chrome :)
We should fix it.
I removed this `base::FeatureList::GetInstance()` call by delaying the construction of AudioParameters in the tests.
scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;Wow, amazing work on figuring out this issue! I agree with deferring creation of AudioParameters, passing an AudioParametersBuilder around and calling .Build() when needed. Thanks for taking a look!
Related details
- Thomas Guilbert
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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
// Check that FeatureList is available as a protection against early startup
// crashes.
if (base::FeatureList::GetInstance() &&Syed AbuTalibIf you're seeing this crash in the wild, better to fix it in its own CL.
Is there something missing from the test environment otherwise?
Thomas GuilbertThis isn't seen in the wild. It's an issue where media/ is accessed before FeatureList is registered.
```
[2284348:2284348:FATAL:base/feature_list.cc:110] Check failed: !feature. Accessed feature EnableHighChannelLayouts before FeatureList registration.
```
Daniel ChengSee other comment. You might be missing something in the test fixture.
Otherwise, the crash might be avoided, but you might always use 8 channels.
Syed AbuTalibThis is because we have a static initializer/global constructor (which btw is banned in Chrome :)
We should fix it.
I removed this `base::FeatureList::GetInstance()` call by delaying the construction of AudioParameters in the tests.
(Sorry just to clarify, this comment about static initializers is wrong. I had gemini-cli do some of the initial diganosis, but I was diving into it more myself and then forgot to delete this draft)
Thomas Guilbert (Gerrit)
Thomas Guilbert added 1 comment![]( "Open in Gerrit")
int channels() const { return channels_; }
int sample_rate() const { return rate_; }
int frames_per_buffer() const { return frames_; }
media::ChannelLayout channel_layout() const { return layout_; }Mixing the builder pattern and these getters is a bit odd. You could instead do a lazy `build()` in `InputAndOutputParams`, the first time `input()` or `output()` is called.
You could still use `AsHumanReadableString()` this way, in the `operator<<()` overload.
Related details
- Syed AbuTalib
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. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 2 comments![]( "Open in Gerrit")
int channels() const { return channels_; }
int sample_rate() const { return rate_; }
int frames_per_buffer() const { return frames_; }
media::ChannelLayout channel_layout() const { return layout_; }Mixing the builder pattern and these getters is a bit odd. You could instead do a lazy `build()` in `InputAndOutputParams`, the first time `input()` or `output()` is called.
You could still use `AsHumanReadableString()` this way, in the `operator<<()` overload.
Actually, I still hit the flag crash when I used `AsHumanReadableString()`, I'm thinking this `operator<<` gets called during the gtest initialization. I decided that since we have a NameGenerator now we can just use the new `std::string name` instead.
Optional: consider adding a string param and a [NameGenerator](https://google.github.io/googletest/reference/testing.html#TYPED_TEST_SUITE) which makes it easier to figure out which test case is failing.
Added a lambda to do this in the `INSTANTIATE_TEST_SUITE_P`
Related details
- Thomas Guilbert
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)
Thomas Guilbert voted and added 1 comment![]( "Open in Gerrit")
Votes added by Thomas Guilbert
| Code-Review | +1 |
1 comment
std::ostream& operator<<(std::ostream& out,
const InputAndOutputParams& test_params) {
return out << test_params.name;
}
Is this still needed at all with the name generator?
Related details
- Syed AbuTalib
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. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 2 comments![]( "Open in Gerrit")
Hey Daniel, hoping you could review this CL as an OWNER for `services/audio/*`. Thanks for helping out earlier!
std::ostream& operator<<(std::ostream& out,
const InputAndOutputParams& test_params) {
return out << test_params.name;
}
Is this still needed at all with the name generator?
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Hey Daniel, hoping you could review this CL as an OWNER for `services/audio/*`. Thanks for helping out earlier!
Ah... I am in OWNERS but generally only for help with landing refactorings and no-op changes. I /could/ review it but it would be better for an audio-specific owner to make sure everything is OK. I've added dalecurtis@ who should be a better reviewer than me here, but it's probably also worth considering if tguilbert would be an appropriate owner here.
Related details
- Dale Curtis
- Syed AbuTalib
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 1 comment![]( "Open in Gerrit")
Adding Dale as an OWNER of `services/audio/`.
Related details
- Dale Curtis
- Syed AbuTalib
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 1 comment![]( "Open in Gerrit")
Adding Dale as an OWNER of `services/audio/`.
Shoot, never reloaded the page so I didn't see DCheng already added Dale. Please ignore.
Related details
- Dale Curtis
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)
Dale Curtis voted Code-Review+1![]( "Open in Gerrit")
- Syed AbuTalib
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib voted Commit-Queue+2![]( "Open in Gerrit")
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)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Enable Channel Mixing for DISCRETE with differing counts
It is still worth using ChannelMixStrategy for `DISCRETE` cases as it is
not a guarantee that the input/output params have the same channel
count.
Also, the unittest file was optimized for testing out a max of stereo
(2) layout, updated the unittests to handle higher channel counts and
mixing between the higher channel counts.
- M services/audio/snooper_node.cc
- M services/audio/snooper_node_unittest.cc
Code-Review: +1 by Dale Curtis, +1 by Thomas Guilbert
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |