Rialto Server profiling by smudri85 · Pull Request #441 · rdkcentral/rialto

smudri85 · 2026-02-06T14:37:24Z

Summary: Add profiling to Rialto Server.
Type: Feature
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-11764

github-actions · 2026-02-06T14:37:36Z

Pull request must be merged with a description containing the required fields,

Summary:
Type: Feature/Fix/Cleanup
Test Plan:
Jira:

If there is no jira releated to this change, please put 'Jira: NO-JIRA'.

Description can be changed by editing the top comment on your pull request and making a new commit.

github-actions · 2026-02-06T14:38:08Z

common/source/Profiler.cpp:98:0: style: Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm]
if (record.stage == stage)
^
common/source/Profiler.cpp:111:0: style: Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm]
if (record.stage == stage && record.info == info)
^
common/source/Profiler.cpp:184:0: style: Consider using std::find_if algorithm instead of a raw loop. [useStlAlgorithm]
if (record.id == id)
^
nofile:0:0: information: Active checkers: 161/592 (use --checkers-report= to see details) [checkersReport]

rdkcmf-jenkins · 2026-02-06T14:41:11Z

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

Protex Server Path: /home/blackduck/github/rialto/441/rdkcentral/rialto
Commit: 282cf54

Report detail: gist'

Copilot

Pull request overview

This pull request adds profiling functionality to the Rialto Server to track performance metrics and pipeline events. The implementation includes a generic profiling interface in the common module and a GStreamer-specific profiler that integrates with the media player pipeline. The profiler records timestamped events at various stages of media playback and can be enabled via environment variable.

Changes:

Added core profiling infrastructure with IProfiler interface and Profiler implementation
Integrated GstProfiler into the media player pipeline to track key events
Added unit tests and mocks for the profiling functionality

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 25 comments.

Show a summary per file

File	Description
common/interface/IProfiler.h	Defines the profiler interface with methods for recording, finding, logging, and dumping profiling records
common/include/Profiler.h	Declares the concrete Profiler class implementation with internal Record structure
common/source/Profiler.cpp	Implements the profiler with thread-safe record storage, environment-based enabling, and file dumping
common/CMakeLists.txt	Adds Profiler.cpp to the build configuration
media/server/gstplayer/include/GstProfiler.h	Defines GStreamer-specific profiler wrapper for pipeline profiling
media/server/gstplayer/include/GenericPlayerContext.h	Adds GstProfiler instance to player context
media/server/gstplayer/source/GstProfiler.cpp	Implements GStreamer profiler with pad probe callbacks and element filtering
media/server/gstplayer/source/GstGenericPlayer.cpp	Integrates profiler into pipeline lifecycle (creation, termination, segment handling)
media/server/gstplayer/source/tasks/generic/HandleBusMessage.cpp	Records pipeline state changes
media/server/gstplayer/source/tasks/generic/FinishSetupSource.cpp	Records when all sources are attached
media/server/gstplayer/source/tasks/generic/AttachSource.cpp	Records AppSrc element creation
media/server/gstplayer/source/tasks/generic/DeepElementAdded.cpp	Schedules profiling for dynamically added GStreamer elements
media/server/gstplayer/CMakeLists.txt	Adds GstProfiler.cpp to build and includes common headers
tests/unittests/common/unittests/ProfilerTests.cpp	Unit tests for core profiler functionality
tests/unittests/common/unittests/CMakeLists.txt	Adds ProfilerTests.cpp to test build
tests/unittests/common/mocks/ProfilerMock.h	Mock implementation of IProfiler for testing
tests/unittests/common/mocks/ProfilerFactoryMock.h	Mock implementation of IProfilerFactory for testing

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-06T14:46:17Z

media/server/gstplayer/source/tasks/generic/AttachSource.cpp

