Skip to content
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.

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

Fix #277 - Stack overflow in Bin Mon message handler #278

Merged
merged 4 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class BinauralMonitoringBackend {
bool isExporting() { return isExporting_; }

private:
void onSceneReceived(proto::SceneStore store);
void onSceneReceived(const proto::SceneStore& store);
void onConnection(communication::ConnectionId connectionId,
const std::string& streamEndpoint);
void onConnectionLost();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class SceneStore;
namespace communication {
class MonitoringMetadataReceiver {
public:
using RequestHandler = std::function<void(proto::SceneStore)>;
using RequestHandler = std::function<void(const proto::SceneStore& store)>;
MonitoringMetadataReceiver(std::shared_ptr<spdlog::logger> logger = nullptr);
~MonitoringMetadataReceiver();
MonitoringMetadataReceiver(const MonitoringMetadataReceiver&) = delete;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ class MonitoringBackend {
bool isExporting() { return isExporting_; }

private:
void onSceneReceived(proto::SceneStore store);
void onSceneReceived(const proto::SceneStore& store);
void onConnection(communication::ConnectionId connectionId,
const std::string& streamEndpoint);
void onConnectionLost();
void updateActiveGains(proto::SceneStore store);
void updateActiveGains(const proto::SceneStore& store);

std::shared_ptr<spdlog::logger> logger_;
std::mutex gainsMutex_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ struct ItemGains {
class SceneGainsCalculator {
public:
SceneGainsCalculator(Layout outputLayout, int inputChannelCount);
bool update(proto::SceneStore store);
bool update(const proto::SceneStore &store);
Eigen::MatrixXf directGains();
Eigen::MatrixXf diffuseGains();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,8 @@ BinauralMonitoringBackend::getLatestObjectsTypeMetadata(ConnId id) {
return std::optional<ObjectsEarMetadataAndRouting>();
}

void BinauralMonitoringBackend::onSceneReceived(proto::SceneStore store) {
void BinauralMonitoringBackend::onSceneReceived(
const proto::SceneStore& store) {
isExporting_ = store.has_is_exporting() && store.is_exporting();

size_t totalDsChannels = 0;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "communication/monitoring_metadata_receiver.hpp"
#include "scene_store.pb.h"
#include <functional>
#include <future>

namespace ear {
namespace plugin {
Expand Down Expand Up @@ -62,7 +63,12 @@ void MonitoringMetadataReceiver::handleReceive(std::error_code ec,
if (!sceneStore.ParseFromArray(message.data(), message.size())) {
throw std::runtime_error("Failed to parse Scene Object");
}
handler_(std::move(sceneStore));
// Called by NNG callback on thread with small stack.
// Launch task in another thread to overcome stack limitation.
auto future = std::async(std::launch::async, [this, &sceneStore]() {
handler_(sceneStore);
Copy link
Contributor Author

@firthm01 firthm01 May 13, 2024

Choose a reason for hiding this comment

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

This bit isn't ideal. I think this causes a copy. Using std::move makes the compiler complain because sceneStore is apparently a const here... not sure if that happens as part of the capture list?
I think as future.get() is blocking, we are safe in terms of lifetime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is fixed by d216065

Copy link
Contributor

@rsjbailey rsjbailey May 14, 2024

Choose a reason for hiding this comment

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

Would marking the lambda as mutable fix the const warning? I can't get a small example that looks the same to emit that warning so I'm guessing a bit. I should probably check out the project and try it properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to clarify as the later commit makes my message confusing. This code causes a compiler warning;

auto future = std::async(std::launch::async, [this, sceneStore]() {
    handler_(std::move(sceneStore));
});

The current code (below) it's happy with, but I'm not sure if this still causes a copy from the capture list to passing in to handle_ ;

auto future = std::async(std::launch::async, [this, sceneStore = std::move(sceneStore)]() {
    handler_(sceneStore);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think the mutable thing fixes it, so

auto future = std::async(std::launch::async, [this, sceneStore = std::move(sceneStore)]() mutable {
  handler_(std::move(sceneStore));
}

I'm a bit rusty, but I think under the hood a lambda creates a function object with the captures as data members, and by default the call operator is const so that repeated calls to the same lambda with the same arguments produce the same results, so it's as if you did

struct MyLambda {
  void operator()() const {
    that->handler(std::move(sceneStore)); // won't compile as sceneStore is const on a const object
  }
  MonitoringMetadataReceiver* that;
  proto::SceneStore sceneStore;
};

MonitoringMetadataReceiver::handleReceive() {
// snip ...
  MyLambda const lambda{this, std::move(sceneStore)};
  auto future = std::async(std::launch::async, lambda);
}

The mutable keyword just removes the consts

Bit weird, but if anything, the rest of the language is wrong and lambdas have the correct defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh thanks for the explanation - that makes sense why it complains!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why the handler doesn't just take a const reference, maybe it should? (I've not checked to see what it's doing at the other end, if it needs to keep hold of a copy then passing by value makes sense)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to const refs in ab7adb9

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, could capture by reference in the async lambda too (as we're immediately blocking on it's completion)

auto future = std::async(std::launch::async, [this, &sceneStore]() {
    handler_(sceneStore);
});
future.get(); //blocking

});
future.get(); //blocking
} catch (const std::runtime_error& e) {
EAR_LOGGER_ERROR(
logger_, "Failed to parse and dispatch scene metadata: {}", e.what());
Expand Down
8 changes: 4 additions & 4 deletions ear-production-suite-plugins/lib/src/monitoring_backend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,20 +47,20 @@ MonitoringBackend::~MonitoringBackend() {
controlConnection_.onConnectionEstablished(nullptr);
}

void MonitoringBackend::onSceneReceived(proto::SceneStore store) {
void MonitoringBackend::onSceneReceived(const proto::SceneStore& store) {
isExporting_ = store.has_is_exporting() && store.is_exporting();
updateActiveGains(std::move(store));
updateActiveGains(store);
}

GainHolder MonitoringBackend::currentGains() {
std::lock_guard<std::mutex> lock(gainsMutex_);
return gains_;
}

void MonitoringBackend::updateActiveGains(proto::SceneStore store) {
void MonitoringBackend::updateActiveGains(const proto::SceneStore& store) {
{
std::lock_guard<std::mutex> lock(gainsCalculatorMutex_);
gainsCalculator_.update(std::move(store));
gainsCalculator_.update(store);
}
{
std::lock_guard<std::mutex> lock(gainsMutex_);
Expand Down
56 changes: 23 additions & 33 deletions ear-production-suite-plugins/lib/src/scene_gains_calculator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@
#include "ear/metadata.hpp"
#include "helper/eps_to_ear_metadata_converter.hpp"
#include "helper/container_helpers.hpp"
#include <future>
#include <algorithm>


namespace {

int inputCount(ear::plugin::ItemGains const& itemGains) {
Expand Down Expand Up @@ -53,41 +51,33 @@ SceneGainsCalculator::SceneGainsCalculator(ear::Layout outputLayout,
totalOutputChannels{static_cast<int>(outputLayout.channels().size())},
totalInputChannels{inputChannelCount} {}

bool SceneGainsCalculator::update(proto::SceneStore store) {
// Called by NNG callback on thread with small stack.
// Launch task in another thread to overcome stack limitation.
auto future = std::async(std::launch::async, [this, store]() {

// First figure out what we need to process updates for
std::vector<communication::ConnectionId> cachedIdsChecklist;
cachedIdsChecklist.reserve(routingCache_.size());
for(auto const&[key, val] : routingCache_) {
cachedIdsChecklist.push_back(key);
}
/// Check-off found items, and also delete changed items from routing cache to be re-evaluated
for(const auto& item : store.monitoring_items()) {
auto itemId = communication::ConnectionId{ item.connection_id() };
cachedIdsChecklist.erase(std::remove(cachedIdsChecklist.begin(), cachedIdsChecklist.end(), itemId), cachedIdsChecklist.end());
if(item.changed()) {
removeItem(itemId);
}
}
/// Delete removed items from routing cache (i.e, those that weren't checked-off and therefore remain in cachedIdsChecklist)
for(const auto& itemId : cachedIdsChecklist) {
bool SceneGainsCalculator::update(const proto::SceneStore& store) {
// First figure out what we need to process updates for
std::vector<communication::ConnectionId> cachedIdsChecklist;
cachedIdsChecklist.reserve(routingCache_.size());
for(auto const&[key, val] : routingCache_) {
cachedIdsChecklist.push_back(key);
}
/// Check-off found items, and also delete changed items from routing cache to be re-evaluated
for(const auto& item : store.monitoring_items()) {
auto itemId = communication::ConnectionId{ item.connection_id() };
cachedIdsChecklist.erase(std::remove(cachedIdsChecklist.begin(), cachedIdsChecklist.end(), itemId), cachedIdsChecklist.end());
if(item.changed()) {
removeItem(itemId);
}
}
/// Delete removed items from routing cache (i.e, those that weren't checked-off and therefore remain in cachedIdsChecklist)
for(const auto& itemId : cachedIdsChecklist) {
removeItem(itemId);
}

// Now get the gain updates we need
for(const auto& item : store.monitoring_items()) {
/// If it's not in routingCache_, it's new or changed, so needs re-evaluating
if(!mapHasKey(routingCache_, communication::ConnectionId{ item.connection_id() })) {
addOrUpdateItem(item);
}
// Now get the gain updates we need
for(const auto& item : store.monitoring_items()) {
/// If it's not in routingCache_, it's new or changed, so needs re-evaluating
if(!mapHasKey(routingCache_, communication::ConnectionId{ item.connection_id() })) {
addOrUpdateItem(item);
}

});

future.get();
}

return true;
}
Expand Down
Loading