RDKEMW-18181: CPU and Memory metrics - #577
Open
DouglasAdler wants to merge 11 commits into
Open
Conversation
Introduce a private metrics module that lets ready clients receive connected and periodic sample requests, report process CPU metrics back to the server, and log combined client/server CPU usage. Wire the module into client and server IPC setup and update client controller tests/mocks for the new dependency.
…luggable output - Add process memory (VmRSS) to client metrics reporting - Add cgroup memory (v2 with v1 fallback) to server metrics - Implement Welford's algorithm for online mean/variance (MetricsAccumulator) - Add StateMetricsAggregator: accumulates per-state min/max/mean/stddev - Track playback state transitions per session, emit aggregated report on state change (e.g. PLAYING->END_OF_STREAM reports CPU/memory stats) - Track application state transitions (RUNNING/INACTIVE), emit report - Add IMetricsReporter interface with LogMetricsReporter and CompositeMetricsReporter for pluggable output destinations - Add MetricsThresholdChecker with configurable warning/critical levels and 2-sample debounce to avoid alert storms - Only aggregate and check thresholds on PERIODIC samples (not STATE_TRANSITION boundary samples which have unreliable CPU data) - Minimum 100ms elapsed time for CPU percentage calculation to prevent division-by-tiny-delta artifacts - Wire playback state notifications through MediaPipelineClient - Wire application state notifications through SessionManagementServer - Add METRICS_SAMPLE_REASON_STATE_TRANSITION to proto enum - Update mocks for new interface methods
Signed-off-by: Douglas Adler <douglas.adler@yahoo.com>
On transition to INACTIVE, record a server-side memory snapshot after pipelines and shared memory have been freed. This fires unconditionally (before the manager ACK) so it is captured even if the IPC socket breaks. The snapshot reads /proc/self/smaps_rollup and logs: server_mem_kb - VmRSS total cgroup_mem_kb - cgroup memory usage anon_kb - anonymous (= private_dirty_kb) private_dirty_kb - truly committed RAM the OS cannot reclaim private_clean_kb - file-backed, OS-reclaimable shared_clean_kb - loaded .so libs, OS-reclaimable Also call malloc_trim(0) in PlaybackService::switchToInactive() to return heap fragmentation to the OS after pipelines are torn down. Add shm_mem_kb (Pss_Shmem from smaps_rollup) to each periodic sample so the memfd-backed shared transport buffer is visible during playback. Promote reportPeriodicSample log level from INFO to MIL so samples appear in production logs.
Signed-off-by: Douglas Adler <douglas.adler@yahoo.com>
Replace the copied private metrics proto files with symbolic links and remove the duplicated MetricsCollector header content. Move private metrics ownership into PlaybackService so MediaPipeline and WebAudio can share playback-state reporting without passing a raw service pointer through IPC. Keep API-provided video geometry authoritative and use environment geometry only as a fallback. Retain the 15-second sampling interval while limiting active routine logs to 10-minute intervals. Report significant changes early, suppress stable inactive samples, and handle nonresponsive clients. Add unit coverage for IPC, collectors, reporters, services, WebAudio, video geometry, and metrics ownership. Signed-off-by: Douglas Adler <douglas.adler@yahoo.com>
|
Pull request title must follow the pattern: Pull request description must follow the Commit message format for RDK-E:
< JIRA TICKET >: < one line summary of change less than 65 characters > |
|
media/server/main/source/MetricsCollector.cpp:124:9: style: Condition 'becameUnresponsive' is always false [knownConditionTrueFalse] |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new “private” metrics collection pipeline to Rialto so the server can automatically collect CPU/memory from both client and server processes, reporting on significant changes and on state transitions. It also extends state propagation to feed metrics boundaries and adds an environment-driven fallback video geometry path in the GStreamer player.
Changes:
- Introduces a new PrivateMetrics IPC module (proto + client IPC + server IPC/service/main) and integrates it into session lifecycle, media pipeline, and WebAudio paths.
- Adds server-side metrics framework components (collector, reporters, threshold checker, aggregators) plus comprehensive unit tests.
- Adds environment-based default video geometry fallback and improves sink rectangle handling (including
render-rectangle).
Reviewed changes
Copilot reviewed 96 out of 96 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unittests/media/server/service/webAudioPlayerService/WebAudioPlayerServiceTestsFixture.h | Adds PrivateMetricsService mock to WebAudio service tests fixture. |
| tests/unittests/media/server/service/webAudioPlayerService/WebAudioPlayerServiceTestsFixture.cpp | Passes metrics service into WebAudioPlayerService constructor in tests. |
| tests/unittests/media/server/service/sessionServerManager/SessionServerManagerTestsFixture.cpp | Updates expectations to include application state change notifications to session management. |
| tests/unittests/media/server/service/playbackService/PlaybackServiceTestsFixture.h | Adds fixture helper for PrivateMetricsService exposure test. |
| tests/unittests/media/server/service/playbackService/PlaybackServiceTestsFixture.cpp | Adds a new fixture assertion for metrics service exposure. |
| tests/unittests/media/server/service/playbackService/PlaybackServiceTests.cpp | Adds a new gtest validating PrivateMetricsService is exposed. |
| tests/unittests/media/server/service/metrics/PrivateMetricsServiceTests.cpp | New unit tests for service-layer routing and lifecycle handling. |
| tests/unittests/media/server/service/metrics/MetricsClientsTests.cpp | New tests for MediaPipeline/WebAudio metrics client wrappers forwarding + reporting. |
| tests/unittests/media/server/service/mediaPipelineService/MediaPipelineServiceTestsFixture.h | Adds PrivateMetricsService mock to MediaPipeline service tests fixture. |
| tests/unittests/media/server/service/mediaPipelineService/MediaPipelineServiceTestsFixture.cpp | Passes metrics service into MediaPipelineService constructor in tests. |
| tests/unittests/media/server/service/CMakeLists.txt | Registers new service-layer metrics unit tests. |
| tests/unittests/media/server/mocks/service/PrivateMetricsServiceMock.h | Adds mock for IPrivateMetricsService. |
| tests/unittests/media/server/mocks/service/PlaybackServiceMock.h | Extends playback service mock to expose PrivateMetricsService. |
| tests/unittests/media/server/mocks/main/MetricsReporterMock.h | Adds mock for IMetricsReporter. |
| tests/unittests/media/server/mocks/main/MetricsCollectorMock.h | Adds mocks for IMetricsCollector and IMetricsCollectorFactory. |
| tests/unittests/media/server/mocks/main/MetricsCollectorClientMock.h | Adds mock for IMetricsCollectorClient. |
| tests/unittests/media/server/mocks/ipc/SessionManagementServerMock.h | Adds notifyApplicationStateChanged to session management server mock. |
| tests/unittests/media/server/mocks/ipc/PrivateMetricsModuleServiceMock.h | Adds mocks for PrivateMetrics module IPC service + factory. |
| tests/unittests/media/server/main/metrics/MetricsHelpersTests.cpp | New unit tests for accumulator/aggregator/reporters/threshold checker behavior. |
| tests/unittests/media/server/main/metrics/MetricsCollectorTests.cpp | New unit tests for MetricsCollector sampling/state boundaries/timer behavior. |
| tests/unittests/media/server/main/metrics/LogMetricsReporterTests.cpp | New unit tests for log suppression/reporting logic. |
| tests/unittests/media/server/main/CMakeLists.txt | Registers new server-main metrics unit tests. |
| tests/unittests/media/server/ipc/sessionManagementServer/SessionManagementServerTestsFixture.h | Adds PrivateMetrics module/service mocks to session management IPC tests. |
| tests/unittests/media/server/ipc/sessionManagementServer/SessionManagementServerTestsFixture.cpp | Wires PrivateMetrics module factory and client connect/disconnect expectations. |
| tests/unittests/media/server/ipc/privateMetricsModuleService/PrivateMetricsModuleServiceTests.cpp | New unit tests for PrivateMetricsModuleService lifecycle, RPCs, and factory. |
| tests/unittests/media/server/ipc/CMakeLists.txt | Registers new IPC-layer private metrics tests. |
| tests/unittests/media/server/gstplayer/genericPlayer/tasksTests/SetupElementTest.cpp | Adds tests for environment fallback video geometry behavior. |
| tests/unittests/media/server/gstplayer/genericPlayer/GstGenericPlayerPrivateTest.cpp | Updates test to check render-rectangle property absence handling. |
| tests/unittests/media/server/gstplayer/genericPlayer/common/GenericTasksTestsBase.h | Adds helpers for fallback geometry setup and pending-geometry checks. |
| tests/unittests/media/server/gstplayer/genericPlayer/common/GenericTasksTestsBase.cpp | Implements new helpers for fallback geometry scenarios. |
| tests/unittests/media/client/mocks/ipc/PrivateMetricsIpcMock.h | Adds client-side mock for IPrivateMetricsIpc. |
| tests/unittests/media/client/mocks/ipc/PrivateMetricsIpcFactoryMock.h | Adds client-side mock for IPrivateMetricsIpcFactory. |
| tests/unittests/media/client/main/clientController/MemoryManagementTest.cpp | Updates client controller tests to include PrivateMetricsIpc construction. |
| tests/unittests/media/client/main/clientController/CreateTest.cpp | Adds PrivateMetricsIpc creation/failure tests and a basic reporting test. |
| tests/unittests/media/client/ipc/privateMetricsIpc/PrivateMetricsIpcTests.cpp | New unit tests for PrivateMetricsIpc RPC + event forwarding behavior. |
| tests/unittests/media/client/ipc/CMakeLists.txt | Registers new client IPC private metrics tests. |
| proto/privatemetricsmodule.proto | Defines new PrivateMetricsModule proto service, messages, and enums. |
| proto/CMakeLists.txt | Adds privatemetricsmodule.proto to protobuf generation list. |
| media/server/service/source/WebAudioPlayerService.h | Adds metrics service dependency to WebAudioPlayerService. |
| media/server/service/source/WebAudioPlayerService.cpp | Wraps WebAudio client with WebAudioPlayerMetricsClient for state reporting. |
| media/server/service/source/WebAudioPlayerMetricsClient.h | New wrapper client that reports WebAudio state transitions to metrics service. |
| media/server/service/source/WebAudioPlayerMetricsClient.cpp | Implements state reporting + forwarding for WebAudio client callbacks. |
| media/server/service/source/SessionServerManager.cpp | Notifies session management server of application state transitions for metrics. |
| media/server/service/source/PrivateMetricsService.h | New service-layer router managing collectors per connected client. |
| media/server/service/source/PrivateMetricsService.cpp | Implements routing, app-state updates, and INACTIVE server memory snapshot logging. |
| media/server/service/source/PlaybackService.h | Adds PrivateMetricsService ownership and getter to PlaybackService. |
| media/server/service/source/PlaybackService.cpp | Instantiates PrivateMetricsService, wires into services, trims heap on inactive. |
| media/server/service/source/MediaPipelineService.h | Adds metrics service dependency to MediaPipelineService. |
| media/server/service/source/MediaPipelineService.cpp | Wraps media pipeline client with MediaPipelineMetricsClient for playback-state reporting. |
| media/server/service/source/MediaPipelineMetricsClient.h | New wrapper client that reports playback state transitions to metrics service. |
| media/server/service/source/MediaPipelineMetricsClient.cpp | Implements playback state reporting + forwarding for media pipeline client callbacks. |
| media/server/service/include/IPrivateMetricsService.h | New service API for client lifecycle, metrics reports, and state notifications. |
| media/server/service/include/IPlaybackService.h | Extends playback service interface to expose PrivateMetricsService. |
| media/server/service/CMakeLists.txt | Adds new service sources for metrics wrappers and service implementation. |
| media/server/main/source/MetricsThresholdChecker.cpp | New threshold checker implementation for metrics alerting. |
| media/server/main/source/LogMetricsReporter.cpp | New log reporter with periodic suppression and change detection. |
| media/server/main/source/CompositeMetricsReporter.cpp | New fan-out reporter to multiple reporters. |
| media/server/main/interface/IMetricsCollectorClient.h | New callback interface for requesting metrics samples from the client via IPC. |
| media/server/main/interface/IMetricsCollector.h | New collector interfaces + factory contract and metrics data struct. |
| media/server/main/interface/IMainThread.h | Adds missing <cstdint> include. |
| media/server/main/include/StateMetricsAggregator.h | New per-state aggregator for metrics statistics. |
| media/server/main/include/MetricsThresholdChecker.h | New threshold config + checker API. |
| media/server/main/include/MetricsCollector.h | New collector class declaration and internal state tracking. |
| media/server/main/include/MetricsAccumulator.h | New running statistics accumulator (Welford). |
| media/server/main/include/LogMetricsReporter.h | New log reporter API. |
| media/server/main/include/IMetricsReporter.h | New reporter interface + report/alert structures. |
| media/server/main/include/CompositeMetricsReporter.h | New composite reporter API. |
| media/server/main/CMakeLists.txt | Adds new server-main metrics sources to build. |
| media/server/ipc/source/SessionManagementServer.cpp | Integrates PrivateMetricsModuleService and forwards app-state changes to it. |
| media/server/ipc/source/PrivateMetricsModuleService.cpp | New IPC module handling RPCs/events and implementing IMetricsCollectorClient. |
| media/server/ipc/source/MediaPipelineModuleService.cpp | Includes PrivateMetrics module header (integration point). |
| media/server/ipc/source/MediaPipelineClient.cpp | Formatting-only constructor signature change. |
| media/server/ipc/source/IpcFactory.cpp | Wires PrivateMetricsModuleServiceFactory into session management server creation. |
| media/server/ipc/interface/ISessionManagementServer.h | Adds notifyApplicationStateChanged to session management server interface. |
| media/server/ipc/include/SessionManagementServer.h | Adds private metrics module factory + module member + notifier method. |
| media/server/ipc/include/PrivateMetricsModuleService.h | New IPC service header + factory + IMetricsCollectorClient implementation. |
| media/server/ipc/include/IPrivateMetricsModuleService.h | New IPC interface for PrivateMetrics module service/factory. |
| media/server/ipc/include/IMediaPipelineModuleService.h | Minor whitespace/formatting adjustment. |
| media/server/ipc/CMakeLists.txt | Adds PrivateMetricsModuleService source to build. |
| media/server/gstplayer/source/tasks/generic/SetupElement.cpp | Applies environment fallback geometry only when API geometry not set. |
| media/server/gstplayer/source/GstGenericPlayer.cpp | Adds env parsing for default geometry, playbin sink overrides, and render-rectangle support. |
| media/server/gstplayer/include/GenericPlayerContext.h | Adds default geometry and an “set by API” atomic flag to context. |
| media/client/main/source/ClientController.cpp | Creates PrivateMetricsIpc and reports CPU/memory samples on server request. |
| media/client/main/include/ClientController.h | Implements IPrivateMetricsIpcClient and adds metrics sampling helpers and member. |
| media/client/ipc/source/PrivateMetricsIpc.cpp | New client IPC module to notify readiness, report metrics RPCs, and process sample request events. |
| media/client/ipc/interface/IPrivateMetricsIpc.h | New client IPC interfaces for metrics module. |
| media/client/ipc/include/PrivateMetricsIpc.h | New client IPC header for metrics module implementation. |
| media/client/ipc/CMakeLists.txt | Adds PrivateMetricsIpc source to build. |
| docs/ServerManagerDesign.html | Adds design documentation (includes metrics-related control/data request discussion). |
| docs/MetricsDesign.md | Adds full system design documentation for metrics gathering. |
| docs/metrics/RialtoMetricsReport.md | Adds a findings/report document describing metrics output and production observations. |
| cmake/wpeframeworkcore-config.cmake | Adds a CMake package config file (currently machine-path specific). |
| cmake/wpeframeworkcom-config.cmake | Adds a CMake package config file (currently machine-path specific). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+151
to
+154
| void PlaybackServiceTests::getPrivateMetricsServiceShouldSucceed() | ||
| { | ||
| EXPECT_NE(&m_sut->getPrivateMetricsService(), nullptr); | ||
| } |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| kServerMetrics.cgroupMemoryUsageKb, kServerMetrics.cgroupMemoryLimitKb); | ||
|
|
||
| std::lock_guard<std::mutex> lock{m_mutex}; | ||
| m_previousSample = PreviousSample{metrics.monotonicTimeMs, metrics.processCpuTimeMs, metrics.processMemoryKb, |
| // Update previous sample | ||
| { | ||
| std::lock_guard<std::mutex> lock{m_mutex}; | ||
| m_previousSample = |
| // Update previous sample | ||
| { | ||
| std::lock_guard<std::mutex> lock{m_mutex}; | ||
| m_previousSample = |
| { | ||
| if (sline.rfind("Pss_Shmem:", 0) == 0) | ||
| { | ||
| std::sscanf(sline.c_str(), "Pss_Shmem: %" SCNu64, &shmMemoryKb); |
| { | ||
| if (line.rfind("VmRSS:", 0) == 0) | ||
| { | ||
| std::sscanf(line.c_str(), "VmRSS: %" SCNu64, &serverMemoryKb); |
| else if (sline.rfind("Private_Clean:", 0) == 0) | ||
| std::sscanf(sline.c_str(), "Private_Clean: %" SCNu64, &privateCleanKb); | ||
| else if (sline.rfind("Private_Dirty:", 0) == 0) | ||
| std::sscanf(sline.c_str(), "Private_Dirty: %" SCNu64, &privateDirtyKb); |
| { | ||
| std::lock_guard<std::mutex> lock{m_mutex}; | ||
| m_clientIds[ipcClient] = kClientId; | ||
| m_ipcClients[kClientId] = ipcClient; |
| auto report{sessionState.aggregator.finalize(kNowMs)}; | ||
| StateTransitionReport transitionReport; | ||
| transitionReport.context = context; | ||
| transitionReport.metrics = report; |
| auto report{m_globalAggregator.finalize(kNowMs)}; | ||
| StateTransitionReport transitionReport; | ||
| transitionReport.context = "global"; | ||
| transitionReport.metrics = report; |
|
|
||
| namespace firebolt::rialto::server | ||
| { | ||
| MetricsThresholdChecker::MetricsThresholdChecker(MetricsThresholdConfig config, IMetricsReporter *reporter) |
Adjust threshold for CPU change on the metrics sample periodic message Signed-off-by: Douglas Adler <douglas.adler@yahoo.com>
|
media/server/main/source/MetricsCollector.cpp:124:9: style: Condition 'becameUnresponsive' is always false [knownConditionTrueFalse] |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 94 out of 94 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
tests/unittests/media/server/service/playbackService/PlaybackServiceTestsFixture.cpp:154
- This test is checking &getPrivateMetricsService() against nullptr, but taking the address of a reference is always non-null in well-defined C++. As written, the assertion can never fail and doesn't validate behavior. Prefer an assertion that reflects what can actually be guaranteed here (e.g., that the accessor doesn't throw).
media/client/ipc/source/PrivateMetricsIpc.cpp:36 - sampleReasonToString() doesn't handle METRICS_SAMPLE_REASON_STATE_TRANSITION, so logs will label state-transition sample requests as UNKNOWN. This makes troubleshooting harder and can mislead log-based analysis.
case firebolt::rialto::METRICS_SAMPLE_REASON_CONNECTED:
return "CONNECTED";
case firebolt::rialto::METRICS_SAMPLE_REASON_PERIODIC:
return "PERIODIC";
case firebolt::rialto::METRICS_SAMPLE_REASON_UNKNOWN:
media/server/service/source/PrivateMetricsService.cpp:114
- notifyApplicationStateChanged() holds m_mutex while performing synchronous /proc and cgroup file I/O for the INACTIVE snapshot. That can block reportMetrics/clientReady/clientDisconnected and state notifications, increasing tail latency around state transitions. Unlock before the snapshot I/O once shared state (m_currentApplicationState + collector notifications) is updated.
media/server/gstplayer/source/GstGenericPlayer.cpp:148 - setRenderRectangleProperty() calls g_value_set_int() and g_object_set_property() directly, bypassing IGlibWrapper. The rest of GstGenericPlayer uses wrapper APIs (e.g., gObjectSet/gValueInit/gValueUnset) so tests can mock GLib interactions; these direct calls break that pattern and make the behavior harder to unit test consistently.
bool setRenderRectangleProperty(const std::shared_ptr<firebolt::rialto::wrappers::IGstWrapper> &gstWrapper,
const std::shared_ptr<firebolt::rialto::wrappers::IGlibWrapper> &glibWrapper,
GstElement *videoSink, const firebolt::rialto::server::Rectangle &rectangle)
{
GValue renderRectangle = G_VALUE_INIT;
glibWrapper->gValueInit(&renderRectangle, GST_TYPE_ARRAY);
auto appendCoordinate = [&](int coordinate) {
GValue value = G_VALUE_INIT;
glibWrapper->gValueInit(&value, G_TYPE_INT);
g_value_set_int(&value, coordinate);
gstWrapper->gstValueArrayAppendValue(&renderRectangle, &value);
glibWrapper->gValueUnset(&value);
};
appendCoordinate(rectangle.x);
appendCoordinate(rectangle.y);
appendCoordinate(rectangle.width);
appendCoordinate(rectangle.height);
g_object_set_property(G_OBJECT(videoSink), "render-rectangle", &renderRectangle);
glibWrapper->gValueUnset(&renderRectangle);
return true;
Comment on lines
+113
to
+130
| auto ipcClient{ipcController->getClient()}; | ||
| const int kClientId{m_nextClientId.fetch_add(1)}; | ||
| { | ||
| std::lock_guard<std::mutex> lock{m_mutex}; | ||
| m_clientIds[ipcClient] = kClientId; | ||
| m_ipcClients[kClientId] = ipcClient; | ||
| } | ||
|
|
||
| RIALTO_SERVER_LOG_MIL("Client ready for private metrics samples, assigned clientId=%d", kClientId); | ||
| done->Run(); | ||
|
|
||
| // Create a shared_ptr to this as IMetricsCollectorClient, aliasing with shared_from_this() | ||
| // so the IPC layer stays alive as long as the MetricsCollector holds a reference. | ||
| auto self = shared_from_this(); | ||
| std::shared_ptr<firebolt::rialto::server::IMetricsCollectorClient> clientInterface( | ||
| self, static_cast<firebolt::rialto::server::IMetricsCollectorClient *>(this)); | ||
| m_metricsService.clientReady(kClientId, clientInterface); | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Coverage statistics of your commit: |
|
media/server/main/source/MetricsCollector.cpp:124:9: style: Condition 'becameUnresponsive' is always false [knownConditionTrueFalse] |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 94 out of 94 changed files in this pull request and generated no new comments.
Suppressed comments (5)
media/client/ipc/source/PrivateMetricsIpc.cpp:39
- sampleReasonToString() is missing handling for METRICS_SAMPLE_REASON_STATE_TRANSITION, so state-transition reports will be logged as "UNKNOWN". This makes debugging and log correlation harder.
const char *sampleReasonToString(::firebolt::rialto::MetricsSampleReason reason)
{
switch (reason)
{
case firebolt::rialto::METRICS_SAMPLE_REASON_CONNECTED:
return "CONNECTED";
case firebolt::rialto::METRICS_SAMPLE_REASON_PERIODIC:
return "PERIODIC";
case firebolt::rialto::METRICS_SAMPLE_REASON_UNKNOWN:
default:
return "UNKNOWN";
}
tests/unittests/media/server/service/playbackService/PlaybackServiceTestsFixture.cpp:154
- This test assertion is ineffective: getPrivateMetricsService() returns a reference, so taking its address can never produce nullptr. As written, the test will always pass even if the implementation changes in a way that would be problematic.
media/server/service/source/PrivateMetricsService.cpp:46 - clientReady() unconditionally dereferences m_collectorFactory. IMetricsCollectorFactory::createFactory() can return nullptr on failure, which would make this crash (and it would be hard to diagnose because it occurs on the first clientReady call). Add a nullptr guard with a clear log message.
media/server/service/source/PrivateMetricsService.cpp:116 - notifyApplicationStateChanged() holds m_mutex while doing multiple filesystem reads (/proc, /sys/fs/cgroup) when transitioning to INACTIVE. This can block concurrent clientReady/reportMetrics/clientDisconnected calls unnecessarily. Consider releasing the mutex after updating state and notifying collectors, then performing the INACTIVE snapshot IO without holding the lock.
media/server/gstplayer/source/GstGenericPlayer.cpp:148 - setRenderRectangleProperty() calls g_value_set_int() and g_object_set_property() directly instead of going through the existing wrapper layer. This reduces unit-testability/consistency (most GLib/GObject interactions in this component use IGlibWrapper/IGstWrapper) and makes it harder to mock these calls in tests.
bool setRenderRectangleProperty(const std::shared_ptr<firebolt::rialto::wrappers::IGstWrapper> &gstWrapper,
const std::shared_ptr<firebolt::rialto::wrappers::IGlibWrapper> &glibWrapper,
GstElement *videoSink, const firebolt::rialto::server::Rectangle &rectangle)
{
GValue renderRectangle = G_VALUE_INIT;
glibWrapper->gValueInit(&renderRectangle, GST_TYPE_ARRAY);
auto appendCoordinate = [&](int coordinate) {
GValue value = G_VALUE_INIT;
glibWrapper->gValueInit(&value, G_TYPE_INT);
g_value_set_int(&value, coordinate);
gstWrapper->gstValueArrayAppendValue(&renderRectangle, &value);
glibWrapper->gValueUnset(&value);
};
appendCoordinate(rectangle.x);
appendCoordinate(rectangle.y);
appendCoordinate(rectangle.width);
appendCoordinate(rectangle.height);
g_object_set_property(G_OBJECT(videoSink), "render-rectangle", &renderRectangle);
glibWrapper->gValueUnset(&renderRectangle);
return true;
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
media/server/main/source/MetricsCollector.cpp:124:9: style: Condition 'becameUnresponsive' is always false [knownConditionTrueFalse] |
|
Coverage statistics of your commit: |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 94 out of 94 changed files in this pull request and generated no new comments.
Suppressed comments (8)
tests/unittests/media/server/service/playbackService/PlaybackServiceTestsFixture.cpp:154
- This assertion is always true because
getPrivateMetricsService()returns a reference and taking its address can never producenullptr. Consider asserting the accessor returns a stable instance (not a temporary/new object).
media/server/gstplayer/source/GstGenericPlayer.cpp:147 g_object_set_propertyis called directly here, bypassing the existingIGlibWrapperabstraction used throughout the player. This reduces testability/consistency; consider routing property setting via the wrapper (or extending it to coverg_object_set_property).
g_object_set_property(G_OBJECT(videoSink), "render-rectangle", &renderRectangle);
glibWrapper->gValueUnset(&renderRectangle);
media/server/service/source/SessionServerManager.cpp:215
oldStateis hard-coded toINACTIVE, butswitchToActive()can be entered fromNOT_RUNNING/uninitialised flows (seeswitchToNotRunning()setting app state toUNKNOWN). This makes the emitted application-state transition inaccurate for metrics.
media/client/main/source/ClientController.cpp:29ClientController.cppusesstd::runtime_errorbut does not include<stdexcept>in this translation unit (andClientController.halso doesn’t). This can break compilation depending on indirect includes.
#include <cinttypes>
#include <cstdio>
#include <cstring>
#include <fstream>
#include <sys/mman.h>
media/server/service/source/SessionServerManager.cpp:236
switchToInactive()always reportsRUNNING -> INACTIVEeven when the previous server state wasn’t ACTIVE (e.g., NOT_RUNNING/uninitialised). For metrics/state tracking, the old state should be derived fromm_currentState.
media/server/service/source/PrivateMetricsService.cpp:120notifyApplicationStateChanged()holdsm_mutexwhile doing multiple filesystem reads (/proc,/sys/fs/cgroup) and parsing. This can blockreportMetrics()and other state notifications unnecessarily. Consider unlocking oncem_currentApplicationStateis updated and collectors have been notified, before taking the INACTIVE snapshot.
media/server/gstplayer/source/GstGenericPlayer.cpp:137- This helper bypasses the project’s GLib/GObject wrappers by calling
g_value_set_intdirectly. That makes the code harder to unit test with the existing wrapper mocks (and is inconsistent with the nearby wrapper usage forgValueInit/gValueUnset).
This issue also appears on line 145 of the same file.
auto appendCoordinate = [&](int coordinate) {
GValue value = G_VALUE_INIT;
glibWrapper->gValueInit(&value, G_TYPE_INT);
g_value_set_int(&value, coordinate);
gstWrapper->gstValueArrayAppendValue(&renderRectangle, &value);
media/server/gstplayer/source/tasks/generic/SetupElement.cpp:377
- This PR is titled/described as adding CPU/Memory metrics, but this hunk introduces new behaviour for video geometry fallback sourced from environment/defaults. If this is intentional, it should be called out in the PR description (or split into a separate PR) so reviewers can assess the additional surface area appropriately.
else if (!m_context.videoGeometrySetByApi.load() && !m_context.defaultVideoGeometry.empty())
{
m_context.pendingGeometry = m_context.defaultVideoGeometry;
m_player.setVideoSinkRectangle();
}
|
Coverage statistics of your commit: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RDKEMW-18181: CPU and Memory metrics
Reason for change: Add automatic metrics collection for CPU and Memory usage
for both the client and server sessions. Report data on >10% change or state
transition.
Test Procedure: Rialto CI