[media][effects] Add additional checks when adding an Effect.
This validates that an added effects parameters match what the
processor requires. This is a simplified implementation that requires
all Effects do in-place processing so all Effects will have the same
sample rate and channel configuration.
We also make the EffectsProcessor movable and expand on some
class-level documentation.
Test: fx run-test effexts_loader_unittests
Change-Id: Ia5e0318af6749e0a697feb30b411b4a28a213418
diff --git a/src/media/audio/lib/effects_loader/effects_processor.cc b/src/media/audio/lib/effects_loader/effects_processor.cc
index 7c808c9..da10664 100644
--- a/src/media/audio/lib/effects_loader/effects_processor.cc
+++ b/src/media/audio/lib/effects_loader/effects_processor.cc
@@ -8,10 +8,54 @@
namespace media::audio {
+EffectsProcessor::EffectsProcessor(EffectsProcessor&& o) noexcept
+ : effects_chain_(std::move(o.effects_chain_)),
+ channels_in_(o.channels_in_),
+ channels_out_(o.channels_out_) {
+ o.channels_in_ = 0;
+ o.channels_out_ = 0;
+}
+
+EffectsProcessor& EffectsProcessor::operator=(EffectsProcessor&& o) noexcept {
+ effects_chain_ = std::move(o.effects_chain_);
+ channels_in_ = o.channels_in_;
+ channels_out_ = o.channels_out_;
+ o.channels_in_ = 0;
+ o.channels_out_ = 0;
+ return *this;
+}
+
// Insert an effect instance at the end of the chain.
-void EffectsProcessor::AddEffect(Effect e) {
+zx_status_t EffectsProcessor::AddEffect(Effect e) {
FXL_DCHECK(e);
+
+ fuchsia_audio_effects_parameters params;
+ zx_status_t status = e.GetParameters(¶ms);
+ if (status != ZX_OK) {
+ return status;
+ }
+
+ // For now we only support in-place processors.
+ if (params.channels_in != params.channels_out) {
+ FXL_LOG(ERROR) << "Can't add effect; only in-place effects are currently supported.";
+ return ZX_ERR_INVALID_ARGS;
+ }
+
+ if (effects_chain_.empty()) {
+ // This is the first effect; the processors input channels will be whatever this effect
+ // accepts.
+ channels_in_ = params.channels_in;
+ } else if (params.channels_in != channels_out_) {
+ // We have existing effects and this effect excepts different channelization than what we're
+ // currently producing.
+ FXL_LOG(ERROR) << "Can't add effect; needs " << params.channels_in << " channels but have "
+ << channels_out_ << " channels";
+ return ZX_ERR_INVALID_ARGS;
+ }
+
+ channels_out_ = params.channels_out;
effects_chain_.emplace_back(std::move(e));
+ return ZX_OK;
}
// Aborts if position is out-of-range.
diff --git a/src/media/audio/lib/effects_loader/effects_processor.h b/src/media/audio/lib/effects_loader/effects_processor.h
index 807e69e..7591591 100644
--- a/src/media/audio/lib/effects_loader/effects_processor.h
+++ b/src/media/audio/lib/effects_loader/effects_processor.h
@@ -13,17 +13,54 @@
namespace media::audio {
-// EffectsProcessor represents a chain of active effect instances and manages chaining calls of
+// EffectsProcessor represents a queue of active effect instances and manages chaining calls of
// Process/ProcessInPlace through a chain of effects.
//
// This class is designed to be used synchronously and is not explicitly multi-thread-safe.
class EffectsProcessor {
public:
- void AddEffect(Effect e);
+ // Creates a new, empty effects processor.
+ EffectsProcessor() = default;
+
+ // Allow move.
+ EffectsProcessor(EffectsProcessor&& o) noexcept;
+ EffectsProcessor& operator=(EffectsProcessor&& o) noexcept;
+
+ // Disallow copy.
+ EffectsProcessor(const EffectsProcessor&) = delete;
+ EffectsProcessor& operator=(const EffectsProcessor&) = delete;
+
+ // Adds an `Effect` to the end of the queue of effects included in this processor.
+ //
+ // When the first `Effect` is added, that effects input channels becomes the input to the entire
+ // processor. Likewise that effects output channels becomes the processors output channels.
+ //
+ // When subsequent effects are added, the new effects input channels must match exactly the out-
+ // put channels of last added effect. The output channels for the processor will be updated to
+ // match the output channels of the newly added effect.
+ //
+ // Currently AddEffect will fail if an Effect has channels_in != channels_out (ex: all effects
+ // must do in-place processing). This is a short-term limitation that will be lifted in the
+ // future.
+ //
+ // Aborts if `e` is an invalid `Effect`.
+ zx_status_t AddEffect(Effect e);
// Returns the number of active instances in the enclosed effect chain.
[[nodiscard]] uint16_t size() const { return effects_chain_.size(); }
+ // Returns the number of input channels for this effect. This will be the number of channels
+ // expected for input frames to `ProcessInPlace`.
+ //
+ // Returns 0 if this processor has no effects.
+ [[nodiscard]] uint32_t channels_in() const { return channels_in_; }
+
+ // Returns the number of output channels for this effect. Currently this always equals
+ // `channels_in()`.
+ //
+ // Returns 0 if this processor has no effects.
+ [[nodiscard]] uint32_t channels_out() const { return channels_out_; }
+
[[nodiscard]] auto begin() { return effects_chain_.begin(); }
[[nodiscard]] auto end() { return effects_chain_.end(); }
[[nodiscard]] auto cbegin() const { return effects_chain_.cbegin(); }
@@ -38,6 +75,8 @@
private:
std::vector<Effect> effects_chain_;
+ uint32_t channels_in_ = 0;
+ uint32_t channels_out_ = 0;
};
} // namespace media::audio
diff --git a/src/media/audio/lib/effects_loader/effects_processor_unittest.cc b/src/media/audio/lib/effects_loader/effects_processor_unittest.cc
index 7472cc0..adaaefb 100644
--- a/src/media/audio/lib/effects_loader/effects_processor_unittest.cc
+++ b/src/media/audio/lib/effects_loader/effects_processor_unittest.cc
@@ -46,10 +46,10 @@
// Create processor
{
EffectsProcessor processor;
- processor.AddEffect(std::move(effect3));
- processor.AddEffect(std::move(effect1));
- processor.AddEffect(std::move(effect2));
- processor.AddEffect(std::move(effect4));
+ EXPECT_EQ(processor.AddEffect(std::move(effect3)), ZX_OK);
+ EXPECT_EQ(processor.AddEffect(std::move(effect1)), ZX_OK);
+ EXPECT_EQ(processor.AddEffect(std::move(effect2)), ZX_OK);
+ EXPECT_EQ(processor.AddEffect(std::move(effect4)), ZX_OK);
EXPECT_EQ(processor.size(), 4);
EXPECT_EQ(effects_handle3, processor.GetEffectAt(0).get());
@@ -65,6 +65,75 @@
test_effects()->clear_effects();
}
+TEST_F(EffectsProcessorTest, AddEffectWithMismatchedChannelConfig) {
+ ASSERT_EQ(ZX_OK, test_effects()->add_effect({{"assign_to_1.0", FUCHSIA_AUDIO_EFFECTS_CHANNELS_ANY,
+ FUCHSIA_AUDIO_EFFECTS_CHANNELS_SAME_AS_IN},
+ TEST_EFFECTS_ACTION_ASSIGN,
+ 1.0}));
+ Effect single_channel_effect1 = effects_loader()->CreateEffect(0, 1, 1, 1, {});
+ Effect single_channel_effect2 = effects_loader()->CreateEffect(0, 1, 1, 1, {});
+ Effect two_channel_effect = effects_loader()->CreateEffect(0, 1, 2, 2, {});
+
+ EffectsProcessor processor;
+ EXPECT_EQ(processor.channels_in(), 0u);
+ EXPECT_EQ(processor.channels_out(), 0u);
+
+ // Add a single channel effect (chans in == chans out == 1).
+ EXPECT_EQ(processor.AddEffect(std::move(single_channel_effect1)), ZX_OK);
+ EXPECT_EQ(processor.channels_in(), 1u);
+ EXPECT_EQ(processor.channels_out(), 1u);
+
+ // Add a second single channel effect.
+ EXPECT_EQ(processor.AddEffect(std::move(single_channel_effect2)), ZX_OK);
+ EXPECT_EQ(processor.channels_in(), 1u);
+ EXPECT_EQ(processor.channels_out(), 1u);
+
+ // Add a two channel effect. This should fail as the processor is currently producing single
+ // channel audio out of the last effect.
+ EXPECT_EQ(processor.AddEffect(std::move(two_channel_effect)), ZX_ERR_INVALID_ARGS);
+}
+
+TEST_F(EffectsProcessorTest, MoveProcessor) {
+ ASSERT_EQ(ZX_OK, test_effects()->add_effect({{"assign_to_1.0", FUCHSIA_AUDIO_EFFECTS_CHANNELS_ANY,
+ FUCHSIA_AUDIO_EFFECTS_CHANNELS_SAME_AS_IN},
+ TEST_EFFECTS_ACTION_ASSIGN,
+ 1.0}));
+ EffectsProcessor p1;
+ p1.AddEffect(effects_loader()->CreateEffect(0, 1, 1, 1, {}));
+ p1.AddEffect(effects_loader()->CreateEffect(0, 1, 1, 1, {}));
+
+ EXPECT_EQ(p1.size(), 2u);
+ EXPECT_EQ(p1.channels_in(), 1u);
+ EXPECT_EQ(p1.channels_out(), 1u);
+
+ EffectsProcessor p2;
+ EXPECT_EQ(p2.size(), 0u);
+ EXPECT_EQ(p2.channels_in(), 0u);
+ EXPECT_EQ(p2.channels_out(), 0u);
+
+ // Move assignment.
+ p2 = std::move(p1);
+ // p1 is now empty.
+ EXPECT_EQ(p1.size(), 0u);
+ EXPECT_EQ(p1.channels_in(), 0u);
+ EXPECT_EQ(p1.channels_out(), 0u);
+ // p2 has effects.
+ EXPECT_EQ(p2.size(), 2u);
+ EXPECT_EQ(p2.channels_in(), 1u);
+ EXPECT_EQ(p2.channels_out(), 1u);
+
+ // Move construction.
+ EffectsProcessor p3(std::move(p2));
+ // p2 is now empty.
+ EXPECT_EQ(p2.size(), 0u);
+ EXPECT_EQ(p2.channels_in(), 0u);
+ EXPECT_EQ(p2.channels_out(), 0u);
+ // p3 has effects.
+ EXPECT_EQ(p3.size(), 2u);
+ EXPECT_EQ(p3.channels_in(), 1u);
+ EXPECT_EQ(p3.channels_out(), 1u);
+}
+
// Verify (at a VERY Basic level) the methods that handle data flow.
TEST_F(EffectsProcessorTest, ProcessInPlaceFlush) {
ASSERT_EQ(ZX_OK,
@@ -106,10 +175,10 @@
Effect effect4 = effects_loader()->CreateEffect(3, 1, 1, 1, {});
ASSERT_TRUE(effect1 && effect2 && effect3 && effect4);
- processor.AddEffect(std::move(effect1));
- processor.AddEffect(std::move(effect2));
- processor.AddEffect(std::move(effect3));
- processor.AddEffect(std::move(effect4));
+ EXPECT_EQ(processor.AddEffect(std::move(effect1)), ZX_OK);
+ EXPECT_EQ(processor.AddEffect(std::move(effect2)), ZX_OK);
+ EXPECT_EQ(processor.AddEffect(std::move(effect3)), ZX_OK);
+ EXPECT_EQ(processor.AddEffect(std::move(effect4)), ZX_OK);
EXPECT_EQ(4u, test_effects()->num_instances());
// The first 2 processors will mutate data, but this will be clobbered by the 3rd processor which
