Skip to content

Commit

Permalink
fix: avoid memory leak on C code
Browse files Browse the repository at this point in the history
  • Loading branch information
ABeltramo committed Sep 8, 2024
1 parent 829a38c commit 36f407b
Show file tree
Hide file tree
Showing 7 changed files with 56 additions and 49 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ GstBuffer *get_frame(WaylandState &w_state) {
static void destroy(WaylandState *w_state) {
logs::log(logs::trace, "~WaylandState");
display_finish(w_state->display);
delete (w_state);
}

bool add_input_device(WaylandState &w_state, const std::string &device_path) {
Expand Down
6 changes: 6 additions & 0 deletions src/moonlight-server/gst-plugin/gstrtpmoonlightpay_audio.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,12 @@ void gst_rtp_moonlight_pay_audio_dispose(GObject *object) {

GST_DEBUG_OBJECT(rtpmoonlightpay_audio, "dispose");

// free rtpmoonlightpay_audio->packets_buffer matrix
for (int i = 0; i < AUDIO_TOTAL_SHARDS; i++) {
delete[] rtpmoonlightpay_audio->packets_buffer[i];
}
delete[] rtpmoonlightpay_audio->packets_buffer;

G_OBJECT_CLASS(gst_rtp_moonlight_pay_audio_parent_class)->dispose(object);
}

Expand Down
2 changes: 1 addition & 1 deletion src/moonlight-server/state/config.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ void unpair(const Config &cfg, const PairedClient &client);
* Returns the first PairedClient with the given client_cert
*/
inline std::optional<PairedClient> get_client_via_ssl(const Config &cfg, x509::x509_ptr client_cert) {
auto paired_clients = cfg.paired_clients.load();
auto paired_clients = cfg.paired_clients->load();
auto search_result =
std::find_if(paired_clients->begin(), paired_clients->end(), [&client_cert](const PairedClient &pair_client) {
auto paired_cert = x509::cert_from_string(pair_client.client_cert);
Expand Down
41 changes: 19 additions & 22 deletions src/moonlight-server/state/configTOML.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -148,27 +148,24 @@ static bool is_available(const GPU_VENDOR &gpu_vendor, const GstEncoder &setting
settings.check_elements.begin(),
settings.check_elements.end(),
[settings, gpu_vendor](const auto &el_name) {
// Can we instantiate the plugin?
if (auto el = gst_element_factory_make(el_name.c_str(), nullptr)) {
// Is the selected GPU vendor compatible with the encoder?
// (Particularly useful when using multiple GPUs, e.g. nvcodec might be available but user
// wants to encode using the Intel GPU)
auto encoder_vendor = encoder_type(settings);
if (encoder_vendor == NVIDIA && gpu_vendor != GPU_VENDOR::NVIDIA) {
logs::log(logs::debug, "Skipping NVIDIA encoder, not a NVIDIA GPU ({})", (int)gpu_vendor);
} else if (encoder_vendor == VAAPI && (gpu_vendor != GPU_VENDOR::INTEL && gpu_vendor != GPU_VENDOR::AMD)) {
logs::log(logs::debug, "Skipping VAAPI encoder, not an Intel or AMD GPU ({})", (int)gpu_vendor);
} else if (encoder_vendor == QUICKSYNC && gpu_vendor != GPU_VENDOR::INTEL) {
logs::log(logs::debug, "Skipping QUICKSYNC encoder, not an Intel GPU ({})", (int)gpu_vendor);
}
// Can Gstreamer instantiate the element? This will only work if all the drivers are in place
else if (auto el = gst_element_factory_make(el_name.c_str(), nullptr)) {
gst_object_unref(el);
// Is the selected GPU vendor compatible with the encoder?
// (Particularly useful when using multiple GPUs, e.g. nvcodec might be available but user
// wants to encode using the Intel GPU)
auto encoder_vendor = encoder_type(settings);
if (encoder_vendor == NVIDIA && gpu_vendor != GPU_VENDOR::NVIDIA) {
logs::log(logs::debug, "Skipping NVIDIA encoder, not a NVIDIA GPU ({})", (int)gpu_vendor);
return false;
} else if (encoder_vendor == VAAPI && (gpu_vendor != GPU_VENDOR::INTEL && gpu_vendor != GPU_VENDOR::AMD)) {
logs::log(logs::debug, "Skipping VAAPI encoder, not an Intel or AMD GPU ({})", (int)gpu_vendor);
return false;
} else if (encoder_vendor == QUICKSYNC && gpu_vendor != GPU_VENDOR::INTEL) {
logs::log(logs::debug, "Skipping QUICKSYNC encoder, not an Intel GPU ({})", (int)gpu_vendor);
return false;
}
return true;
} else {
return false;
}

return false;
});
}
return false;
Expand Down Expand Up @@ -340,19 +337,19 @@ Config load_or_default(const std::string &source, const std::shared_ptr<dp::even
}) | //
ranges::to<immer::vector<state::App>>(); //

auto clients_atom = new immer::atom<state::PairedClientList>(paired_clients);
auto clients_atom = std::make_shared<immer::atom<state::PairedClientList>>(paired_clients);
return Config{.uuid = uuid,
.hostname = hostname,
.config_source = source,
.support_hevc = hevc_encoder.has_value(),
.support_av1 = av1_encoder.has_value() && encoder_type(*av1_encoder) != SOFTWARE,
.paired_clients = *clients_atom,
.paired_clients = clients_atom,

This comment has been minimized.

Copy link
@IvanCharcos

IvanCharcos Sep 9, 2024

Hi @ABeltramo

If we already have a config.toml that is configured for our own local env, would changes like this still make it in that config?

Is there a way to load both what is generated based on new code like this and already-existing user-edits?

This comment has been minimized.

Copy link
@ABeltramo

ABeltramo Sep 9, 2024

Author Member

I'm not sure what you are referring to, this specific code here is just to replace new with smart pointers. We do load the config.toml on startup and we merge in some sensible defaults for missing values; this part here doesn't do any of the above though 😅

This comment has been minimized.

Copy link
@IvanCharcos

IvanCharcos Sep 9, 2024

Maybe not this specific line here but in general.

We do load the config.toml on startup and we merge in some sensible defaults for missing values

Thanks!

.apps = apps};
}

void pair(const Config &cfg, const PairedClient &client) {
// Update CFG
cfg.paired_clients.update(
cfg.paired_clients->update(
[&client](const state::PairedClientList &paired_clients) { return paired_clients.push_back(client); });

// Update TOML
Expand All @@ -364,7 +361,7 @@ void pair(const Config &cfg, const PairedClient &client) {

void unpair(const Config &cfg, const PairedClient &client) {
// Update CFG
cfg.paired_clients.update([&client](const state::PairedClientList &paired_clients) {
cfg.paired_clients->update([&client](const state::PairedClientList &paired_clients) {
return paired_clients //
| ranges::views::filter([&client](auto paired_client) { //
return paired_client->client_cert != client.client_cert; //
Expand Down
2 changes: 1 addition & 1 deletion src/moonlight-server/state/data-structures.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ struct Config {
* Mutable, paired_clients will be loaded up on startup
* but can be added at runtime
*/
immer::atom<PairedClientList> &paired_clients;
std::shared_ptr<immer::atom<PairedClientList>> paired_clients;

/**
* List of available Apps
Expand Down
31 changes: 17 additions & 14 deletions tests/testGSTPlugin.cpp
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
#include <catch2/catch_test_macros.hpp>
#include <catch2/matchers/catch_matchers_string.hpp>
#include <catch2/matchers/catch_matchers_vector.hpp>
#include <catch2/matchers/catch_matchers_container_properties.hpp>
#include <catch2/matchers/catch_matchers_contains.hpp>
#include <catch2/matchers/catch_matchers_string.hpp>
#include <catch2/matchers/catch_matchers_vector.hpp>

using Catch::Matchers::Equals;

Expand Down Expand Up @@ -291,7 +291,7 @@ TEST_CASE_METHOD(GStreamerTestsFixture, "Create RTP VIDEO packets", "[GSTPlugin]
auto flatten_packets = gst_buffer_list_unfold(rtp_packets);
auto packets_content = gst_buffer_copy_content(flatten_packets);

std::vector<unsigned char*> packets_ptr(total_shards);
std::vector<unsigned char *> packets_ptr(total_shards);
for (int shard_idx = 0; shard_idx < total_shards; shard_idx++) {
packets_ptr[shard_idx] = &packets_content.front() + (shard_idx * rtp_packet_size);
}
Expand All @@ -300,7 +300,8 @@ TEST_CASE_METHOD(GStreamerTestsFixture, "Create RTP VIDEO packets", "[GSTPlugin]
std::vector<unsigned char> marks = {0, 0, 0, 0};

auto rs = moonlight::fec::create(data_shards, parity_shards);
auto result = moonlight::fec::decode(rs.get(), &packets_ptr.front(), &marks.front(), total_shards, rtp_packet_size);
auto result =
moonlight::fec::decode(rs.get(), &packets_ptr.front(), &marks.front(), total_shards, rtp_packet_size);

REQUIRE(result == 0);
REQUIRE_THAT(packets_content, Equals(gst_buffer_copy_content(flatten_packets)));
Expand All @@ -312,7 +313,8 @@ TEST_CASE_METHOD(GStreamerTestsFixture, "Create RTP VIDEO packets", "[GSTPlugin]
std::vector<unsigned char> marks = {1, 0, 0, 0};

auto rs = moonlight::fec::create(data_shards, parity_shards);
auto result = moonlight::fec::decode(rs.get(), &packets_ptr.front(), &marks.front(), total_shards, rtp_packet_size);
auto result =
moonlight::fec::decode(rs.get(), &packets_ptr.front(), &marks.front(), total_shards, rtp_packet_size);

REQUIRE(result == 0);

Expand Down Expand Up @@ -345,15 +347,17 @@ TEST_CASE_METHOD(GStreamerTestsFixture, "Create RTP VIDEO packets", "[GSTPlugin]
* AUDIO
*/
TEST_CASE_METHOD(GStreamerTestsFixture, "Audio RTP packet creation", "[GSTPlugin]") {
auto rtpmoonlightpay = (gst_rtp_moonlight_pay_audio *)g_object_new(gst_TYPE_rtp_moonlight_pay_audio, nullptr);
auto rtpmoonlightpay = std::shared_ptr<gst_rtp_moonlight_pay_audio>(
(gst_rtp_moonlight_pay_audio *)g_object_new(gst_TYPE_rtp_moonlight_pay_audio, nullptr),
g_object_unref);

rtpmoonlightpay->encrypt = true;
rtpmoonlightpay->aes_key = "0123456789012345";
rtpmoonlightpay->aes_iv = "12345678";

auto payload_str = "TUNZ TUNZ TUMP TUMP!"s;
auto payload = gst_buffer_new_and_fill(payload_str.size(), payload_str.c_str());
auto rtp_packets = audio::split_into_rtp(rtpmoonlightpay, payload);
auto rtp_packets = audio::split_into_rtp(rtpmoonlightpay.get(), payload);

REQUIRE(gst_buffer_list_length(rtp_packets) == 1);
REQUIRE(rtpmoonlightpay->cur_seq_number == 1);
Expand All @@ -377,7 +381,7 @@ TEST_CASE_METHOD(GStreamerTestsFixture, "Audio RTP packet creation", "[GSTPlugin
REQUIRE_THAT(decrypted, Equals(payload_str));
}

rtp_packets = audio::split_into_rtp(rtpmoonlightpay, payload);
rtp_packets = audio::split_into_rtp(rtpmoonlightpay.get(), payload);
REQUIRE(gst_buffer_list_length(rtp_packets) == 1);
REQUIRE(rtpmoonlightpay->cur_seq_number == 2);
auto second_pkt = gst_buffer_list_get(rtp_packets, 0);
Expand All @@ -400,7 +404,7 @@ TEST_CASE_METHOD(GStreamerTestsFixture, "Audio RTP packet creation", "[GSTPlugin
REQUIRE_THAT(decrypted, Equals(payload_str));
}

rtp_packets = audio::split_into_rtp(rtpmoonlightpay, payload);
rtp_packets = audio::split_into_rtp(rtpmoonlightpay.get(), payload);
REQUIRE(gst_buffer_list_length(rtp_packets) == 1);
REQUIRE(rtpmoonlightpay->cur_seq_number == 3);
auto third_pkt = gst_buffer_list_get(rtp_packets, 0);
Expand All @@ -426,7 +430,7 @@ TEST_CASE_METHOD(GStreamerTestsFixture, "Audio RTP packet creation", "[GSTPlugin
/* When the 4th packet arrives, we'll also FEC encode all the previous and return
* the data packet + 2 more FEC packets
*/
rtp_packets = audio::split_into_rtp(rtpmoonlightpay, payload);
rtp_packets = audio::split_into_rtp(rtpmoonlightpay.get(), payload);
REQUIRE(gst_buffer_list_length(rtp_packets) == 3); // One data packet + 2 FEC packets
REQUIRE(rtpmoonlightpay->cur_seq_number == 4);

Expand Down Expand Up @@ -478,7 +482,7 @@ TEST_CASE_METHOD(GStreamerTestsFixture, "Audio RTP packet creation", "[GSTPlugin
SECTION("Missing one packet should still lead to successful reconstruct") {
auto original_pkt = gst_buffer_copy_content(first_pkt, sizeof(audio::AudioRTPHeaders));
auto missing_pkt = std::vector<unsigned char>(packet_size);
rtpmoonlightpay->packets_buffer[0] = &missing_pkt[0];
std::copy(missing_pkt.begin(), missing_pkt.end(), rtpmoonlightpay->packets_buffer[0]);
std::vector<unsigned char> marks = {1, 0, 0, 0, 0, 0};

auto result = moonlight::fec::decode(rtpmoonlightpay->rs.get(),
Expand All @@ -488,10 +492,9 @@ TEST_CASE_METHOD(GStreamerTestsFixture, "Audio RTP packet creation", "[GSTPlugin
packet_size);

REQUIRE(result == 0);
REQUIRE_THAT(std::string(missing_pkt.begin() + sizeof(audio::AudioRTPHeaders), missing_pkt.end()),
REQUIRE_THAT(std::string(rtpmoonlightpay->packets_buffer[0] + sizeof(audio::AudioRTPHeaders),
rtpmoonlightpay->packets_buffer[0] + packet_size),
Equals(std::string(original_pkt.begin(), original_pkt.end())));
}

g_object_unref(rtpmoonlightpay);
}
}
22 changes: 11 additions & 11 deletions tests/testMoonlight.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,18 +55,18 @@ TEST_CASE("LocalState load TOML", "[LocalState]") {
}

SECTION("Paired Clients") {
REQUIRE_THAT(state.paired_clients.load().get(), Catch::Matchers::SizeIs(1));
REQUIRE_THAT(state.paired_clients.load().get()[0]->client_cert, Equals("A VERY VALID CERTIFICATE"));
REQUIRE(state.paired_clients.load().get()[0]->run_uid == 1234);
REQUIRE(state.paired_clients.load().get()[0]->run_gid == 5678);
REQUIRE_THAT(state.paired_clients.load().get()[0]->app_state_folder, Equals("some/folder"));
REQUIRE_THAT(state.paired_clients->load().get(), Catch::Matchers::SizeIs(1));
REQUIRE_THAT(state.paired_clients->load().get()[0]->client_cert, Equals("A VERY VALID CERTIFICATE"));
REQUIRE(state.paired_clients->load().get()[0]->run_uid == 1234);
REQUIRE(state.paired_clients->load().get()[0]->run_gid == 5678);
REQUIRE_THAT(state.paired_clients->load().get()[0]->app_state_folder, Equals("some/folder"));
}
}

TEST_CASE("LocalState pairing information", "[LocalState]") {
auto event_bus = std::make_shared<dp::event_bus>();
auto clients_atom = new immer::atom<state::PairedClientList>();
auto cfg = state::Config{.config_source = "config.test.toml", .paired_clients = *clients_atom};
auto clients_atom = std::make_shared<immer::atom<state::PairedClientList>>();
auto cfg = state::Config{.config_source = "config.test.toml", .paired_clients = clients_atom};
auto a_client_cert = "-----BEGIN CERTIFICATE-----\n"
"MIICvzCCAaegAwIBAgIBADANBgkqhkiG9w0BAQsFADAjMSEwHwYDVQQDDBhOVklE\n"
"SUEgR2FtZVN0cmVhbSBDbGllbnQwHhcNMjEwNzEwMDgzNjE3WhcNNDEwNzA1MDgz\n"
Expand Down Expand Up @@ -112,15 +112,15 @@ TEST_CASE("LocalState pairing information", "[LocalState]") {
REQUIRE(state::get_client_via_ssl(cfg, another_cert).has_value() == false);
state::pair(cfg, {another_cert});
REQUIRE(state::get_client_via_ssl(cfg, another_cert).has_value() == true);
REQUIRE_THAT(cfg.paired_clients.load().get(), Catch::Matchers::SizeIs(2));
REQUIRE_THAT(cfg.paired_clients->load().get(), Catch::Matchers::SizeIs(2));

state::unpair(cfg, {a_client_cert});
REQUIRE(state::get_client_via_ssl(cfg, a_client_cert).has_value() == false);
REQUIRE(state::get_client_via_ssl(cfg, another_cert).has_value() == true);
REQUIRE_THAT(cfg.paired_clients.load().get(), Catch::Matchers::SizeIs(1));
REQUIRE_THAT(cfg.paired_clients->load().get(), Catch::Matchers::SizeIs(1));

state::unpair(cfg, {another_cert});
REQUIRE_THAT(cfg.paired_clients.load().get(), Catch::Matchers::SizeIs(0));
REQUIRE_THAT(cfg.paired_clients->load().get(), Catch::Matchers::SizeIs(0));
}
}

Expand Down Expand Up @@ -353,7 +353,7 @@ TEST_CASE("launch", "[MoonlightProtocol]") {

TEST_CASE("Multiple users", "[HTTP]") {
auto event_bus = std::make_shared<dp::event_bus>();
auto paired_clients = immer::atom<state::PairedClientList>();
auto paired_clients = std::shared_ptr<immer::atom<state::PairedClientList>>();
auto app_state = state::AppState{
.config = state::Config{.paired_clients = paired_clients},
.host = {},
Expand Down

0 comments on commit 36f407b

Please sign in to comment.