+    if (appSrc)
+    {
+        auto recordId = m_context.gstProfiler->createRecord("Created AppSrc Element", m_gstWrapper->gstElementGetName(appSrc));
+        if(recordId)


Inconsistent spacing with codebase conventions. The codebase predominantly uses 'if (' with a space after 'if'. This pattern appears throughout the codebase (e.g., media/server/gstplayer/source/GstGenericPlayer.cpp). The spacing should be consistent.

Suggested change

if(recordId)

if (recordId)

Copilot · 2026-02-06T14:46:17Z

common/interface/IProfiler.h

+     *
+     * @param[in] stage : Stage name of the record to be found
+     *
+     * @retval Record identificator for found record or std::nullopt.


Spelling error in documentation. 'identificator' should be 'identifier'. This typo appears in the documentation for the return value.

Copilot · 2026-02-06T14:46:18Z

common/interface/IProfiler.h

+    /**
+     * @brief Logs a record for given identificator.
+     *
+     * @param[in] id : Record identificator


Spelling error in documentation. 'identificator' should be 'identifier'. This typo appears in the documentation for the parameter.

Copilot · 2026-02-06T14:46:18Z

media/server/gstplayer/source/GstProfiler.cpp

+    if (!pad)
+        return;
+
+    auto* probeCtx = new ProbeCtx{this, stage.value_or(""), std::string(m_gstWrapper->gstElementGetName(element))};


Redundant use of value_or(""). Line 74 uses stage.value_or("") but this is unnecessary because stage has already been checked for has_value() on line 67, and the function returns early if stage is nullopt. At this point, stage is guaranteed to have a value, so stage.value() should be used instead.

Suggested change

auto* probeCtx = new ProbeCtx{this, stage.value_or(""), std::string(m_gstWrapper->gstElementGetName(element))};

auto* probeCtx = new ProbeCtx{this, stage.value(), std::string(m_gstWrapper->gstElementGetName(element))};

Copilot · 2026-02-06T14:46:18Z

tests/unittests/common/unittests/ProfilerTests.cpp

@@ -0,0 +1,67 @@
+#include "IProfiler.h"


Missing copyright header. Other test files in the codebase consistently include a copyright and license header at the top of the file. This header should be added to maintain consistency with the rest of the codebase.

Copilot · 2026-02-06T14:46:23Z

common/source/Profiler.cpp

+    std::lock_guard<std::mutex> lock(m_mutex);
+
+    const auto* record = findById(id);
+    if(record)


Inconsistent spacing with codebase conventions. The codebase predominantly uses 'if (' with a space after 'if'. This pattern appears throughout the codebase (e.g., media/server/gstplayer/source/GstGenericPlayer.cpp). The spacing should be consistent.

Suggested change

if(record)

if (record)

Copilot · 2026-02-06T14:46:23Z

media/server/gstplayer/source/GstGenericPlayer.cpp


    RIALTO_SERVER_LOG_MIL("RialtoServer's pipeline terminated");
+    auto recordId = m_context.gstProfiler->createRecord("Pipeline Terminated");
+    if(recordId)


Inconsistent spacing with codebase conventions. The codebase predominantly uses 'if (' with a space after 'if'. This pattern appears throughout the codebase (e.g., media/server/gstplayer/source/GstGenericPlayer.cpp). The spacing should be consistent.

Suggested change

if(recordId)

if (recordId)

Copilot · 2026-02-06T14:46:24Z

media/server/gstplayer/source/GstProfiler.cpp

+std::optional<GstProfiler::RecordId> GstProfiler::createRecord(std::string stage)
+{
+    if (!m_enabled) return std::nullopt;
+
+    return m_profiler->record(stage);


Potential null pointer dereference. If m_profiler is null (which can happen if profilerFactory is null or createProfiler returns null), calling m_profiler->record() will cause a crash. The methods createRecord and logRecord check m_enabled but not m_profiler itself. Since m_enabled is only set to true if m_profiler is successfully created, the current implementation appears safe, but this creates a tight coupling that is not immediately obvious. Consider adding an explicit null check on m_profiler or documenting this invariant.

Copilot · 2026-02-06T14:46:24Z

tests/unittests/common/mocks/ProfilerFactoryMock.h

@@ -0,0 +1,16 @@
+#include "IProfiler.h"


Missing copyright header. Other header files in the codebase consistently include a copyright and license header at the top of the file. This header should be added to maintain consistency with the rest of the codebase.

Copilot · 2026-02-06T14:46:24Z

media/server/gstplayer/include/GstProfiler.h

@@ -0,0 +1,60 @@
+#ifndef FIREBOLT_RIALTO_SERVER_GST_PROFILER_H_


Missing copyright header. Other header files in the codebase consistently include a copyright and license header at the top of the file. This header should be added to maintain consistency with the rest of the codebase.

rdkcmf-jenkins · 2026-02-06T14:59:35Z

Coverity Issue - Uncaught exception

An exception of type "std::bad_optional_access" is thrown but the exception specification "/implicit/noexcept" doesn't allow it to be thrown. This will result in a call to terminate().

Medium Impact, CWE-248
UNCAUGHT_EXCEPT

Issue location

This issue was discovered outside the diff for this Pull Request. You can find it at:
media/server/gstplayer/source/GstGenericPlayer.cpp:210

rdkcmf-jenkins · 2026-02-06T14:59:36Z

media/server/gstplayer/source/GstProfiler.cpp

+    :   m_pipeline{pipeline},
+        m_gstWrapper{gstWrapper},
+        m_glibWrapper{glibWrapper},
+        m_profilerFactory{profilerFactory}


Coverity issue no longer present as of: undefined

Show issue

Coverity Issue - Variable copied when it could be moved

"profilerFactory" is passed-by-value as parameter to "std::shared_ptrfirebolt::rialto::common::IProfilerFactory::shared_ptr(std::shared_ptrfirebolt::rialto::common::IProfilerFactory const &) /explicit =default/", when it could be moved instead.

Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE

How to fix

Use "std::move(""profilerFactory"")" instead of "profilerFactory".

rdkcmf-jenkins · 2026-02-06T14:59:36Z

media/server/gstplayer/source/GstProfiler.cpp

+{
+    if (!m_enabled) return std::nullopt;
+
+    return m_profiler->record(stage);


Coverity issue no longer present as of: undefined

Show issue

Coverity Issue - Variable copied when it could be moved

"stage" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead.

Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE

How to fix

Use "std::move(""stage"")" instead of "stage".

rdkcmf-jenkins · 2026-02-06T14:59:36Z

media/server/gstplayer/source/GstProfiler.cpp

+{
+    if (!m_enabled) return std::nullopt;
+
+    return m_profiler->record(stage, info);


Coverity issue no longer present as of: undefined

Show issue

Coverity Issue - Variable copied when it could be moved

"stage" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead.

Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE

How to fix

Use "std::move(""stage"")" instead of "stage".

rdkcmf-jenkins · 2026-02-06T14:59:36Z

media/server/gstplayer/source/GstProfiler.cpp

+{
+    if (!m_enabled) return std::nullopt;
+
+    return m_profiler->record(stage, info);


Coverity issue no longer present as of: undefined

Show issue

Coverity Issue - Variable copied when it could be moved

"info" is passed-by-value as parameter to "std::__cxx11::basic_string<char, std::char_traits, std::allocator >::basic_string(std::__cxx11::basic_string<char, std::char_traits, std::allocator > const &)", when it could be moved instead.

Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE

How to fix

Use "std::move(""info"")" instead of "info".

mhughesacn · 2026-02-06T17:58:18Z

Hi @smudri85 : The blackduck issue is not a problem but all new code files should have proper headers please.

MartinH, RDK CMF Compliance Team

rdkcmf-jenkins · 2026-02-06T23:22:15Z

b'## WARNING: A Blackduck scan failure has been waived

A prior failure has been upvoted

Upvote reason: ok
Commit: 282cf54
'

Copilot

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 10 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-11T12:31:07Z

media/server/gstplayer/source/GstProfiler.cpp

+    if (!pad)
+        return;
+
+    auto *probeCtx = new ProbeCtx{this, stage.value(), std::string(m_gstWrapper->gstElementGetName(element))};


gstElementGetName() returns an allocated string that must be freed (and it can be NULL). Constructing std::string(...) directly from the return value can both leak and crash if NULL is returned. Store the returned gchar*, check for null, copy into std::string, then free it with m_glibWrapper->gFree(...).

Suggested change

auto *probeCtx = new ProbeCtx{this, stage.value(), std::string(m_gstWrapper->gstElementGetName(element))};

gchar *elementNameCStr = m_gstWrapper->gstElementGetName(element);

std::string elementName;

if (elementNameCStr)

{

elementName = elementNameCStr;

m_glibWrapper->gFree(elementNameCStr);

}

auto *probeCtx = new ProbeCtx{this, stage.value(), std::move(elementName)};

Copilot · 2026-02-11T12:31:08Z

media/server/gstplayer/source/GstProfiler.cpp

+    gst_pad_add_probe(pad, GST_PAD_PROBE_TYPE_BUFFER, &GstProfiler::probeCb, probeCtx, &GstProfiler::probeCtxDestroy);
+


gst_pad_add_probe returns a probe id, but it’s currently ignored. Without tracking/removing the probe in ~GstProfiler(), ProbeCtx can outlive GstProfiler and probeCb would then dereference a dangling self pointer (use-after-free risk). Store the probe id(s) and remove them during teardown (e.g., via gst_pad_remove_probe).

Copilot · 2026-02-11T12:31:08Z

media/server/gstplayer/source/GstProfiler.cpp

+    for (auto token : kKlassTokens)
+    {
+        if (g_strrstr(klass, token.data()) != nullptr)
+        {
+            return std::string(token.data()) + " FB Exit";


This code calls g_strrstr directly even though the codebase consistently goes through IGlibWrapper for GLib APIs (e.g. DeepElementAdded.cpp uses m_glibWrapper->gStrrstr(...)). Using the wrapper keeps the gstplayer code unit-testable and consistent; prefer m_glibWrapper->gStrrstr(klass, token.data()) here.

Copilot · 2026-02-11T12:31:08Z

common/source/Profiler.cpp

+#include "RialtoCommonLogging.h"
+
+#include <algorithm>
+#include <fstream>


Profiler.cpp uses std::getenv and std::tolower but doesn’t include the standard headers that declare them (<cstdlib> and <cctype>). This can fail to compile depending on toolchain/headers; add the missing includes explicitly.

Suggested change

#include <fstream>

#include <fstream>

#include <cstdlib>

#include <cctype>

Copilot · 2026-02-11T12:31:08Z

media/server/gstplayer/source/tasks/generic/AttachSource.cpp

+        const char *name = m_gstWrapper->gstElementGetName(appSrc);
+        std::string info = name ? name : "<null>";
+
+        auto recordId = m_context.gstProfiler->createRecord("Created AppSrc Element", std::move(info));
+        if (recordId)
+            m_context.gstProfiler->logRecord(recordId.value());


gstElementGetName() returns a newly allocated gchar* that must be freed with g_free (per IGstWrapper contract). Here it’s treated as const char* and never freed, which will leak; also the type should match the wrapper signature. Capture the returned pointer, copy into std::string, then free it via m_glibWrapper->gFree(...).

Suggested change

const char *name = m_gstWrapper->gstElementGetName(appSrc);

std::string info = name ? name : "<null>";

auto recordId = m_context.gstProfiler->createRecord("Created AppSrc Element", std::move(info));

if (recordId)

m_context.gstProfiler->logRecord(recordId.value());

gchar *name = m_gstWrapper->gstElementGetName(appSrc);

std::string info = name ? name : "<null>";

auto recordId = m_context.gstProfiler->createRecord("Created AppSrc Element", std::move(info));

if (recordId)

m_context.gstProfiler->logRecord(recordId.value());

if (name)

{

m_glibWrapper->gFree(name);

}

Copilot · 2026-02-11T12:31:09Z

media/server/gstplayer/source/GstProfiler.cpp

+#include "RialtoCommonLogging.h"
+
+#include <mutex>
+#include <string>


This file uses std::array and std::string_view (see kKlassTokens) but doesn’t include <array> / <string_view>. Relying on transitive includes is fragile and can break builds; add the missing standard headers (and consider dropping unused <mutex>).

Suggested change

#include <string>

#include <string>

#include <array>

#include <string_view>

Copilot · 2026-02-11T12:31:09Z

media/server/gstplayer/include/GenericPlayerContext.h

+    /**
+     * @brief Profiler for player pipeline
+     */
+    std::unique_ptr<GstProfiler> gstProfiler;


GenericPlayerContext stores a std::unique_ptr<GstProfiler> (concrete type) and includes GstProfiler.h, but this PR also introduces IGstProfiler + a GstProfilerMock. With the current field type, you can’t inject the mock in unit tests and you force all includers of GenericPlayerContext.h to pull in the full GstProfiler definition. Prefer storing std::unique_ptr<IGstProfiler> (forward-declare IGstProfiler and include IGstProfiler.h instead).

Copilot · 2026-02-11T12:31:09Z

media/server/gstplayer/CMakeLists.txt

        $<TARGET_PROPERTY:RialtoWrappers,INTERFACE_INCLUDE_DIRECTORIES>
        $<TARGET_PROPERTY:RialtoCommon,INTERFACE_INCLUDE_DIRECTORIES>
        ${GStreamerApp_INCLUDE_DIRS}
+        ${CMAKE_CURRENT_LIST_DIR}/../../../common/include


Adding ${CMAKE_CURRENT_LIST_DIR}/../../../common/include exposes RialtoCommon’s private headers to gstplayer and risks accidental coupling / ODR issues. GstProfiler includes IProfiler.h (from RialtoCommon’s public interface), and gstplayer already consumes RialtoCommon’s interface include dirs, so this extra include path looks unnecessary and should be removed unless there’s a concrete include that requires it.

Suggested change

${CMAKE_CURRENT_LIST_DIR}/../../../common/include

Copilot · 2026-02-11T12:31:09Z

tests/unittests/common/unittests/ProfilerTests.cpp

+    void SetUp() override
+    {
+        factory = IProfilerFactory::createFactory();
+        ASSERT_TRUE(factory);
+
+        profiler = factory->createProfiler("UnitTestModule");
+        ASSERT_TRUE(profiler);
+    }


These tests will typically be skipped because Profiler defaults to disabled unless PROFILER_ENABLED is set. To make the unit tests meaningful and deterministic, set PROFILER_ENABLED=1 in SetUp() (and restore/unset it in TearDown() to avoid leaking env changes across tests).

Copilot · 2026-02-11T12:31:10Z

tests/unittests/common/unittests/ProfilerTests.cpp

+    const std::string path = "/tmp/rialto_profiler_ut_dump.txt";
+    ASSERT_TRUE(profiler->dump(path));
+
+    std::ifstream in(path);
+    ASSERT_TRUE(in.good());


DumpCreatesFile writes a fixed path in /tmp but doesn’t clean it up afterward. Consider removing the file in the test (and/or using a unique temp filename) to avoid leaving artifacts and reduce the chance of collisions when tests run in parallel.

Summary: Add profiling to Rialto Server. Type: Feature Test Plan: UT/CT, Fullstack Jira: RDKEMW-11764

Summary: Add profiling to Rialto Server. Type: Feature Test Plan: UT/CT, Fullstack Jira: RDKEMW-117641

Summary: Add profiling to Rialto Server. Type: Feature Test Plan: UT/CT, Fullstack Jira: RDKEMW-11764

Copilot

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-11T14:18:04Z

tests/unittests/common/unittests/ProfilerTests.cpp

+    const auto suffix = std::to_string(std::random_device{}());
+    const std::string path = std::string{"/tmp/rialto_profiler_ut_dump_"} + suffix + ".txt";
+    ASSERT_TRUE(profiler->dump(path));
+
+    std::ifstream in(path);


DumpCreatesFile writes a dump file under /tmp but never removes it. Consider deleting the file at the end of the test (or using a temp-file helper) to avoid leaving artifacts behind across repeated test runs.

Copilot · 2026-02-11T14:18:04Z

media/server/gstplayer/source/tasks/generic/FinishSetupSource.cpp

+    auto recordId = m_context.gstProfiler->createRecord("All Sources Attached");
+    if (recordId)
+        m_context.gstProfiler->logRecord(recordId.value());


m_context.gstProfiler is dereferenced unconditionally. Several existing unit tests construct GenericPlayerContext without a profiler; this will cause a null-deref when FinishSetupSource runs. Add a null check (or ensure the context always has a valid no-op profiler).

Suggested change

auto recordId = m_context.gstProfiler->createRecord("All Sources Attached");

if (recordId)

m_context.gstProfiler->logRecord(recordId.value());

if (m_context.gstProfiler)

{

auto recordId = m_context.gstProfiler->createRecord("All Sources Attached");

if (recordId)

m_context.gstProfiler->logRecord(recordId.value());

}

Copilot · 2026-02-11T14:18:04Z

media/server/gstplayer/source/tasks/generic/DeepElementAdded.cpp

                m_callbackRegistered = true;
            }
