Skip to content

Copy channelhandle by value instead of pointers #4672

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daschuer merged 2 commits into from
Feb 14, 2022
Merged

Copy channelhandle by value instead of pointers #4672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3603b8
Store channelhandles in messages by value instead of pointers. This f…
ywwg Feb 11, 2022
8bad0e3
EffectsRequest: Remove union initialization.
ywwg Feb 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/effects/backends/effectprocessor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,10 @@ class EffectProcessor {
virtual void loadEngineEffectParameters(
const QMap<QString, EngineEffectParameterPointer>& parameters) = 0;
virtual EffectState* createState(const mixxx::EngineParameters& engineParameters) = 0;
virtual void deleteStatesForInputChannel(const ChannelHandle* inputChannel) = 0;
virtual void deleteStatesForInputChannel(ChannelHandle inputChannel) = 0;

// Called from the audio thread
virtual bool loadStatesForInputChannel(const ChannelHandle* inputChannel,
virtual bool loadStatesForInputChannel(ChannelHandle inputChannel,
const EffectStatesMap* pStatesMap) = 0;

/// Called from the audio thread
Expand Down Expand Up @@ -202,11 +202,11 @@ class EffectProcessorImpl : public EffectProcessor {
return createSpecificState(engineParameters);
};

bool loadStatesForInputChannel(const ChannelHandle* inputChannel,
bool loadStatesForInputChannel(ChannelHandle inputChannel,
const EffectStatesMap* pStatesMap) final {
if (kEffectDebugOutput) {
qDebug() << "EffectProcessorImpl::loadStatesForInputChannel" << this
<< "input" << *inputChannel;
<< "input" << inputChannel;
}

// NOTE: ChannelHandleMap is like a map in that it associates an
Expand All @@ -220,7 +220,7 @@ class EffectProcessorImpl : public EffectProcessor {
// pStatesMap to build a new ChannelHandleMap with
// dynamic_cast'ed states.
ChannelHandleMap<EffectSpecificState*>& effectSpecificStatesMap =
m_channelStateMatrix[*inputChannel];
m_channelStateMatrix[inputChannel];

// deleteStatesForInputChannel should have been called before a new
// map of EffectStates was sent to this function, or this is the first
Expand Down Expand Up @@ -256,10 +256,10 @@ class EffectProcessorImpl : public EffectProcessor {
};

/// Called from main thread for garbage collection after an input channel is disabled
void deleteStatesForInputChannel(const ChannelHandle* inputChannel) final {
void deleteStatesForInputChannel(ChannelHandle inputChannel) final {
if (kEffectDebugOutput) {
qDebug() << "EffectProcessorImpl::deleteStatesForInputChannel"
<< this << *inputChannel;
<< this << inputChannel;
}

// NOTE: ChannelHandleMap is like a map in that it associates an
Expand All @@ -269,7 +269,7 @@ class EffectProcessorImpl : public EffectProcessor {
// engine thread in loadStatesForInputChannel.

ChannelHandleMap<EffectSpecificState*>& stateMap =
m_channelStateMatrix[*inputChannel];
m_channelStateMatrix[inputChannel];
for (EffectSpecificState* pState : stateMap) {
VERIFY_OR_DEBUG_ASSERT(pState != nullptr) {
continue;
Expand Down
4 changes: 2 additions & 2 deletions src/effects/effectchain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ void EffectChain::enableForInputChannel(const ChannelHandleAndGroup& handleGroup
EffectsRequest* request = new EffectsRequest();
request->type = EffectsRequest::ENABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL;
request->pTargetChain = m_pEngineEffectChain;
request->EnableInputChannelForChain.pChannelHandle = &handleGroup.handle();
request->EnableInputChannelForChain.channelHandle = handleGroup.handle();

// Allocate EffectStates here in the main thread to avoid allocating
// memory in the realtime audio callback thread. Pointers to the
Expand Down Expand Up @@ -428,7 +428,7 @@ void EffectChain::disableForInputChannel(const ChannelHandleAndGroup& handleGrou
EffectsRequest* request = new EffectsRequest();
request->type = EffectsRequest::DISABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL;
request->pTargetChain = m_pEngineEffectChain;
request->DisableInputChannelForChain.pChannelHandle = &handleGroup.handle();
request->DisableInputChannelForChain.channelHandle = handleGroup.handle();
m_pMessenger->writeRequest(request);
}

Expand Down
4 changes: 2 additions & 2 deletions src/effects/effectsmessenger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,10 @@ void EffectsMessenger::collectGarbage(const EffectsRequest* pRequest) {
} else if (pRequest->type == EffectsRequest::DISABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL) {
if (kEffectDebugOutput) {
qDebug() << debugString() << "deleting states for input channel"
<< pRequest->DisableInputChannelForChain.pChannelHandle
<< pRequest->DisableInputChannelForChain.channelHandle
<< "for EngineEffectChain" << pRequest->pTargetChain;
}
pRequest->pTargetChain->deleteStatesForInputChannel(
pRequest->DisableInputChannelForChain.pChannelHandle);
pRequest->DisableInputChannelForChain.channelHandle);
}
}
2 changes: 1 addition & 1 deletion src/engine/channelhandle.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class ChannelHandleAndGroup {
return m_name;
}

inline const ChannelHandle& handle() const {
inline ChannelHandle handle() const {
return m_handle;
}

Expand Down
2 changes: 1 addition & 1 deletion src/engine/channels/enginechannel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class EngineChannel : public EngineObject {

virtual ChannelOrientation getOrientation() const;

inline const ChannelHandle& getHandle() const {
inline ChannelHandle getHandle() const {
return m_group.handle();
}

Expand Down
6 changes: 3 additions & 3 deletions src/engine/effects/engineeffect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,16 +53,16 @@ EffectState* EngineEffect::createState(const mixxx::EngineParameters& enginePara
return m_pProcessor->createState(engineParameters);
}

void EngineEffect::loadStatesForInputChannel(const ChannelHandle* inputChannel,
void EngineEffect::loadStatesForInputChannel(ChannelHandle inputChannel,
EffectStatesMap* pStatesMap) {
if (kEffectDebugOutput) {
qDebug() << "EngineEffect::loadStatesForInputChannel" << this
<< "loading states for input" << *inputChannel;
<< "loading states for input" << inputChannel;
}
m_pProcessor->loadStatesForInputChannel(inputChannel, pStatesMap);
}

void EngineEffect::deleteStatesForInputChannel(const ChannelHandle* inputChannel) {
void EngineEffect::deleteStatesForInputChannel(ChannelHandle inputChannel) {
m_pProcessor->deleteStatesForInputChannel(inputChannel);
}

Expand Down
4 changes: 2 additions & 2 deletions src/engine/effects/engineeffect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ class EngineEffect final : public EffectsRequestHandler {
EffectState* createState(const mixxx::EngineParameters& engineParameters);

/// Called in audio thread to load EffectStates received from the main thread
void loadStatesForInputChannel(const ChannelHandle* inputChannel,
void loadStatesForInputChannel(ChannelHandle inputChannel,
EffectStatesMap* pStatesMap);
/// Called from the main thread for garbage collection after an input channel is disabled
void deleteStatesForInputChannel(const ChannelHandle* inputChannel);
void deleteStatesForInputChannel(ChannelHandle inputChannel);

/// Called in audio thread
bool processEffectsRequest(
Expand Down
20 changes: 10 additions & 10 deletions src/engine/effects/engineeffectchain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,21 +123,21 @@ bool EngineEffectChain::processEffectsRequest(EffectsRequest& message,
qDebug() << debugString() << this
<< "ENABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL"
<< message.pTargetChain
<< *message.EnableInputChannelForChain.pChannelHandle;
<< message.EnableInputChannelForChain.channelHandle;
}
response.success = enableForInputChannel(
message.EnableInputChannelForChain.pChannelHandle,
message.EnableInputChannelForChain.channelHandle,
message.EnableInputChannelForChain.pEffectStatesMapArray);
break;
case EffectsRequest::DISABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL:
if (kEffectDebugOutput) {
qDebug() << debugString() << this
<< "DISABLE_EFFECT_CHAIN_FOR_INPUT_CHANNEL"
<< message.pTargetChain
<< *message.DisableInputChannelForChain.pChannelHandle;
<< message.DisableInputChannelForChain.channelHandle;
}
response.success = disableForInputChannel(
message.DisableInputChannelForChain.pChannelHandle);
message.DisableInputChannelForChain.channelHandle);
break;
default:
return false;
Expand All @@ -146,12 +146,12 @@ bool EngineEffectChain::processEffectsRequest(EffectsRequest& message,
return true;
}

bool EngineEffectChain::enableForInputChannel(const ChannelHandle* inputHandle,
bool EngineEffectChain::enableForInputChannel(ChannelHandle inputHandle,
EffectStatesMapArray* statesForEffectsInChain) {
if (kEffectDebugOutput) {
qDebug() << "EngineEffectChain::enableForInputChannel" << this << inputHandle;
}
auto& outputMap = m_chainStatusForChannelMatrix[*inputHandle];
auto& outputMap = m_chainStatusForChannelMatrix[inputHandle];
for (auto&& outputChannelStatus : outputMap) {
VERIFY_OR_DEBUG_ASSERT(outputChannelStatus.enableState !=
EffectEnableState::Enabled) {
Expand Down Expand Up @@ -180,8 +180,8 @@ bool EngineEffectChain::enableForInputChannel(const ChannelHandle* inputHandle,
return true;
}

bool EngineEffectChain::disableForInputChannel(const ChannelHandle* inputHandle) {
auto& outputMap = m_chainStatusForChannelMatrix[*inputHandle];
bool EngineEffectChain::disableForInputChannel(ChannelHandle inputHandle) {
auto& outputMap = m_chainStatusForChannelMatrix[inputHandle];
for (auto&& outputChannelStatus : outputMap) {
if (outputChannelStatus.enableState != EffectEnableState::Disabled) {
outputChannelStatus.enableState = EffectEnableState::Disabling;
Expand All @@ -195,7 +195,7 @@ bool EngineEffectChain::disableForInputChannel(const ChannelHandle* inputHandle)
}

// Called from the main thread for garbage collection after an input channel is disabled
void EngineEffectChain::deleteStatesForInputChannel(const ChannelHandle* inputChannel) {
void EngineEffectChain::deleteStatesForInputChannel(const ChannelHandle inputChannel) {
// If an output channel is not presently being processed, for example when
// PFL is not active, then process() cannot be relied upon to set this
// chain's EffectEnableState from Disabling to Disabled. This must be done
Expand All @@ -209,7 +209,7 @@ void EngineEffectChain::deleteStatesForInputChannel(const ChannelHandle* inputCh
// QMap. So it is okay that m_chainStatusForChannelMatrix may be
// accessed concurrently in the audio engine thread in process(),
// enableForInputChannel(), or disableForInputChannel().
auto& outputMap = m_chainStatusForChannelMatrix[*inputChannel];
auto& outputMap = m_chainStatusForChannelMatrix[inputChannel];
for (auto&& outputChannelStatus : outputMap) {
outputChannelStatus.enableState = EffectEnableState::Disabled;
}
Expand Down
6 changes: 3 additions & 3 deletions src/engine/effects/engineeffectchain.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class EngineEffectChain final : public EffectsRequestHandler {
const GroupFeatureState& groupFeatures);

/// called from main thread
void deleteStatesForInputChannel(const ChannelHandle* channel);
void deleteStatesForInputChannel(const ChannelHandle channel);

private:
struct ChannelStatus {
Expand All @@ -64,9 +64,9 @@ class EngineEffectChain final : public EffectsRequestHandler {
bool updateParameters(const EffectsRequest& message);
bool addEffect(EngineEffect* pEffect, int iIndex);
bool removeEffect(EngineEffect* pEffect, int iIndex);
bool enableForInputChannel(const ChannelHandle* inputHandle,
bool enableForInputChannel(ChannelHandle inputHandle,
EffectStatesMapArray* statesForEffectsInChain);
bool disableForInputChannel(const ChannelHandle* inputHandle);
bool disableForInputChannel(ChannelHandle inputHandle);

// Gets or creates a ChannelStatus entry in m_channelStatus for the provided
// handle.
Expand Down
18 changes: 4 additions & 14 deletions src/engine/effects/message.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,23 +34,14 @@ struct EffectsRequest {
NUM_REQUEST_TYPES
};

// Creates a new uninitialized EffectsRequest. Callers are responsible for making sure that
// they initialize all the values of the struct corresponding to the type they select.
EffectsRequest()
: type(NUM_REQUEST_TYPES),
request_id(-1),
value(0.0) {
pTargetChain = nullptr;
pTargetEffect = nullptr;
#define CLEAR_STRUCT(x) memset(&x, 0, sizeof(x));
CLEAR_STRUCT(AddEffectChain);
CLEAR_STRUCT(RemoveEffectChain);
CLEAR_STRUCT(EnableInputChannelForChain);
CLEAR_STRUCT(DisableInputChannelForChain);
CLEAR_STRUCT(AddEffectToChain);
CLEAR_STRUCT(RemoveEffectFromChain);
CLEAR_STRUCT(SetEffectChainParameters);
CLEAR_STRUCT(SetEffectParameters);
CLEAR_STRUCT(SetParameterParameters);
#undef CLEAR_STRUCT
}

// This is called from the main thread by EffectsManager after receiving a
Expand Down Expand Up @@ -92,15 +83,14 @@ struct EffectsRequest {
} AddEffectChain;
struct {
EngineEffectChain* pChain;
int iIndex;
SignalProcessingStage signalProcessingStage;
} RemoveEffectChain;
struct {
EffectStatesMapArray* pEffectStatesMapArray;
const ChannelHandle* pChannelHandle;
ChannelHandle channelHandle;
Copy link
Copy Markdown
Contributor

@uklotzde uklotzde Feb 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was expecting that ChannelHandle with its custom constructor is not usable in this union. Removing the memset seems to work, though.

Copy link
Copy Markdown
Member Author

@ywwg ywwg Feb 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. Unfortunately that means the data isn't guaranteed to be zeroed out. According to what I've read online, as long as we memset that largest possible struct in the union, that will zero out all the data. I believe that SetEffectChainParameters is the largest one since it has three items, one of which is a double.

Another option would be to remove the default constructor and force callers to construct messages with specific types in mind -- then we'd know for sure nobody's accessing uninitialized data

Copy link
Copy Markdown
Contributor

@Be-ing Be-ing Feb 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems really brittle and easy to break when refactoring.

Copy link
Copy Markdown
Member Author

@ywwg ywwg Feb 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree -- for now we think it's just better to state that we don't guarantee initialization and callers are responsible for it. We can clean this up with the followup rewrite using variant or whatever

} EnableInputChannelForChain;
struct {
const ChannelHandle* pChannelHandle;
ChannelHandle channelHandle;
} DisableInputChannelForChain;
struct {
EngineEffect* pEffect;
Expand Down