Skip to content

Commit

Permalink
Merge pull request #13240 from Swiftb0y/refactor/gh13222-remove-refer…
Browse files Browse the repository at this point in the history
…enceholder

Refactor/gh13222 remove referenceholder
  • Loading branch information
ywwg authored Aug 27, 2024
2 parents 3c6e23f + d50eae4 commit 400ae46
Show file tree
Hide file tree
Showing 11 changed files with 83 additions and 135 deletions.
1 change: 0 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1443,7 +1443,6 @@ set(MIXXX_LIB_PRECOMPILED_HEADER
src/util/rampingvalue.h
src/util/rangelist.h
src/util/readaheadsamplebuffer.h
src/util/reference.h
src/util/regex.h
src/util/rescaler.h
src/util/ringdelaybuffer.h
Expand Down
7 changes: 5 additions & 2 deletions src/effects/backends/builtin/balanceeffect.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,13 @@ class BalanceGroupState : public EffectState {

class BalanceEffect : public EffectProcessorImpl<BalanceGroupState> {
public:
// TODO re-evaluate default/delete here. Is adhering to rule of zero possible?
BalanceEffect() = default;
~BalanceEffect() override = default;
BalanceEffect(const BalanceEffect&) = delete;
BalanceEffect& operator=(const BalanceEffect&) = delete;
BalanceEffect(BalanceEffect&&) = delete;
BalanceEffect& operator=(BalanceEffect&&) = delete;

static QString getId();
static EffectManifestPointer getManifest();
Expand All @@ -52,6 +57,4 @@ class BalanceEffect : public EffectProcessorImpl<BalanceGroupState> {
EngineEffectParameterPointer m_pBalanceParameter;
EngineEffectParameterPointer m_pMidSideParameter;
EngineEffectParameterPointer m_pBypassFreqParameter;

DISALLOW_COPY_AND_ASSIGN(BalanceEffect);
};
9 changes: 4 additions & 5 deletions src/effects/effectsmanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,12 @@ EffectsManager::EffectsManager(

m_pBackendManager = EffectsBackendManagerPointer(new EffectsBackendManager());

auto [pRequestPipe, pResponsePipe] = TwoWayMessagePipe<EffectsRequest*,
EffectsResponse>::makeTwoWayMessagePipe(kEffectMessagePipeFifoSize,
auto [requestPipe, responsePipe] = makeTwoWayMessagePipe<EffectsRequest*,
EffectsResponse>(kEffectMessagePipeFifoSize,
kEffectMessagePipeFifoSize);

m_pMessenger = EffectsMessengerPointer(new EffectsMessenger(
std::move(pRequestPipe)));
m_pEngineEffectsManager = std::make_unique<EngineEffectsManager>(std::move(pResponsePipe));
m_pMessenger = EffectsMessengerPointer::create(std::move(requestPipe));
m_pEngineEffectsManager = std::make_unique<EngineEffectsManager>(std::move(responsePipe));

m_pEffectPresetManager = EffectPresetManagerPointer(
new EffectPresetManager(pConfig, m_pBackendManager));
Expand Down
21 changes: 6 additions & 15 deletions src/effects/effectsmessenger.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@
#include "util/make_const_iterator.h"

EffectsMessenger::EffectsMessenger(
std::unique_ptr<EffectsRequestPipe> pRequestPipe)
: m_bShuttingDown(false),
m_pRequestPipe(std::move(pRequestPipe)),
m_nextRequestId(0) {
EffectsRequestPipe&& requestPipe)
: m_requestPipe(std::move(requestPipe)),
m_nextRequestId(0),
m_bShuttingDown(false) {
}

EffectsMessenger::~EffectsMessenger() {
Expand All @@ -28,18 +28,13 @@ bool EffectsMessenger::writeRequest(EffectsRequest* request) {
collectGarbage(request);
}

VERIFY_OR_DEBUG_ASSERT(m_pRequestPipe) {
delete request;
return false;
}

// This is effectively only garbage collection at this point so only deal
// with responses when writing new requests.
processEffectsResponses();

request->request_id = m_nextRequestId++;
// TODO(XXX) use preallocated requests to avoid delete calls from engine
if (m_pRequestPipe->writeMessage(request)) {
if (m_requestPipe.writeMessage(request)) {
m_activeRequests[request->request_id] = request;
return true;
}
Expand All @@ -48,12 +43,8 @@ bool EffectsMessenger::writeRequest(EffectsRequest* request) {
}

void EffectsMessenger::processEffectsResponses() {
VERIFY_OR_DEBUG_ASSERT(m_pRequestPipe) {
return;
}

EffectsResponse response;
while (m_pRequestPipe->readMessage(&response)) {
while (m_requestPipe.readMessage(&response)) {
auto it = m_activeRequests.constFind(response.request_id);

VERIFY_OR_DEBUG_ASSERT(it != m_activeRequests.constEnd()) {
Expand Down
10 changes: 5 additions & 5 deletions src/effects/effectsmessenger.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
/// for why this design is used for effects rather than alternatives.
class EffectsMessenger {
public:
EffectsMessenger(std::unique_ptr<EffectsRequestPipe> pRequestPipe);
// passing by rvalue-ref because we want to ensure we're the only on with access to that pipe
EffectsMessenger(EffectsRequestPipe&& requestPipe);
~EffectsMessenger();
/// Write an EffectsRequest to the EngineEffectsManager. EffectsMessenger takes
/// ownership of request and deletes it once a response is received.
Expand All @@ -30,9 +31,8 @@ class EffectsMessenger {
return "EffectsMessenger";
}

bool m_bShuttingDown;

std::unique_ptr<EffectsRequestPipe> m_pRequestPipe;
qint64 m_nextRequestId;
QHash<qint64, EffectsRequest*> m_activeRequests;
EffectsRequestPipe m_requestPipe;
qint64 m_nextRequestId;
bool m_bShuttingDown;
};
4 changes: 1 addition & 3 deletions src/engine/effects/engineeffect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,9 @@ EngineEffect::EngineEffect(EffectManifestPointer pManifest,
}

EngineEffect::~EngineEffect() {
if (kEffectDebugOutput) {
if constexpr (kEffectDebugOutput) {
qDebug() << debugString() << "destroyed";
}
m_parametersById.clear();
m_parameters.clear();
}

void EngineEffect::initalizeInputChannel(ChannelHandle inputChannel) {
Expand Down
2 changes: 1 addition & 1 deletion src/engine/effects/engineeffect.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class EngineEffect final : public EffectsRequestHandler {
const QSet<ChannelHandleAndGroup>& registeredInputChannels,
const QSet<ChannelHandleAndGroup>& registeredOutputChannels);
/// Called in main thread by EffectSlot
// Doesn't deal with ownership; only for conditional debug output
~EngineEffect();

/// Called from the main thread to make sure that the channel already has states
Expand Down Expand Up @@ -71,5 +72,4 @@ class EngineEffect final : public EffectsRequestHandler {
QVector<EngineEffectParameterPointer> m_parameters;
QMap<QString, EngineEffectParameterPointer> m_parametersById;

DISALLOW_COPY_AND_ASSIGN(EngineEffect);
};
14 changes: 7 additions & 7 deletions src/engine/effects/engineeffectsmanager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
#include "util/defs.h"
#include "util/sample.h"

EngineEffectsManager::EngineEffectsManager(std::unique_ptr<EffectsResponsePipe> pResponsePipe)
: m_pResponsePipe(std::move(pResponsePipe)),
EngineEffectsManager::EngineEffectsManager(EffectsResponsePipe&& responsePipe)
: m_responsePipe(std::move(responsePipe)),
m_buffer1(kMaxEngineSamples),
m_buffer2(kMaxEngineSamples) {
// Try to prevent memory allocation.
Expand All @@ -16,13 +16,13 @@ EngineEffectsManager::EngineEffectsManager(std::unique_ptr<EffectsResponsePipe>

void EngineEffectsManager::onCallbackStart() {
EffectsRequest* request = nullptr;
while (m_pResponsePipe->readMessage(&request)) {
while (m_responsePipe.readMessage(&request)) {
EffectsResponse response(*request);
bool processed = false;
switch (request->type) {
case EffectsRequest::ADD_EFFECT_CHAIN:
case EffectsRequest::REMOVE_EFFECT_CHAIN:
if (processEffectsRequest(*request, m_pResponsePipe.get())) {
if (processEffectsRequest(*request, &m_responsePipe)) {
processed = true;
}
break;
Expand All @@ -44,7 +44,7 @@ void EngineEffectsManager::onCallbackStart() {
break;
}
processed = request->pTargetChain->processEffectsRequest(
*request, m_pResponsePipe.get());
*request, &m_responsePipe);
if (processed) {
// When an effect becomes active (part of a chain), keep
// it in our main list so that we can respond to
Expand All @@ -71,7 +71,7 @@ void EngineEffectsManager::onCallbackStart() {
}

processed = request->pTargetEffect
->processEffectsRequest(*request, m_pResponsePipe.get());
->processEffectsRequest(*request, &m_responsePipe);

if (!processed) {
// If we got here, the message was not handled for an
Expand All @@ -87,7 +87,7 @@ void EngineEffectsManager::onCallbackStart() {
}

if (!processed) {
m_pResponsePipe->writeMessage(response);
m_responsePipe.writeMessage(response);
}
}
}
Expand Down
5 changes: 3 additions & 2 deletions src/engine/effects/engineeffectsmanager.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ struct GroupFeatureState;
/// PFL switch --> QuickEffectChains & StandardEffectChains --> mix channels into headphone mix --> headphone effect processing
class EngineEffectsManager final : public EffectsRequestHandler {
public:
EngineEffectsManager(std::unique_ptr<EffectsResponsePipe> pResponsePipe);
// passing by rvalue-ref because we want to ensure we're the only on with access to that pipe
EngineEffectsManager(EffectsResponsePipe&& responsePipe);
~EngineEffectsManager() override = default;

void onCallbackStart();
Expand Down Expand Up @@ -95,7 +96,7 @@ class EngineEffectsManager final : public EffectsRequestHandler {
CSAMPLE_GAIN newGain = CSAMPLE_GAIN_ONE,
bool fadeout = false);

std::unique_ptr<EffectsResponsePipe> m_pResponsePipe;
EffectsResponsePipe m_responsePipe;
QHash<SignalProcessingStage, QList<EngineEffectChain*>> m_chainsByStage;
QList<EngineEffect*> m_effects;

Expand Down
124 changes: 51 additions & 73 deletions src/util/messagepipe.h
Original file line number Diff line number Diff line change
@@ -1,109 +1,87 @@
#pragma once

#include <QScopedPointer>
#include <QSharedPointer>
#include <gsl/pointers>
#include <memory>

#include "rigtorp/SPSCQueue.h"

#include "util/class.h"
#include "util/reference.h"

// MessagePipe represents one side of a TwoWayMessagePipe. The direction of the
// pipe is with respect to the owner so sender and receiver are
// perspective-dependent.
template <class SenderMessageType, class ReceiverMessageType>
class MessagePipe {
template<typename T>
using not_null_shared_queue = gsl::not_null<std::shared_ptr<rigtorp::SPSCQueue<T>>>;

public:
MessagePipe(rigtorp::SPSCQueue<SenderMessageType>& receiver_messages,
rigtorp::SPSCQueue<ReceiverMessageType>& sender_messages,
BaseReferenceHolder* pTwoWayMessagePipeReference)
: m_receiver_messages(receiver_messages),
m_sender_messages(sender_messages),
m_pTwoWayMessagePipeReference(pTwoWayMessagePipeReference) {
MessagePipe(not_null_shared_queue<SenderMessageType> receiver_messages,
not_null_shared_queue<ReceiverMessageType> sender_messages)
: m_receiver_messages(std::move(receiver_messages)),
m_sender_messages(std::move(sender_messages)) {
}

// Disallow copying (could violate the Single-consumer/Single-producer constraint)
// allow moving since that does not violate the constraint
MessagePipe(const MessagePipe&) = delete;
MessagePipe& operator=(const MessagePipe&) = delete;
MessagePipe(MessagePipe&&) noexcept = default;
MessagePipe& operator=(MessagePipe&&) noexcept = default;
~MessagePipe() = default;

// Returns the number of ReceiverMessageType messages waiting to be read by
// the receiver. Non-blocking.
int messageCount() const {
return m_sender_messages.size();
return m_sender_messages->size();
}

// Try to read read a ReceiverMessageType written by the receiver
// addressed to the sender. Non-blocking.
bool readMessage(ReceiverMessageType* message) {
auto front = m_sender_messages.front();
auto front = m_sender_messages->front();
if (!front) {
return false;
}
*message = std::move(*front);
m_sender_messages.pop();
m_sender_messages->pop();
return true;
}

// Writes a message to the receiver and returns true on success.
bool writeMessage(const SenderMessageType& message) {
return m_receiver_messages.try_push(message);
return m_receiver_messages->try_push(message);
}

private:
rigtorp::SPSCQueue<SenderMessageType>& m_receiver_messages;
rigtorp::SPSCQueue<ReceiverMessageType>& m_sender_messages;
QScopedPointer<BaseReferenceHolder> m_pTwoWayMessagePipeReference;

DISALLOW_COPY_AND_ASSIGN(MessagePipe);
not_null_shared_queue<SenderMessageType> m_receiver_messages;
not_null_shared_queue<ReceiverMessageType> m_sender_messages;
};

// TwoWayMessagePipe is a bare-bones wrapper around the above rigtorp::SPSCQueue class that
// facilitates non-blocking two-way communication. To keep terminology clear,
// there are two sides to the message pipe, the sender side and the receiver
// side. The non-blocking aspect of the underlying rigtorp::SPSCQueue class requires that the
// sender methods and target methods each only be called from a single thread
// The most common use-case of this class is sending and receiving messages
// with the callback thread without the callback thread blocking.
//
// This class is an implementation detail and cannot be instantiated
// directly. Use makeTwoWayMessagePipe(...) to create a two-way pipe.
template <class SenderMessageType, class ReceiverMessageType>
class TwoWayMessagePipe {
public:
// Creates a TwoWayMessagePipe with SenderMessageType and
// ReceiverMessageType as the message types. Returns a pair of MessagePipes,
// the first is the sender's pipe (sends SenderMessageType and receives
// ReceiverMessageType messages) and the second is the receiver's pipe
// (sends ReceiverMessageType and receives SenderMessageType messages).
static std::pair<
std::unique_ptr<MessagePipe<SenderMessageType, ReceiverMessageType>>,
std::unique_ptr<MessagePipe<ReceiverMessageType, SenderMessageType>>>
makeTwoWayMessagePipe(
int sender_fifo_size,
int receiver_fifo_size) {
QSharedPointer<TwoWayMessagePipe<SenderMessageType, ReceiverMessageType>> pipe(
new TwoWayMessagePipe<SenderMessageType, ReceiverMessageType>(
sender_fifo_size, receiver_fifo_size));

return {
std::make_unique<MessagePipe<SenderMessageType, ReceiverMessageType>>(
pipe->m_receiver_messages,
pipe->m_sender_messages,
new ReferenceHolder<TwoWayMessagePipe<SenderMessageType,
ReceiverMessageType>>(pipe)),
std::make_unique<MessagePipe<ReceiverMessageType, SenderMessageType>>(
pipe->m_sender_messages,
pipe->m_receiver_messages,
new ReferenceHolder<TwoWayMessagePipe<SenderMessageType,
ReceiverMessageType>>(pipe))};
}

private:
TwoWayMessagePipe(int sender_fifo_size, int receiver_fifo_size)
: m_receiver_messages(receiver_fifo_size),
m_sender_messages(sender_fifo_size) {
}

// Messages waiting to be delivered to the receiver.
rigtorp::SPSCQueue<SenderMessageType> m_receiver_messages;
// Messages waiting to be delivered to the sender.
rigtorp::SPSCQueue<ReceiverMessageType> m_sender_messages;

DISALLOW_COPY_AND_ASSIGN(TwoWayMessagePipe);
/// makeTwoWayMessagePipe is a bare-bones wrapper around the above rigtorp::SPSCQueue class that
/// facilitates non-blocking two-way communication. To keep terminology clear,
/// there are two sides to the message pipe, the sender side and the receiver
/// side. The non-blocking aspect of the underlying rigtorp::SPSCQueue class requires that the
/// sender methods and target methods each only be called from a single thread
/// The most common use-case of this class is sending and receiving messages
/// with the callback thread without the callback thread blocking.
/// Returns a pair of MessagePipes,
/// the first is the sender's pipe (sends SenderMessageType and receives
/// ReceiverMessageType messages) and the second is the receiver's pipe
/// (sends ReceiverMessageType and receives SenderMessageType messages).
template<class SenderMessageType, class ReceiverMessageType>
static std::pair<
MessagePipe<SenderMessageType, ReceiverMessageType>,
MessagePipe<ReceiverMessageType, SenderMessageType>>
makeTwoWayMessagePipe(
int sender_fifo_size,
int receiver_fifo_size) {
auto sender_pipe = gsl::make_not_null(
std::make_shared<rigtorp::SPSCQueue<ReceiverMessageType>>(
sender_fifo_size));
auto receiver_pipe =
gsl::make_not_null(std::make_shared<rigtorp::SPSCQueue<SenderMessageType>>(
receiver_fifo_size));
MessagePipe<SenderMessageType, ReceiverMessageType> requestPipe(receiver_pipe, sender_pipe);
MessagePipe<ReceiverMessageType, SenderMessageType> responsePipe(
std::move(sender_pipe), std::move(receiver_pipe));
return {std::move(requestPipe), std::move(responsePipe)};
};
Loading

0 comments on commit 400ae46

Please sign in to comment.