+
+            m_context.gstProfiler->scheduleGstElementRecord(m_element);


m_context.gstProfiler is used without checking for null. Existing task unit tests build a GenericPlayerContext without initializing gstProfiler, so this new call can crash the tests. Add a null check around the profiler usage (or provide a default no-op profiler in the context).

Suggested change

m_context.gstProfiler->scheduleGstElementRecord(m_element);

if (m_context.gstProfiler)

{

m_context.gstProfiler->scheduleGstElementRecord(m_element);

}

Copilot · 2026-02-11T14:18:05Z

media/server/gstplayer/source/tasks/generic/HandleBusMessage.cpp

+            auto recordId = m_context.gstProfiler->createRecord("Pipeline State Changed",
+                                                                m_gstWrapper->gstElementStateGetName(newState));
+            if (recordId)
+                m_context.gstProfiler->logRecord(recordId.value());


m_context.gstProfiler is dereferenced unconditionally here. In the unit-test suite (e.g. HandleBusMessageTest), GenericPlayerContext is constructed without initializing gstProfiler, so this will crash. Guard these calls with if (m_context.gstProfiler) (or initialize gstProfiler to a no-op implementation in the context).

Suggested change

auto recordId = m_context.gstProfiler->createRecord("Pipeline State Changed",

m_gstWrapper->gstElementStateGetName(newState));

if (recordId)

m_context.gstProfiler->logRecord(recordId.value());

if (m_context.gstProfiler)

{

auto recordId = m_context.gstProfiler->createRecord(

"Pipeline State Changed", m_gstWrapper->gstElementStateGetName(newState));

if (recordId)

{

m_context.gstProfiler->logRecord(recordId.value());

}

}

Copilot · 2026-02-11T14:18:05Z

media/server/gstplayer/source/tasks/generic/AttachSource.cpp

+        const char *name = m_gstWrapper->gstElementGetName(appSrc);
+        std::string info = name ? name : "<null>";


gstElementGetName() (via IGstWrapper) is documented to return an allocated name that must be freed with g_free. Here it’s copied into a std::string but never freed, which will leak each time a source is attached. Free the returned name after copying (or wrap it in an RAII helper).

Suggested change

const char *name = m_gstWrapper->gstElementGetName(appSrc);

std::string info = name ? name : "<null>";

gchar *name = m_gstWrapper->gstElementGetName(appSrc);

std::string info = name ? name : "<null>";

m_glibWrapper->gFree(name);

Copilot · 2026-02-11T14:18:05Z

media/server/gstplayer/source/GstProfiler.cpp

+inline constexpr std::array kKlassTokens{
+    std::string_view{"Source"},
+    std::string_view{"Decryptor"},
+    std::string_view{"Decoder"},
+};


kKlassTokens uses std::array/std::string_view, but the file doesn’t include <array> / <string_view>. Depending on transitive includes can break builds on other toolchains; add the missing standard headers (and consider removing the unused <mutex> include).

Copilot · 2026-02-11T14:18:05Z

media/server/gstplayer/include/GenericPlayerContext.h

+    /**
+     * @brief Profiler for player pipeline
+     */
+    std::unique_ptr<GstProfiler> gstProfiler;
 };


GenericPlayerContext stores the profiler as std::unique_ptr<GstProfiler> (concrete type). Since an IGstProfiler interface and GstProfilerMock exist, consider storing std::unique_ptr<IGstProfiler> instead to reduce coupling and allow mocking in unit tests.

Copilot · 2026-02-11T14:18:06Z

tests/unittests/common/unittests/ProfilerTests.cpp

+    void SetUp() override
+    {
+        factory = IProfilerFactory::createFactory();
+        ASSERT_TRUE(factory);
+


These tests are skipped whenever PROFILER_ENABLED is disabled in the environment, which means CI may not execute any assertions. Consider setting PROFILER_ENABLED=1 in SetUp() (and restoring/unsetting it in TearDown()) so the tests deterministically validate record/find/dump behavior.

Copilot

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-12T09:29:42Z

media/server/gstplayer/source/GstProfiler.cpp

+    auto *probeCtx = new ProbeCtx{this, stage.value(), std::move(elementName)};
+    gst_pad_add_probe(pad, GST_PAD_PROBE_TYPE_BUFFER, &GstProfiler::probeCb, probeCtx, &GstProfiler::probeCtxDestroy);
+
+    m_gstWrapper->gstObjectUnref(pad);


ProbeCtx stores a raw this pointer and the pad-probe callback can run on a GStreamer streaming thread after GstProfiler is destroyed (e.g., during shutdown if a buffer arrives late), leading to a potential use-after-free. Consider making the callback independent of GstProfiler lifetime (e.g., store a shared_ptr<IProfiler> in ProbeCtx, or track probe IDs and remove probes/disable callbacks in ~GstProfiler() before releasing resources).

Copilot · 2026-02-12T09:29:42Z

media/server/gstplayer/include/GenericPlayerContext.h

 #include "FlushOnPrerollController.h"
+#include "GstProfiler.h"
 #include "IGstSrc.h"


GenericPlayerContext includes the concrete GstProfiler.h, which forces any user of the context to pull in the implementation header. Prefer depending on the interface (IGstProfiler) and forward-declaring it here to reduce coupling and build impact.

Copilot · 2026-02-12T09:29:43Z

media/server/gstplayer/source/tasks/generic/AttachSource.cpp

+    std::string profileriInfo;
    gchar *capsStr = m_gstWrapper->gstCapsToString(caps);
    GstElement *appSrc = nullptr;
    if (m_attachedSource->getType() == MediaSourceType::AUDIO)


Typo in variable name: profileriInfo looks unintended and makes the code harder to read/search. Rename it to profilerInfo (and update the use at record creation).

Copilot · 2026-02-12T09:29:43Z

common/source/Profiler.cpp

+Profiler::Profiler(std::string module)
+    : m_module(std::move(module)), m_enabled(parseEnv(std::getenv(kProfilerEnv), false))
+{


Profiler::Profiler calls std::getenv but <cstdlib> isn’t included in this translation unit. Add <cstdlib> explicitly to avoid build breaks from missing transitive includes.

Copilot · 2026-02-12T09:29:43Z

common/source/Profiler.cpp

+    std::string stringValue(value);
+    std::transform(stringValue.begin(), stringValue.end(), stringValue.begin(),
+                   [](unsigned char c) { return static_cast<char>(std::tolower(c)); });
+


parseEnv uses std::tolower but <cctype> isn’t included here. Please include <cctype> (or otherwise ensure std::tolower is declared) rather than relying on indirect includes.

Copilot · 2026-02-12T09:29:43Z

tests/unittests/common/unittests/ProfilerTests.cpp

+    const std::string path = std::string{"/tmp/rialto_profiler_ut_dump_"} + suffix + ".txt";
+    ASSERT_TRUE(profiler->dump(path));
+
+    std::ifstream in(path);
+    ASSERT_TRUE(in.good());


This test writes a dump file under /tmp but doesn’t clean it up, which can leave litter behind across repeated runs. Consider deleting the file at the end of the test (or using a scoped temp-file helper if available).

Copilot · 2026-02-12T09:29:44Z

media/server/gstplayer/include/GstProfiler.h

+    enum class Stage
+    {
+        AppSrc,
+        Decryptor,
+        Decoder
+    };


Stage enum is currently unused (all stages are passed as strings). If it’s not needed imminently, please remove it to avoid dead code; otherwise wire it into the API to keep the type-safety benefit.

Suggested change

enum class Stage

{

AppSrc,

Decryptor,

Decoder

};

Summary: Add profiling to Rialto Server. Type: Feature Test Plan: UT/CT, Fullstack Jira: RDKEMW-11764

github-actions · 2026-02-12T13:49:38Z

Coverage statistics of your commit:
WARNING: Lines coverage decreased from: 85.2% to 84.6%
WARNING: Functions coverage decreased from: 92.5% to 91.9%

Copilot AI review requested due to automatic review settings February 6, 2026 14:37

Copilot started reviewing on behalf of smudri85 February 6, 2026 14:37 View session

Copilot AI reviewed Feb 6, 2026

View reviewed changes

rdkcmf-jenkins reviewed Feb 6, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings February 11, 2026 12:19

smudri85 force-pushed the feature/RDKEMW-11764 branch from 95cc046 to 43eb771 Compare February 11, 2026 12:19

Copilot started reviewing on behalf of smudri85 February 11, 2026 12:20 View session

Copilot AI reviewed Feb 11, 2026

View reviewed changes

smudri85 force-pushed the feature/RDKEMW-11764 branch from 43eb771 to e476ba7 Compare February 11, 2026 13:45

Copilot AI review requested due to automatic review settings February 11, 2026 14:06

smudri85 force-pushed the feature/RDKEMW-11764 branch from e476ba7 to 5862d6c Compare February 11, 2026 14:06

Copilot started reviewing on behalf of smudri85 February 11, 2026 14:06 View session

Sasa Mudri added 7 commits February 11, 2026 15:11

Added Profiler and GstProfiler classes.

6b08b85

Summary: Add profiling to Rialto Server. Type: Feature Test Plan: UT/CT, Fullstack Jira: RDKEMW-11764

Updated Profiler and GstProfiler classes.

56bbeeb

Summary: Add profiling to Rialto Server. Type: Feature Test Plan: UT/CT, Fullstack Jira: RDKEMW-117641

Added IProfiler interface, dump() method and additional changes.

b3caee4

Summary: Add profiling to Rialto Server. Type: Feature Test Plan: UT/CT, Fullstack Jira: RDKEMW-11764

Added Profiler configuration (enable/disable) via environment variable.

34bc578

Summary: Add profiling to Rialto Server. Type: Feature Test Plan: UT/CT, Fullstack Jira: RDKEMW-11764

Added descriptions for IProfiler and some small fixes.

6daba54

Summary: Add profiling to Rialto Server. Type: Feature Test Plan: UT/CT, Fullstack Jira: RDKEMW-11764

Added mocks and unit tests for IProfiler.

e6eb8ab

Summary: Add profiling to Rialto Server. Type: Feature Test Plan: UT/CT, Fullstack Jira: RDKEMW-11764

Tiny change for deep element record stage name.

7c6c30c

Summary: Add profiling to Rialto Server. Type: Feature Test Plan: UT/CT, Fullstack Jira: RDKEMW-11764

Copilot AI reviewed Feb 11, 2026

View reviewed changes

smudri85 force-pushed the feature/RDKEMW-11764 branch 2 times, most recently from 3934f05 to 761b259 Compare February 12, 2026 09:18

Copilot AI review requested due to automatic review settings February 12, 2026 09:18

Copilot started reviewing on behalf of smudri85 February 12, 2026 09:18 View session

Copilot AI reviewed Feb 12, 2026

View reviewed changes

smudri85 force-pushed the feature/RDKEMW-11764 branch from 761b259 to 3d607f2 Compare February 12, 2026 13:28

Added 'github actions' compliance changes.

3d607f2

Summary: Add profiling to Rialto Server. Type: Feature Test Plan: UT/CT, Fullstack Jira: RDKEMW-11764

	auto* probeCtx = new ProbeCtx{this, stage.value_or(""), std::string(m_gstWrapper->gstElementGetName(element))};
	auto* probeCtx = new ProbeCtx{this, stage.value(), std::string(m_gstWrapper->gstElementGetName(element))};

		@@ -0,0 +1,60 @@
		#ifndef FIREBOLT_RIALTO_SERVER_GST_PROFILER_H_

-    auto *probeCtx = new ProbeCtx{this, stage.value(), std::string(m_gstWrapper->gstElementGetName(element))};
+    gchar *elementNameCStr = m_gstWrapper->gstElementGetName(element);
+    std::string elementName;
+    if (elementNameCStr)
+    {
+        elementName = elementNameCStr;
+        m_glibWrapper->gFree(elementNameCStr);
+    }
+    auto *probeCtx = new ProbeCtx{this, stage.value(), std::move(elementName)};

		gst_pad_add_probe(pad, GST_PAD_PROBE_TYPE_BUFFER, &GstProfiler::probeCb, probeCtx, &GstProfiler::probeCtxDestroy);

		const char *name = m_gstWrapper->gstElementGetName(appSrc);
		std::string info = name ? name : "<null>";

Comments

Conversation

smudri85 commented Feb 6, 2026

Uh oh!

github-actions bot commented Feb 6, 2026

Uh oh!

github-actions bot commented Feb 6, 2026

Uh oh!

rdkcmf-jenkins commented Feb 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

rdkcmf-jenkins commented Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverity Issue - Uncaught exception

Issue location

Uh oh!

rdkcmf-jenkins Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Coverity Issue - Variable copied when it could be moved

How to fix

Uh oh!

rdkcmf-jenkins Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Coverity Issue - Variable copied when it could be moved

How to fix

Uh oh!

rdkcmf-jenkins Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Coverity Issue - Variable copied when it could be moved

How to fix

Uh oh!

rdkcmf-jenkins Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Coverity Issue - Variable copied when it could be moved

How to fix

Uh oh!

mhughesacn commented Feb 6, 2026

Uh oh!

rdkcmf-jenkins commented Feb 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

rdkcmf-jenkins commented Feb 6, 2026 •

edited

Loading

rdkcmf-jenkins Feb 6, 2026 •

edited

Loading

rdkcmf-jenkins Feb 6, 2026 •

edited

Loading

rdkcmf-jenkins Feb 6, 2026 •

edited

Loading

rdkcmf-jenkins Feb 6, 2026 •

edited

Loading