Conversation
|
Pull request must be merged with a description containing the required fields, Summary: 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. |
There was a problem hiding this comment.
Pull request overview
Integrates Telemetry (T2) markers into Rialto client/server code paths and wires the telemetry dependency into the build system.
Changes:
- Add a
RialtoTelemetry.hwrapper and emit telemetry string events on various error paths (client IPC, client main, server IPC/main). - Initialize telemetry in the server process (
TELEMETRY_INIT("rialto-server")). - Introduce CMake integration for a
telemetryimported target (Findtelemetry.cmake) and link telemetry into multiple targets.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
| media/server/service/source/main.cpp | Adds telemetry include and initializes telemetry in server main. |
| media/server/service/CMakeLists.txt | Links telemetry to server service target (but CMake formatting is broken). |
| media/server/main/source/MediaKeysCapabilities.cpp | Emits telemetry on factory creation failures; constructor signature formatting changed. |
| media/server/main/CMakeLists.txt | Links telemetry to server main target (but CMake formatting is broken). |
| media/server/ipc/source/MediaKeysModuleService.cpp | Adds telemetry events on factory/service creation failures; changes key session/create & generateRequest call shapes. |
| media/server/ipc/source/MediaKeysCapabilitiesModuleService.cpp | Adds telemetry events on factory/service creation failures. |
| media/server/ipc/CMakeLists.txt | Links telemetry to server IPC target (but CMake formatting is broken). |
| media/server/CMakeLists.txt | Links telemetry to server executable (but CMake formatting is broken). |
| media/CMakeLists.txt | Adds find_package(telemetry REQUIRED) and includes server subdir conditionally. |
| media/client/main/source/MediaPipelineCapabilities.cpp | Adds telemetry events on factory/capabilities creation failures. |
| media/client/main/source/MediaKeysCapabilities.cpp | Adds telemetry events on factory/capabilities creation failures. |
| media/client/main/source/MediaKeys.cpp | Adds telemetry events; changes MediaKeys API usage (adds isLDL, removes ldlState in generateRequest). |
| media/client/main/source/ClientController.cpp | Adds telemetry on shared-memory initialization failures; minor shared_ptr push_back change. |
| media/client/main/CMakeLists.txt | Links telemetry to RialtoClient (but CMake formatting is broken). |
| media/client/ipc/source/WebAudioPlayerIpc.cpp | Adds telemetry events on IPC failures for multiple methods. |
| media/client/ipc/source/MediaPipelineIpc.cpp | Adds telemetry events on IPC failures for multiple methods. |
| media/client/ipc/source/MediaPipelineCapabilitiesIpc.cpp | Adds telemetry events on IPC failures; includes <cstdio>. |
| media/client/ipc/source/MediaKeysIpc.cpp | Adds telemetry events; changes createKeySession/generateRequest signatures & protobuf request fields. |
| media/client/ipc/source/MediaKeysCapabilitiesIpc.cpp | Adds telemetry events on IPC failures. |
| media/client/ipc/source/IpcClient.cpp | Adds telemetry on unexpected IPC disconnect; minor change to closure factory call. |
| media/client/ipc/source/ControlIpc.cpp | Adds telemetry events on IPC failures and schema mismatch; marker string typo. |
| media/client/ipc/CMakeLists.txt | Links telemetry to client IPC impl (but CMake formatting is broken). |
| media/client/common/include/RialtoClientLogging.h | Adds <cstdio> include. |
| logging/include/RialtoTelemetry.h | New telemetry macro wrapper around T2 APIs. |
| logging/CMakeLists.txt | Adds RialtoTelemetry.h to logging target (but CMake formatting is broken). |
| cmake/Findtelemetry.cmake | New CMake find-module for telemetry include/library and imported telemetry target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| add_library(RialtoServerService STATIC | ||
|
|
||
| source/ApplicationSessionServer.cpp | ||
| source/PlaybackService.cpp | ||
| source/CdmService.cpp | ||
| source/ControlService.cpp | ||
| source/SessionServerManager.cpp | ||
| source/MediaPipelineService.cpp | ||
| source/WebAudioPlayerService.cpp | ||
| ) | ||
| set_target_properties ( | ||
| RialtoServerService | ||
| PROPERTIES LINK_FLAGS "-Wl,--unresolved-symbols=report-all" POSITION_INDEPENDENT_CODE ON | ||
| ) | ||
| target_include_directories ( | ||
| RialtoServerService | ||
| source / | ||
| ApplicationSessionServer.cpp source / PlaybackService.cpp source / CdmService.cpp source / | ||
| ControlService.cpp source / SessionServerManager.cpp source / MediaPipelineService.cpp source / | ||
| WebAudioPlayerService.cpp) |
There was a problem hiding this comment.
The CMake syntax here is broken: paths are split into separate tokens (e.g. source / and ApplicationSessionServer.cpp on the next token), which will be interpreted as three separate list items and will fail configuration. Please restore valid CMake list entries like source/ApplicationSessionServer.cpp (one token each) and consistent indentation.
| set(CMAKE_CXX_STANDARD_REQUIRED ON) include(CheckCXXCompilerFlag) | ||
|
|
||
| #Find includes in corresponding build directories | ||
| set(CMAKE_INCLUDE_CURRENT_DIR ON) | ||
|
|
||
| add_library(RialtoServerMain STATIC | ||
|
|
||
| ${PROTO_SRCS} ${PROTO_HEADERS} | ||
|
|
||
| source / | ||
| MediaPipelineServerInternal.cpp source / MediaPipelineCapabilities.cpp source / | ||
| ActiveRequests.cpp source / DataReaderFactory.cpp source / DataReaderV1.cpp source / | ||
| DataReaderV2.cpp source / NeedMediaData.cpp source / SharedMemoryBuffer.cpp source / | ||
| MediaKeysServerInternal.cpp source / MediaKeysCapabilities.cpp source / | ||
| MediaKeySession.cpp source / MainThread.cpp source / WebAudioPlayerServerInternal.cpp source / | ||
| ControlServerInternal.cpp source / HeartbeatProcedure.cpp source / | ||
| TextTrackAccessor.cpp source / TextTrackSession.cpp) | ||
|
|
||
| target_include_directories( | ||
| RialtoServerMain | ||
|
|
||
| PUBLIC interface ${GStreamerApp_INCLUDE_DIRS} | ||
|
|
||
| PRIVATE include $<TARGET_PROPERTY : RialtoPlayerPublic, INTERFACE_INCLUDE_DIRECTORIES> | ||
| $<TARGET_PROPERTY : RialtoServerGstPlayer, INTERFACE_INCLUDE_DIRECTORIES> | ||
| $<TARGET_PROPERTY : RialtoServerCommon, INTERFACE_INCLUDE_DIRECTORIES> | ||
| $<TARGET_PROPERTY : RialtoPlayerCommon, INTERFACE_INCLUDE_DIRECTORIES> | ||
| $<TARGET_PROPERTY : RialtoWrappers, INTERFACE_INCLUDE_DIRECTORIES> | ||
| $<TARGET_PROPERTY : RialtoCommon, INTERFACE_INCLUDE_DIRECTORIES>) | ||
|
|
||
| set_target_properties(RialtoServerMain PROPERTIES LINK_FLAGS "-Wl,--unresolved-symbols=report-all" | ||
|
|
||
| ) | ||
|
|
||
| target_compile_options(RialtoServerMain | ||
|
|
||
| PRIVATE) | ||
|
|
||
| target_link_libraries(RialtoServerMain | ||
|
|
||
| PRIVATE RialtoLogging RialtoServerGstPlayer RialtoWrappers | ||
| RialtoCommon RialtoProtobuf telemetry Threads::Threads) | ||
|
|
||
| install(TARGETS RialtoServerMain DESTINATION lib) |
There was a problem hiding this comment.
This CMake file has invalid formatting that will break parsing (e.g. set(CMAKE_CXX_STANDARD_REQUIRED ON) include(CheckCXXCompilerFlag) is on one line, and source file paths are split into tokens like source / MediaPipelineServerInternal.cpp). Please revert to standard multi-line CMake commands and unsplit file paths (e.g. source/MediaPipelineServerInternal.cpp).
| set(CMAKE_CXX_STANDARD_REQUIRED ON) include(CheckCXXCompilerFlag) | |
| #Find includes in corresponding build directories | |
| set(CMAKE_INCLUDE_CURRENT_DIR ON) | |
| add_library(RialtoServerMain STATIC | |
| ${PROTO_SRCS} ${PROTO_HEADERS} | |
| source / | |
| MediaPipelineServerInternal.cpp source / MediaPipelineCapabilities.cpp source / | |
| ActiveRequests.cpp source / DataReaderFactory.cpp source / DataReaderV1.cpp source / | |
| DataReaderV2.cpp source / NeedMediaData.cpp source / SharedMemoryBuffer.cpp source / | |
| MediaKeysServerInternal.cpp source / MediaKeysCapabilities.cpp source / | |
| MediaKeySession.cpp source / MainThread.cpp source / WebAudioPlayerServerInternal.cpp source / | |
| ControlServerInternal.cpp source / HeartbeatProcedure.cpp source / | |
| TextTrackAccessor.cpp source / TextTrackSession.cpp) | |
| target_include_directories( | |
| RialtoServerMain | |
| PUBLIC interface ${GStreamerApp_INCLUDE_DIRS} | |
| PRIVATE include $<TARGET_PROPERTY : RialtoPlayerPublic, INTERFACE_INCLUDE_DIRECTORIES> | |
| $<TARGET_PROPERTY : RialtoServerGstPlayer, INTERFACE_INCLUDE_DIRECTORIES> | |
| $<TARGET_PROPERTY : RialtoServerCommon, INTERFACE_INCLUDE_DIRECTORIES> | |
| $<TARGET_PROPERTY : RialtoPlayerCommon, INTERFACE_INCLUDE_DIRECTORIES> | |
| $<TARGET_PROPERTY : RialtoWrappers, INTERFACE_INCLUDE_DIRECTORIES> | |
| $<TARGET_PROPERTY : RialtoCommon, INTERFACE_INCLUDE_DIRECTORIES>) | |
| set_target_properties(RialtoServerMain PROPERTIES LINK_FLAGS "-Wl,--unresolved-symbols=report-all" | |
| ) | |
| target_compile_options(RialtoServerMain | |
| PRIVATE) | |
| target_link_libraries(RialtoServerMain | |
| PRIVATE RialtoLogging RialtoServerGstPlayer RialtoWrappers | |
| RialtoCommon RialtoProtobuf telemetry Threads::Threads) | |
| install(TARGETS RialtoServerMain DESTINATION lib) | |
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | |
| include(CheckCXXCompilerFlag) | |
| #Find includes in corresponding build directories | |
| set(CMAKE_INCLUDE_CURRENT_DIR ON) | |
| add_library(RialtoServerMain STATIC | |
| ${PROTO_SRCS} | |
| ${PROTO_HEADERS} | |
| source/MediaPipelineServerInternal.cpp | |
| source/MediaPipelineCapabilities.cpp | |
| source/ActiveRequests.cpp | |
| source/DataReaderFactory.cpp | |
| source/DataReaderV1.cpp | |
| source/DataReaderV2.cpp | |
| source/NeedMediaData.cpp | |
| source/SharedMemoryBuffer.cpp | |
| source/MediaKeysServerInternal.cpp | |
| source/MediaKeysCapabilities.cpp | |
| source/MediaKeySession.cpp | |
| source/MainThread.cpp | |
| source/WebAudioPlayerServerInternal.cpp | |
| source/ControlServerInternal.cpp | |
| source/HeartbeatProcedure.cpp | |
| source/TextTrackAccessor.cpp | |
| source/TextTrackSession.cpp) | |
| target_include_directories( | |
| RialtoServerMain | |
| PUBLIC | |
| interface | |
| ${GStreamerApp_INCLUDE_DIRS} | |
| PRIVATE | |
| include | |
| $<TARGET_PROPERTY:RialtoPlayerPublic,INTERFACE_INCLUDE_DIRECTORIES> | |
| $<TARGET_PROPERTY:RialtoServerGstPlayer,INTERFACE_INCLUDE_DIRECTORIES> | |
| $<TARGET_PROPERTY:RialtoServerCommon,INTERFACE_INCLUDE_DIRECTORIES> | |
| $<TARGET_PROPERTY:RialtoPlayerCommon,INTERFACE_INCLUDE_DIRECTORIES> | |
| $<TARGET_PROPERTY:RialtoWrappers,INTERFACE_INCLUDE_DIRECTORIES> | |
| $<TARGET_PROPERTY:RialtoCommon,INTERFACE_INCLUDE_DIRECTORIES>) | |
| set_target_properties(RialtoServerMain PROPERTIES | |
| LINK_FLAGS "-Wl,--unresolved-symbols=report-all") | |
| target_compile_options(RialtoServerMain | |
| PRIVATE) | |
| target_link_libraries(RialtoServerMain | |
| PRIVATE | |
| RialtoLogging | |
| RialtoServerGstPlayer | |
| RialtoWrappers | |
| RialtoCommon | |
| RialtoProtobuf | |
| telemetry | |
| Threads::Threads) | |
| install(TARGETS RialtoServerMain DESTINATION lib) |
| source / | ||
| AckSender.cpp source / ApplicationManagementServer.cpp source / IpcFactory.cpp source / | ||
| MediaPipelineClient.cpp source / MediaPipelineModuleService.cpp source / | ||
| MediaPipelineCapabilitiesModuleService.cpp source / MediaKeysClient.cpp source / | ||
| MediaKeysModuleService.cpp source / MediaKeysCapabilitiesModuleService.cpp source / | ||
| ControlClientServerInternal.cpp source / ControlModuleService.cpp source / | ||
| ServerManagerModuleService.cpp source / SessionManagementServer.cpp source / | ||
| SetLogLevelsService.cpp source / RialtoCommonModule.cpp source / WebAudioPlayerClient.cpp source / | ||
| WebAudioPlayerModuleService.cpp) |
There was a problem hiding this comment.
The add_library source list is malformed (source / AckSender.cpp ...) which will be treated as separate tokens and fail to locate source files. Please restore proper paths like source/AckSender.cpp and keep each source as a single list entry.
| source / | |
| AckSender.cpp source / ApplicationManagementServer.cpp source / IpcFactory.cpp source / | |
| MediaPipelineClient.cpp source / MediaPipelineModuleService.cpp source / | |
| MediaPipelineCapabilitiesModuleService.cpp source / MediaKeysClient.cpp source / | |
| MediaKeysModuleService.cpp source / MediaKeysCapabilitiesModuleService.cpp source / | |
| ControlClientServerInternal.cpp source / ControlModuleService.cpp source / | |
| ServerManagerModuleService.cpp source / SessionManagementServer.cpp source / | |
| SetLogLevelsService.cpp source / RialtoCommonModule.cpp source / WebAudioPlayerClient.cpp source / | |
| WebAudioPlayerModuleService.cpp) | |
| source/AckSender.cpp | |
| source/ApplicationManagementServer.cpp | |
| source/IpcFactory.cpp | |
| source/MediaPipelineClient.cpp | |
| source/MediaPipelineModuleService.cpp | |
| source/MediaPipelineCapabilitiesModuleService.cpp | |
| source/MediaKeysClient.cpp | |
| source/MediaKeysModuleService.cpp | |
| source/MediaKeysCapabilitiesModuleService.cpp | |
| source/ControlClientServerInternal.cpp | |
| source/ControlModuleService.cpp | |
| source/ServerManagerModuleService.cpp | |
| source/SessionManagementServer.cpp | |
| source/SetLogLevelsService.cpp | |
| source/RialtoCommonModule.cpp | |
| source/WebAudioPlayerClient.cpp | |
| source/WebAudioPlayerModuleService.cpp) |
| set(CMAKE_CXX_STANDARD_REQUIRED ON) include(CheckCXXCompilerFlag) | ||
|
|
||
| add_subdirectory(common) | ||
| add_subdirectory(gstplayer) | ||
| add_subdirectory(main) | ||
| add_subdirectory(ipc) | ||
| add_subdirectory(service) | ||
| add_subdirectory(common) add_subdirectory(gstplayer) add_subdirectory(main) add_subdirectory(ipc) | ||
| add_subdirectory(service) | ||
|
|
||
| add_executable ( | ||
| RialtoServer | ||
| add_executable(RialtoServer | ||
|
|
||
| service/source/main.cpp | ||
| ) | ||
| service / | ||
| source / main.cpp) | ||
|
|
There was a problem hiding this comment.
This CMakeLists.txt is syntactically invalid: multiple commands are concatenated on the same line (e.g. add_subdirectory(common) add_subdirectory(gstplayer) ...) and the executable source path is split into tokens (service / source / main.cpp). This will break CMake configuration; please restore standard formatting and unsplit paths/commands.
| add_subdirectory(public) | ||
| add_subdirectory(common) | ||
| add_subdirectory(client) | ||
| find_package(telemetry REQUIRED) |
There was a problem hiding this comment.
find_package(telemetry REQUIRED) is likely to fail with the provided Findtelemetry.cmake, because that module calls find_package_handle_standard_args(TELEMETRY ...) and sets TELEMETRY_FOUND (uppercase) rather than telemetry_FOUND expected by find_package(telemetry ...). Align the package name/variables (e.g. use find_package_handle_standard_args(telemetry ...) or call find_package(TELEMETRY ...) with a matching FindTELEMETRY.cmake).
| find_package(telemetry REQUIRED) | |
| find_package(TELEMETRY REQUIRED) |
| MediaKeyErrorStatus status = m_cdmService.generateRequest(request->media_keys_handle(), request->key_session_id(), | ||
| covertInitDataType(request->init_data_type()), | ||
| std::vector<std::uint8_t>{request->init_data().begin(), | ||
| request->init_data().end()}); |
There was a problem hiding this comment.
ICdmService::generateRequest currently requires an ldlState parameter, but this call now passes only (handle, sessionId, initDataType, initData). Unless the service interface/protobuf have been updated everywhere, this is a compile break and changes behavior (LDL state no longer reaches the server). Please align the function signatures across IPC/service layers (and update protobuf/messages/tests as needed).
| request->init_data().end()}); | |
| request->init_data().end()}, | |
| request->is_ldl()); |
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to get the shared memory due to '%s'", | ||
| ipcController->ErrorText().c_str()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); |
There was a problem hiding this comment.
The telemetry marker string has a spelling mismatch ("Rialto Client - ControllIpc"). If telemetry dashboards/filters depend on stable marker names, this typo will fragment metrics. Please correct the marker to "Rialto Client - ControlIpc" (and keep it consistent across all events from this module).
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); | |
| TELEMETRY_EVENT_STRING("Rialto Client - ControlIpc", telemetryBuff); |
| { | ||
| RIALTO_CLIENT_LOG_ERROR("Could not initalise the shared memory"); | ||
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Could not initalise the shared memory"); |
There was a problem hiding this comment.
The telemetry message duplicates an existing log string with a typo ("Could not initalise the shared memory"). Since telemetry strings are newly introduced here, please fix the spelling to "Could not initialise the shared memory" to keep telemetry consistent and searchable.
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Could not initalise the shared memory"); | |
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Could not initialise the shared memory"); |
| set(CMAKE_C_STANDARD 99) set(CMAKE_CXX_STANDARD 17) | ||
|
|
||
| set(CMAKE_CXX_STANDARD_REQUIRED ON) include(CheckCCompilerFlag) include(CheckCXXCompilerFlag) | ||
|
|
||
| set(LIB_RIALTO_CLIENT_SOURCES source / AttachedSources.cpp source / ClientController.cpp source / | ||
| ClientLogControl.cpp source / Control.cpp source / KeyIdMap.cpp source / MediaKeys.cpp source / | ||
| MediaKeysCapabilities.cpp source / MediaPipeline.cpp source / MediaPipelineCapabilities.cpp source / | ||
| SharedMemoryHandle.cpp source / WebAudioPlayer.cpp) | ||
|
|
||
| add_library(RialtoClient SHARED ${LIB_RIALTO_CLIENT_SOURCES} | ||
|
|
||
| ) | ||
|
|
||
| target_compile_options(RialtoClient | ||
|
|
||
| PUBLIC - | ||
| DRIALTO_SERVER_SUPPORTS_DECRYPTION = 1) | ||
|
|
||
| target_include_directories(RialtoClient | ||
|
|
||
| PUBLIC ${PROJECT_SOURCE_DIR} / | ||
| include $ < | ||
| BUILD_INTERFACE | ||
| : ${CMAKE_CURRENT_SOURCE_DIR} / include > $ < INSTALL_INTERFACE | ||
| : include / rialto > | ||
|
|
||
| PRIVATE $ < TARGET_PROPERTY | ||
| : RialtoPlayerPublic, INTERFACE_INCLUDE_DIRECTORIES > $ < TARGET_PROPERTY | ||
| : RialtoPlayerCommon, INTERFACE_INCLUDE_DIRECTORIES > $ < TARGET_PROPERTY | ||
| : RialtoClientIpcImpl, INTERFACE_INCLUDE_DIRECTORIES > $ < TARGET_PROPERTY | ||
| : RialtoClientCommon, INTERFACE_INCLUDE_DIRECTORIES > $ < TARGET_PROPERTY | ||
| : RialtoCommon, INTERFACE_INCLUDE_DIRECTORIES > | ||
|
|
||
| ) | ||
|
|
||
| set_target_properties(RialtoClient PROPERTIES LINK_FLAGS | ||
| "-Wl,--unresolved-symbols=report-all" SOVERSION ${ | ||
| PROJECT_VERSION_MAJOR} VERSION ${CMAKE_PROJECT_VERSION}) | ||
|
|
||
| target_link_libraries( | ||
| RialtoClient | ||
|
|
||
| PRIVATE RialtoPlayerCommon RialtoClientIpcImpl RialtoCommon RialtoEthanLog telemetry | ||
|
|
||
| Threads::Threads | ||
|
|
||
| ) | ||
|
|
||
| include(GNUInstallDirs) | ||
|
|
||
| install(TARGETS RialtoClient LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}) |
There was a problem hiding this comment.
This CMakeLists.txt is currently not valid CMake: commands are concatenated on single lines (set(...) set(...)), generator expressions are split into tokens ($ < BUILD_INTERFACE : ... >), and file paths are broken up (source / AttachedSources.cpp). This will break configuration/build. Please restore standard CMake formatting with unsplit paths/expressions.
| set(CMAKE_C_STANDARD 99) set(CMAKE_CXX_STANDARD 17) | |
| set(CMAKE_CXX_STANDARD_REQUIRED ON) include(CheckCCompilerFlag) include(CheckCXXCompilerFlag) | |
| set(LIB_RIALTO_CLIENT_SOURCES source / AttachedSources.cpp source / ClientController.cpp source / | |
| ClientLogControl.cpp source / Control.cpp source / KeyIdMap.cpp source / MediaKeys.cpp source / | |
| MediaKeysCapabilities.cpp source / MediaPipeline.cpp source / MediaPipelineCapabilities.cpp source / | |
| SharedMemoryHandle.cpp source / WebAudioPlayer.cpp) | |
| add_library(RialtoClient SHARED ${LIB_RIALTO_CLIENT_SOURCES} | |
| ) | |
| target_compile_options(RialtoClient | |
| PUBLIC - | |
| DRIALTO_SERVER_SUPPORTS_DECRYPTION = 1) | |
| target_include_directories(RialtoClient | |
| PUBLIC ${PROJECT_SOURCE_DIR} / | |
| include $ < | |
| BUILD_INTERFACE | |
| : ${CMAKE_CURRENT_SOURCE_DIR} / include > $ < INSTALL_INTERFACE | |
| : include / rialto > | |
| PRIVATE $ < TARGET_PROPERTY | |
| : RialtoPlayerPublic, INTERFACE_INCLUDE_DIRECTORIES > $ < TARGET_PROPERTY | |
| : RialtoPlayerCommon, INTERFACE_INCLUDE_DIRECTORIES > $ < TARGET_PROPERTY | |
| : RialtoClientIpcImpl, INTERFACE_INCLUDE_DIRECTORIES > $ < TARGET_PROPERTY | |
| : RialtoClientCommon, INTERFACE_INCLUDE_DIRECTORIES > $ < TARGET_PROPERTY | |
| : RialtoCommon, INTERFACE_INCLUDE_DIRECTORIES > | |
| ) | |
| set_target_properties(RialtoClient PROPERTIES LINK_FLAGS | |
| "-Wl,--unresolved-symbols=report-all" SOVERSION ${ | |
| PROJECT_VERSION_MAJOR} VERSION ${CMAKE_PROJECT_VERSION}) | |
| target_link_libraries( | |
| RialtoClient | |
| PRIVATE RialtoPlayerCommon RialtoClientIpcImpl RialtoCommon RialtoEthanLog telemetry | |
| Threads::Threads | |
| ) | |
| include(GNUInstallDirs) | |
| install(TARGETS RialtoClient LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}) | |
| set(CMAKE_C_STANDARD 99) | |
| set(CMAKE_CXX_STANDARD 17) | |
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | |
| include(CheckCCompilerFlag) | |
| include(CheckCXXCompilerFlag) | |
| set(LIB_RIALTO_CLIENT_SOURCES | |
| source/AttachedSources.cpp | |
| source/ClientController.cpp | |
| source/ClientLogControl.cpp | |
| source/Control.cpp | |
| source/KeyIdMap.cpp | |
| source/MediaKeys.cpp | |
| source/MediaKeysCapabilities.cpp | |
| source/MediaPipeline.cpp | |
| source/MediaPipelineCapabilities.cpp | |
| source/SharedMemoryHandle.cpp | |
| source/WebAudioPlayer.cpp) | |
| add_library(RialtoClient SHARED ${LIB_RIALTO_CLIENT_SOURCES}) | |
| target_compile_options(RialtoClient | |
| PUBLIC -DRIALTO_SERVER_SUPPORTS_DECRYPTION=1) | |
| target_include_directories(RialtoClient | |
| PUBLIC ${PROJECT_SOURCE_DIR}/include | |
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include> | |
| $<INSTALL_INTERFACE:include/rialto> | |
| PRIVATE $<TARGET_PROPERTY:RialtoPlayerPublic,INTERFACE_INCLUDE_DIRECTORIES> | |
| $<TARGET_PROPERTY:RialtoPlayerCommon,INTERFACE_INCLUDE_DIRECTORIES> | |
| $<TARGET_PROPERTY:RialtoClientIpcImpl,INTERFACE_INCLUDE_DIRECTORIES> | |
| $<TARGET_PROPERTY:RialtoClientCommon,INTERFACE_INCLUDE_DIRECTORIES> | |
| $<TARGET_PROPERTY:RialtoCommon,INTERFACE_INCLUDE_DIRECTORIES>) | |
| set_target_properties(RialtoClient PROPERTIES LINK_FLAGS | |
| "-Wl,--unresolved-symbols=report-all" SOVERSION ${PROJECT_VERSION_MAJOR} | |
| VERSION ${CMAKE_PROJECT_VERSION}) | |
| target_link_libraries( | |
| RialtoClient | |
| PRIVATE RialtoPlayerCommon RialtoClientIpcImpl RialtoCommon RialtoEthanLog telemetry | |
| Threads::Threads) | |
| include(GNUInstallDirs) | |
| install(TARGETS RialtoClient LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}) |
| add_library(RialtoClientIpcImpl STATIC | ||
|
|
||
| source/IpcClient.cpp | ||
| source/IpcModule.cpp | ||
| source/MediaPipelineIpc.cpp | ||
| source/MediaPipelineCapabilitiesIpc.cpp | ||
| source/ControlIpc.cpp | ||
| source/MediaKeysIpc.cpp | ||
| source/MediaKeysCapabilitiesIpc.cpp | ||
| source/RialtoCommonIpc.cpp | ||
| source/WebAudioPlayerIpc.cpp | ||
| ) | ||
| source / | ||
| IpcClient.cpp source / IpcModule.cpp source / MediaPipelineIpc.cpp source / | ||
| MediaPipelineCapabilitiesIpc.cpp source / ControlIpc.cpp source / MediaKeysIpc.cpp source / | ||
| MediaKeysCapabilitiesIpc.cpp source / RialtoCommonIpc.cpp source / WebAudioPlayerIpc.cpp) | ||
|
|
There was a problem hiding this comment.
The add_library source list is malformed (source / IpcClient.cpp ...) so CMake will treat source, /, and IpcClient.cpp as separate list items and fail to find sources. Please restore proper paths like source/IpcClient.cpp (one token per path) and normal indentation.
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
8f6ecbf to
d9218c4
Compare
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
d9218c4 to
5e5f67c
Compare
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
5e5f67c to
97d6027
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| #include <stdarg.h> | ||
| #include <stdint.h> | ||
| #include "RialtoTelemetry.h" |
There was a problem hiding this comment.
Including RialtoTelemetry.h from this public logging header makes every target that includes RialtoLogging.h depend on the external telemetry headers/libs, but the RialtoLogging target itself is not linked to telemetry (and most non-media targets don’t link it either). This is likely to break compilation unless telemetry headers are globally available. Consider removing this include from RialtoLogging.h and including RialtoTelemetry.h only where needed, or make RialtoLogging link telemetry publicly (and ensure find_package(telemetry) is done at a scope that covers all users).
| #include "RialtoTelemetry.h" |
| #ifndef __RIALTO_TELEMETRY_H__ | ||
| #define __RIALTO_TELEMETRY_H__ | ||
|
|
There was a problem hiding this comment.
The include guard uses a double-underscore identifier (RIALTO_TELEMETRY_H), which is reserved in C/C++. Please rename the guard to a non-reserved form (e.g., FIREBOLT_RIALTO_LOGGING_RIALTO_TELEMETRY_H_).
| MediaKeyErrorStatus MediaKeys::createKeySession(KeySessionType sessionType, std::weak_ptr<IMediaKeysClient> client, | ||
| int32_t &keySessionId) | ||
| bool isLDL, int32_t &keySessionId) | ||
| { | ||
| RIALTO_CLIENT_LOG_DEBUG("entry:"); | ||
|
|
||
| auto result{m_mediaKeysIpc->createKeySession(sessionType, client, keySessionId)}; | ||
| auto result{m_mediaKeysIpc->createKeySession(sessionType, client, isLDL, keySessionId)}; | ||
| if (isNetflixPlayready(m_keySystem) && MediaKeyErrorStatus::OK == result) |
There was a problem hiding this comment.
MediaKeys::createKeySession signature was changed (added isLDL), but the class declaration in media/client/main/include/MediaKeys.h (and the public IMediaKeys interface) still uses the old signature. This will cause an override mismatch/compile failure until the corresponding headers/interfaces are updated consistently.
| MediaKeyErrorStatus MediaKeys::generateRequest(int32_t keySessionId, InitDataType initDataType, | ||
| const std::vector<uint8_t> &initData, | ||
| const LimitedDurationLicense &ldlState) | ||
| const std::vector<uint8_t> &initData) | ||
| { | ||
| RIALTO_CLIENT_LOG_DEBUG("entry:"); | ||
|
|
||
| return m_mediaKeysIpc->generateRequest(keySessionId, initDataType, initData, ldlState); | ||
| return m_mediaKeysIpc->generateRequest(keySessionId, initDataType, initData); | ||
| } |
There was a problem hiding this comment.
MediaKeys::generateRequest signature was changed to drop ldlState, but media/client/main/include/MediaKeys.h and media/public/include/IMediaKeys.h still declare the old overload (with LimitedDurationLicense). This will not compile and is an API-breaking change unless the headers/protos/callers are updated together.
| MediaKeyErrorStatus MediaKeysIpc::createKeySession(KeySessionType sessionType, std::weak_ptr<IMediaKeysClient> client, | ||
| int32_t &keySessionId) | ||
| bool isLDL, int32_t &keySessionId) | ||
| { |
There was a problem hiding this comment.
MediaKeysIpc::createKeySession now takes an extra isLDL parameter, but the declaration in media/client/ipc/include/MediaKeysIpc.h still has the old signature. Please update the header (and IMediaKeys interface, if applicable) to keep the override consistent.
| # | ||
| # Copyright 2026 Sky UK | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. |
There was a problem hiding this comment.
Findtelemetry.cmake uses find_package_handle_standard_args(TELEMETRY ...) which sets TELEMETRY_FOUND, but the project calls find_package(telemetry ...), which expects telemetry_FOUND. This case mismatch can cause find_package(telemetry REQUIRED) to fail even when the library is present. Use find_package_handle_standard_args(telemetry ...) (or otherwise set telemetry_FOUND) to match the package name.
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to register client due to '%s'", | ||
| ipcController->ErrorText().c_str()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); |
There was a problem hiding this comment.
Telemetry marker name has a typo ("ControllIpc"). Please rename it to "ControlIpc" to keep markers consistent across the codebase/metrics.
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); | |
| TELEMETRY_EVENT_STRING("Rialto Client - ControlIpc", telemetryBuff); |
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Server and Client proto schema versions are not compatible. Server schema version: " | ||
| "%s, Client schema version: %s", | ||
| kServerSchemaVersion.str().c_str(), kCurrentSchemaVersion.str().c_str()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); | ||
|
|
There was a problem hiding this comment.
Telemetry marker name has a typo ("ControllIpc"). Please rename it to "ControlIpc" to keep markers consistent across the codebase/metrics.
| RIALTO_CLIENT_LOG_ERROR("failed to ack due to '%s'", ipcController->ErrorText().c_str()); | ||
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to ack due to '%s'", ipcController->ErrorText().c_str()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); |
There was a problem hiding this comment.
Telemetry marker name has a typo ("ControllIpc"). Please rename it to "ControlIpc" to keep markers consistent across the codebase/metrics.
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); | |
| TELEMETRY_EVENT_STRING("Rialto Client - ControlIpc", telemetryBuff); |
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
97d6027 to
481fb37
Compare
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
481fb37 to
1d4948c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ExternalProject_Add( | ||
| telemetry-source | ||
| GIT_REPOSITORY https://github.com/rdkcentral/telemetry.git | ||
| GIT_TAG develop |
There was a problem hiding this comment.
The ExternalProject fetch uses GIT_TAG develop, which makes native builds non-reproducible and can break unexpectedly when the upstream branch changes. Pin this to a specific tag/commit SHA (or make it configurable with a cache variable) to keep builds deterministic.
| ExternalProject_Add( | |
| telemetry-source | |
| GIT_REPOSITORY https://github.com/rdkcentral/telemetry.git | |
| GIT_TAG develop | |
| set(TELEMETRY_GIT_TAG "develop" CACHE STRING "Git tag or commit SHA for the telemetry native build dependency; set this to a pinned tag or commit for reproducible builds") | |
| ExternalProject_Add( | |
| telemetry-source | |
| GIT_REPOSITORY https://github.com/rdkcentral/telemetry.git | |
| GIT_TAG ${TELEMETRY_GIT_TAG} |
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to get the shared memory due to '%s'", | ||
| ipcController->ErrorText().c_str()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); |
There was a problem hiding this comment.
Telemetry marker string has a typo: "ControllIpc" (double 'l'). This will fragment metrics under an incorrect marker name; please correct the marker identifier.
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); | |
| TELEMETRY_EVENT_STRING("Rialto Client - ControlIpc", telemetryBuff); |
| TELEMETRY_INIT("rialto-server"); | ||
| firebolt::rialto::server::IGstInitialiser::instance().initialise(&argc, &argv); | ||
|
|
There was a problem hiding this comment.
Telemetry is initialized but never uninitialized. If the telemetry library requires cleanup/flushing, consider calling TELEMETRY_UNINIT() on shutdown (e.g., via RAII or atexit) to avoid losing final events or leaking resources.
|
|
||
| #ifdef __cplusplus | ||
| extern "C" { | ||
| #endif | ||
|
|
||
| #define TELEMETRY_INIT(component) \ | ||
| do { \ | ||
| t2_init((char*)component); \ |
There was a problem hiding this comment.
TELEMETRY_INIT casts the component argument to char* before calling t2_init. Casting away const on string literals/const buffers is undefined behavior in C++ and can crash if the telemetry library writes into the buffer. Please avoid (char*) here (e.g., copy into a mutable buffer or provide a safer wrapper).
| #ifdef __cplusplus | |
| extern "C" { | |
| #endif | |
| #define TELEMETRY_INIT(component) \ | |
| do { \ | |
| t2_init((char*)component); \ | |
| #include <stdlib.h> | |
| #include <string.h> | |
| #ifdef __cplusplus | |
| extern "C" { | |
| #endif | |
| static inline void rialto_telemetry_init(const char *component) | |
| { | |
| if (component == NULL) | |
| { | |
| t2_init(NULL); | |
| return; | |
| } | |
| size_t componentLength = strlen(component) + 1; | |
| char *mutableComponent = (char *)malloc(componentLength); | |
| if (mutableComponent == NULL) | |
| { | |
| return; | |
| } | |
| memcpy(mutableComponent, component, componentLength); | |
| t2_init(mutableComponent); | |
| free(mutableComponent); | |
| } | |
| #define TELEMETRY_INIT(component) \ | |
| do { \ | |
| rialto_telemetry_init(component); \ |
| #define TELEMETRY_EVENT_STRING(marker, value) \ | ||
| do { \ | ||
| t2_event_s((char*)marker, (char*)value); \ | ||
| } while(0) |
There was a problem hiding this comment.
TELEMETRY_EVENT_STRING casts both marker and value to char*. Casting away const on string literals/const data is undefined behavior in C++; please avoid (char*) here (copy into mutable storage if required by the telemetry API).
| MediaKeyErrorStatus status = m_cdmService.generateRequest(request->media_keys_handle(), request->key_session_id(), | ||
| covertInitDataType(request->init_data_type()), | ||
| std::vector<std::uint8_t>{request->init_data().begin(), | ||
| request->init_data().end()}); | ||
| response->set_error_status(convertMediaKeyErrorStatus(status)); |
There was a problem hiding this comment.
generateRequest no longer forwards ldl_state from the protobuf request, but ICdmService::generateRequest still requires LimitedDurationLicense and the proto schema still contains ldl_state. This is a compilation issue (wrong call signature) and a behavior change that looks unrelated to the telemetry-focused PR description; please either keep forwarding ldl_state or update the API/proto consistently.
| MediaKeyErrorStatus MediaKeysIpc::generateRequest(int32_t keySessionId, InitDataType initDataType, | ||
| const std::vector<uint8_t> &initData, | ||
| const LimitedDurationLicense &ldlState) | ||
| const std::vector<uint8_t> &initData) | ||
| { | ||
| if (!reattachChannelIfRequired()) |
There was a problem hiding this comment.
MediaKeysIpc::generateRequest no longer accepts/serializes LimitedDurationLicense ldlState, but IMediaKeys::generateRequest still defines that parameter and the protobuf schema still contains ldl_state. Please make the API/proto/client+server implementations consistent (or keep forwarding ldl_state).
| firebolt::rialto::GenerateRequestRequest request; | ||
| request.set_media_keys_handle(m_mediaKeysHandle); | ||
| request.set_key_session_id(keySessionId); | ||
| request.set_init_data_type(protoInitDataType); | ||
| request.set_ldl_state(protoLimitedDurationLicense); | ||
|
|
There was a problem hiding this comment.
The protobuf request previously set ldl_state, but that field assignment has been removed. If the server still depends on LDL state (and the proto still defines it), this is a behavior change that should be either preserved or coordinated with corresponding server-side changes.
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to create the media keys module service factory, reason: %s", e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysModuleService", telemetryBuff); |
There was a problem hiding this comment.
Telemetry marker uses "Rialto Client" in server-side IPC service code. Consider using a server-specific marker name to avoid misattributing events.
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to create the media keys capabilities module service factory, reason: %s", e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysCapabilitiesModuleService", telemetryBuff); |
There was a problem hiding this comment.
Telemetry marker uses "Rialto Client" in server-side IPC service code. Consider using a server-specific marker name to avoid misattributing events.
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
1d4948c to
a334d54
Compare
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
a334d54 to
6e9705c
Compare
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
6e9705c to
18cfe18
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| find_package_handle_standard_args(TELEMETRY DEFAULT_MSG TELEMETRY_LIBRARY TELEMETRY_INCLUDE_DIR) | ||
|
|
||
| mark_as_advanced(TELEMETRY_INCLUDE_DIR TELEMETRY_LIBRARY) | ||
|
|
||
| if(TELEMETRY_FOUND) | ||
| set(TELEMETRY_LIBRARIES ${TELEMETRY_LIBRARY}) | ||
| set(TELEMETRY_INCLUDE_DIRS ${TELEMETRY_INCLUDE_DIR}) | ||
| endif() | ||
|
|
||
| if(TELEMETRY_FOUND AND NOT TARGET telemetry) |
There was a problem hiding this comment.
find_package(telemetry REQUIRED) expects the module to set telemetry_FOUND, but this module calls find_package_handle_standard_args(TELEMETRY ...) which sets TELEMETRY_FOUND instead. This likely makes find_package(telemetry) fail even when the library/header are found. Call find_package_handle_standard_args(telemetry ...) (matching the package name) and update the subsequent *_FOUND checks accordingly.
| find_package_handle_standard_args(TELEMETRY DEFAULT_MSG TELEMETRY_LIBRARY TELEMETRY_INCLUDE_DIR) | |
| mark_as_advanced(TELEMETRY_INCLUDE_DIR TELEMETRY_LIBRARY) | |
| if(TELEMETRY_FOUND) | |
| set(TELEMETRY_LIBRARIES ${TELEMETRY_LIBRARY}) | |
| set(TELEMETRY_INCLUDE_DIRS ${TELEMETRY_INCLUDE_DIR}) | |
| endif() | |
| if(TELEMETRY_FOUND AND NOT TARGET telemetry) | |
| find_package_handle_standard_args(telemetry DEFAULT_MSG TELEMETRY_LIBRARY TELEMETRY_INCLUDE_DIR) | |
| mark_as_advanced(TELEMETRY_INCLUDE_DIR TELEMETRY_LIBRARY) | |
| if(telemetry_FOUND) | |
| set(TELEMETRY_LIBRARIES ${TELEMETRY_LIBRARY}) | |
| set(TELEMETRY_INCLUDE_DIRS ${TELEMETRY_INCLUDE_DIR}) | |
| endif() | |
| if(telemetry_FOUND AND NOT TARGET telemetry) |
| catch (const std::exception &e) | ||
| { | ||
| RIALTO_SERVER_LOG_ERROR("Failed to create the media keys capabilities module service factory, reason: %s", | ||
| e.what()); | ||
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to create the media keys capabilities module service factory, reason: %s", e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysCapabilitiesModuleService", telemetryBuff); | ||
| } |
There was a problem hiding this comment.
snprintf is used here without including <cstdio>/<stdio.h> in this translation unit. Please add the standard header include to avoid build failures on stricter compilers.
| if (!m_controlIpc->getSharedMemory(shmFd, shmBufferLen)) | ||
| { | ||
| RIALTO_CLIENT_LOG_ERROR("Failed to get the shared memory"); | ||
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to get the shared memory"); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - ClientController", telemetryBuff); | ||
| return false; |
There was a problem hiding this comment.
Client-side telemetry events are emitted here, but there is no TELEMETRY_INIT(...) call anywhere under media/client (searching the repo shows only the server main.cpp initializes telemetry). If the T2 library requires initialization, these events may be dropped or cause undefined behavior. Please add a clear initialization point for the client library/process before emitting telemetry (and consider uninit on shutdown if required).
| MediaKeyErrorStatus MediaKeys::createKeySession(KeySessionType sessionType, std::weak_ptr<IMediaKeysClient> client, | ||
| int32_t &keySessionId) | ||
| bool isLDL, int32_t &keySessionId) | ||
| { | ||
| RIALTO_CLIENT_LOG_DEBUG("entry:"); | ||
|
|
||
| auto result{m_mediaKeysIpc->createKeySession(sessionType, client, keySessionId)}; | ||
| auto result{m_mediaKeysIpc->createKeySession(sessionType, client, isLDL, keySessionId)}; | ||
| if (isNetflixPlayready(m_keySystem) && MediaKeyErrorStatus::OK == result) |
There was a problem hiding this comment.
The method signature was changed to add bool isLDL, but the corresponding declaration in media/client/main/include/MediaKeys.h (and the public interface media/public/include/IMediaKeys.h) still declares createKeySession(..., int32_t &keySessionId) without this parameter. This will not compile and is an API break; update the interface/header(s) and all callers/tests consistently (or keep the old signature and pass the LDL info another way).
| #define TELEMETRY_INIT(component) \ | ||
| do { \ | ||
| t2_init((char*)component); \ | ||
| } while(0) | ||
|
|
||
| #define TELEMETRY_UNINIT() \ | ||
| do { \ | ||
| t2_uninit(); \ | ||
| } while(0) | ||
|
|
||
| #define TELEMETRY_EVENT_STRING(marker, value) \ | ||
| do { \ | ||
| t2_event_s((char*)marker, (char*)value); \ | ||
| } while(0) |
There was a problem hiding this comment.
These macros cast away const and may pass string literals as char* (e.g., t2_event_s((char*)marker, ...)). If the telemetry library ever writes to the buffer, this is undefined behavior and can crash. Prefer const char*-accepting inline wrappers (or const_cast<char*> with a clearly documented non-mutating contract) and avoid C-style casts.
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
18cfe18 to
0a5a826
Compare
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
0a5a826 to
7b69f75
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to create the media keys capabilities factory, reason: %s", e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysCapabilities", telemetryBuff); |
There was a problem hiding this comment.
snprintf is used here but this translation unit does not include <cstdio> (or <stdio.h>). This can fail to compile depending on transitive includes. Add the appropriate header in this file.
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to register client due to '%s'", | ||
| ipcController->ErrorText().c_str()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); |
There was a problem hiding this comment.
Same telemetry marker typo as above (ControllIpc -> ControlIpc).
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); | |
| TELEMETRY_EVENT_STRING("Rialto Client - ControlIpc", telemetryBuff); |
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); | ||
|
|
There was a problem hiding this comment.
Same telemetry marker typo as above (ControllIpc -> ControlIpc).
| RIALTO_CLIENT_LOG_ERROR("failed to ack due to '%s'", ipcController->ErrorText().c_str()); | ||
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to ack due to '%s'", ipcController->ErrorText().c_str()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); |
There was a problem hiding this comment.
Same telemetry marker typo as above (ControllIpc -> ControlIpc).
| TELEMETRY_EVENT_STRING("Rialto Client - ControllIpc", telemetryBuff); | |
| TELEMETRY_EVENT_STRING("Rialto Client - ControlIpc", telemetryBuff); |
| RIALTO_CLIENT_LOG_ERROR("failed to stop due to '%s'", ipcController->ErrorText().c_str()); | ||
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to stop due to '%s'", ipcController->ErrorText().c_str()); |
There was a problem hiding this comment.
The new telemetry message repeats the existing copy/paste error from the log line: this is haveData() but the message says "stop". This makes telemetry misleading and harder to triage. Update both the log and telemetry strings to reflect the correct operation (haveData).
| RIALTO_CLIENT_LOG_ERROR("failed to stop due to '%s'", ipcController->ErrorText().c_str()); | |
| char telemetryBuff[128] = {0}; | |
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to stop due to '%s'", ipcController->ErrorText().c_str()); | |
| RIALTO_CLIENT_LOG_ERROR("failed to haveData due to '%s'", ipcController->ErrorText().c_str()); | |
| char telemetryBuff[128] = {0}; | |
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to haveData due to '%s'", | |
| ipcController->ErrorText().c_str()); |
| RIALTO_CLIENT_LOG_ERROR("failed to get mute due to '%s'", ipcController->ErrorText().c_str()); | ||
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to get mute due to '%s'", |
There was a problem hiding this comment.
In getTextTrackIdentifier(), the error/telemetry message incorrectly says "get mute" (copy/paste from getMute). This will produce misleading telemetry. Update the message (and ideally the log line too) to mention getTextTrackIdentifier.
| RIALTO_CLIENT_LOG_ERROR("failed to get mute due to '%s'", ipcController->ErrorText().c_str()); | |
| char telemetryBuff[128] = {0}; | |
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to get mute due to '%s'", | |
| RIALTO_CLIENT_LOG_ERROR("failed to getTextTrackIdentifier due to '%s'", | |
| ipcController->ErrorText().c_str()); | |
| char telemetryBuff[128] = {0}; | |
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to getTextTrackIdentifier due to '%s'", |
| ExternalProject_Add( | ||
| telemetry-source | ||
| GIT_REPOSITORY https://github.com/rdkcentral/telemetry.git | ||
| GIT_TAG develop | ||
| PREFIX ${CMAKE_CURRENT_BINARY_DIR}/third-party | ||
| CONFIGURE_COMMAND ./configure --prefix=${CMAKE_CURRENT_BINARY_DIR}/third-party/telemetry-install | ||
| BUILD_COMMAND make | ||
| INSTALL_COMMAND make install |
There was a problem hiding this comment.
In the native-build path, the ExternalProject pulls GIT_TAG develop, which is a moving target and makes builds non-reproducible (and can break unexpectedly when upstream changes). Consider pinning to a specific release tag or commit SHA (and optionally exposing it as a cache variable) to make native builds deterministic.
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
7b69f75 to
4751459
Compare
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
4751459 to
4c7c46c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
media/client/main/CMakeLists.txt:90
- This target now links
telemetryas a raw library name. The repository’sFindtelemetry.cmakeinstead locatestelemetry_msgsenderand exposes it via${TELEMETRY_LIBRARIES}, so linkingtelemetrydirectly is likely inconsistent (and may fail unless-ltelemetryexists). Prefer linking via${TELEMETRY_LIBRARIES}/an imported target and gating it behind the telemetry enable option, or rely onRialtoEthanLog/RialtoLoggingto carry the dependency PUBLIC when enabled.
target_link_libraries(
RialtoClient
PRIVATE
RialtoPlayerCommon
RialtoClientIpcImpl
RialtoCommon
RialtoEthanLog
telemetry
Threads::Threads
)
media/client/common/include/RialtoClientLogging.h:29
- This header is written to be usable from both C and C++ (
extern "C"guarded by__cplusplus), but it now includes the C++ header<cstdio>, which will not compile when included from a C translation unit. If you needsnprintf/stdio declarations here, prefer including<stdio.h>(or only include<cstdio>under#ifdef __cplusplus).
#include "RialtoLogging.h"
#include <cstdio>
#ifdef __cplusplus
extern "C"
{
#endif
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| telemetry | ||
| Threads::Threads | ||
| ) |
There was a problem hiding this comment.
telemetry is linked here as a raw library name, but Findtelemetry.cmake locates telemetry_msgsender and exposes it via ${TELEMETRY_LIBRARIES}. This mismatch can cause link failures (and forces a dependency even when telemetry is intended to be stubbed). Prefer linking via ${TELEMETRY_LIBRARIES}/an imported target and gate it behind the same telemetry enable option used in logging/CMakeLists.txt.
| telemetry | |
| Threads::Threads | |
| ) | |
| Threads::Threads | |
| ) | |
| if (TELEMETRY_LIBRARIES) | |
| target_link_libraries ( | |
| RialtoServerIpc | |
| PRIVATE | |
| ${TELEMETRY_LIBRARIES} | |
| ) | |
| endif () |
| telemetry | ||
| ) |
There was a problem hiding this comment.
telemetry is linked here as a plain library name, but the project’s Findtelemetry.cmake finds telemetry_msgsender and provides ${TELEMETRY_LIBRARIES}. This mismatch can lead to link errors and makes telemetry a hard dependency even when a stub build is desired. Prefer ${TELEMETRY_LIBRARIES}/an imported target and guard it behind the same telemetry-enable flag used in logging/CMakeLists.txt.
| telemetry | |
| ) | |
| ) | |
| if (TELEMETRY_LIBRARIES) | |
| target_link_libraries ( | |
| RialtoClientIpcImpl | |
| PRIVATE | |
| ${TELEMETRY_LIBRARIES} | |
| ) | |
| endif () |
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to create the media keys capabilities factory, reason: %s", e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysCapabilities", telemetryBuff); | ||
| } |
There was a problem hiding this comment.
The telemetry marker string uses the prefix "Rialto Client - ..." even though this code is in the server component (media/server/main). This makes server-side telemetry harder to query/aggregate and is inconsistent with other server markers in this PR (e.g. "Rialto Server - ControlModuleService"). Consider renaming the marker prefix to "Rialto Server - ..." for server-side emission.
| RIALTO_SERVER_LOG_ERROR("ipc library provided incompatible controller object"); | ||
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "ipc library provided incompatible controller object"); | ||
| TELEMETRY_EVENT_STRING("Rialto Server - ControlModuleService", telemetryBuff); |
There was a problem hiding this comment.
This file now calls snprintf(...) but does not include <cstdio>/<stdio.h> in its includes. Add the appropriate header include to ensure snprintf is declared on all toolchains.
| RIALTO_SERVER_LOG_ERROR("Failed to initialize ApplicationManagementServer - Ipc server instance is NULL"); | ||
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to initialize ApplicationManagementServer - Ipc server instance is NULL"); | ||
| TELEMETRY_EVENT_STRING("Rialto Server - ApplicationManagementServer", telemetryBuff); |
There was a problem hiding this comment.
This file now calls snprintf(...) but does not include <cstdio>/<stdio.h> in its includes. Add the appropriate header include rather than relying on indirect includes.
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
acafb48 to
e5a32c6
Compare
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
e5a32c6 to
d794b21
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to create the media keys module service factory, reason: %s", e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysModuleService", telemetryBuff); |
There was a problem hiding this comment.
snprintf is used here but this file does not include <cstdio> (or <stdio.h>), which can cause a build failure in C++ due to snprintf being undeclared. Please include the appropriate header.
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to create the media keys module service factory, reason: %s", e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysModuleService", telemetryBuff); | ||
| } |
There was a problem hiding this comment.
This is server IPC code (media/server/...) but the telemetry marker is labeled "Rialto Client". Please update the marker prefix/name to reflect server-side emission to avoid confusing dashboards.
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to create the media keys capabilities, reason: %s", | ||
| e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysCapabilities", telemetryBuff); | ||
| } |
There was a problem hiding this comment.
Server-side telemetry marker is still labeled "Rialto Client" here. Please rename to a server-specific marker name/prefix (and keep it consistent with the other catch block in this file).
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to create the media keys module service, reason: %s", | ||
| e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysModuleService", telemetryBuff); | ||
| } |
There was a problem hiding this comment.
Server IPC code is emitting a telemetry marker labeled "Rialto Client" here as well. Please update the marker name/prefix consistently across both catch blocks in this file.
| RIALTO_SERVER_LOG_ERROR("Failed to create the media keys capabilities module service, reason: %s", e.what()); | ||
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to create the media keys capabilities module service, reason: %s", e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysCapabilitiesModuleService", telemetryBuff); | ||
| } |
There was a problem hiding this comment.
This second catch block also emits telemetry as "Rialto Client" from server code. Please update marker naming consistently across the file.
| # Telemetry support | ||
| if(CMAKE_TELEMETRY_2_0_REQUIRED) | ||
| message(STATUS "Telemetry 2.0 support enabled") | ||
| target_compile_definitions(RialtoLogging PUBLIC RIALTO_TELEMETRY_SUPPORT=1) | ||
| target_include_directories(RialtoLogging PUBLIC ${TELEMETRY_INCLUDE_DIRS}) | ||
| target_link_libraries(RialtoLogging PUBLIC ${TELEMETRY_LIBRARIES}) |
There was a problem hiding this comment.
Telemetry support is enabled based on CMAKE_TELEMETRY_2_0_REQUIRED, but this CMakeLists never calls find_package(telemetry ...) (or otherwise defines TELEMETRY_INCLUDE_DIRS / TELEMETRY_LIBRARIES). As written, turning telemetry on can break the build because <telemetry_busmessage_sender.h> won’t be found and the link libraries may be empty. Consider invoking the new Findtelemetry module when telemetry is enabled and failing early if the dependency is required but not found.
| #define TELEMETRY_INIT(component) \ | ||
| do \ | ||
| { \ | ||
| t2_init((char *)component); \ | ||
| } while (0) | ||
|
|
||
| #define TELEMETRY_UNINIT() \ | ||
| do \ | ||
| { \ | ||
| t2_uninit(); \ | ||
| } while (0) | ||
|
|
||
| #define TELEMETRY_EVENT_STRING(marker, value) \ | ||
| do \ | ||
| { \ | ||
| t2_event_s((char *)marker, (char *)value); \ | ||
| } while (0) | ||
|
|
There was a problem hiding this comment.
These macros cast away constness (e.g., (char *)marker) and can pass string literals as mutable char*, which is undefined behavior if the telemetry API writes to the buffer. Please keep const-correctness (e.g., change the wrapper to accept const char*/std::string, or create mutable copies only when the underlying API truly requires char*).
| if(NOT NATIVE_BUILD) | ||
| find_path(TELEMETRY_INCLUDE_DIR NAMES telemetry_busmessage_sender.h) | ||
| find_library(TELEMETRY_LIBRARY NAMES telemetry_msgsender) | ||
|
|
||
| include(FindPackageHandleStandardArgs) | ||
| find_package_handle_standard_args(TELEMETRY DEFAULT_MSG TELEMETRY_LIBRARY TELEMETRY_INCLUDE_DIR) | ||
|
|
||
| mark_as_advanced(TELEMETRY_INCLUDE_DIR TELEMETRY_LIBRARY) | ||
|
|
||
| if(TELEMETRY_FOUND) | ||
| set(TELEMETRY_LIBRARIES ${TELEMETRY_LIBRARY}) | ||
| set(TELEMETRY_INCLUDE_DIRS ${TELEMETRY_INCLUDE_DIR}) | ||
| else() | ||
| set(TELEMETRY_LIBRARIES "") | ||
| set(TELEMETRY_INCLUDE_DIRS "") | ||
| endif() |
There was a problem hiding this comment.
The module filename suggests it will be used via find_package(telemetry), but find_package_handle_standard_args is invoked with TELEMETRY, producing TELEMETRY_FOUND rather than telemetry_FOUND. This case mismatch can make find_package(telemetry) report "not found" even when the library/header are present. Align the package name/casing throughout (filename, find_package_handle_standard_args argument, and _FOUND variable checks).
| RIALTO_CLIENT_LOG_ERROR("Failed to play due to '%s'", ipcController->ErrorText().c_str()); | ||
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to play due to '%s'", ipcController->ErrorText().c_str()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - WebAudioPlayerIpc", telemetryBuff); |
There was a problem hiding this comment.
In this failure path, ipcController->ErrorText() is evaluated twice (once for logging and once for telemetry formatting). Consider storing it in a local std::string/const auto & once and reusing it to avoid duplicate work and potential inconsistencies (it also simplifies unit-test mocking expectations).
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
d794b21 to
5b2e201
Compare
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
5b2e201 to
7e7c015
Compare
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
7e7c015 to
f2e3c10
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
media/server/service/source/main.cpp:59
- TELEMETRY_INIT is called but TELEMETRY_UNINIT is never invoked on exit. If the telemetry library expects cleanup/flush, this can leak resources or drop the final events. Consider calling TELEMETRY_UNINIT() before returning (including failure paths), or wrap init/uninit in a small RAII helper.
TELEMETRY_INIT("rialto-server");
firebolt::rialto::server::IGstInitialiser::instance().initialise(&argc, &argv);
auto appSessionServer =
firebolt::rialto::server::IApplicationSessionServerFactory::getFactory()->createApplicationSessionServer();
if (!appSessionServer->init(argc, argv))
{
return EXIT_FAILURE;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #define TELEMETRY_INIT(component) \ | ||
| do \ | ||
| { \ | ||
| t2_init(reinterpret_cast<char *>(component)); \ | ||
| } while (0) |
There was a problem hiding this comment.
When telemetry support is enabled, these macros won’t compile with string literals / const char* arguments (e.g. TELEMETRY_INIT("rialto-server")), because reinterpret_cast<char*>(...) attempts to cast away const. Use a const-correct interface (preferably update wrappers to accept const char*), or use const_cast<char*> with clear documentation that the telemetry API does not mutate the input buffer.
| #define RIALTO_CLIENT_LOGGING_H_ | ||
|
|
||
| #include "RialtoLogging.h" | ||
| #include <cstdio> |
There was a problem hiding this comment.
RialtoClientLogging.h now includes , but this header doesn’t use stdio APIs directly. Pulling into a widely-included logging header increases coupling/compile time; prefer including only in the .cpp files that call snprintf.
| #include <cstdio> |
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
f2e3c10 to
c584e0c
Compare
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
c584e0c to
507f494
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
media/server/service/source/main.cpp:63
TELEMETRY_INIT()is called in main(), butTELEMETRY_UNINIT()is not called on the normal success path afterstartService()returns. At the same time, telemetry is uninitialised inPlaybackService::~PlaybackService(), which couples global telemetry lifetime to a service object and can lead to double-uninit (e.g.,init()failure path uninitialises and then destructors run). Manage telemetry init/uninit in one place (ideally main with an RAII guard) and remove the destructor-based uninit.
TELEMETRY_INIT("rialto-server");
firebolt::rialto::server::IGstInitialiser::instance().initialise(&argc, &argv);
auto appSessionServer =
firebolt::rialto::server::IApplicationSessionServerFactory::getFactory()->createApplicationSessionServer();
if (!appSessionServer->init(argc, argv))
{
TELEMETRY_UNINIT();
return EXIT_FAILURE;
}
appSessionServer->startService();
media/client/common/include/RialtoClientLogging.h:28
RialtoClientLogging.his written to be usable from C (it has#ifdef __cplusplus/extern "C"guards), but it now unconditionally includes the C++ header<cstdio>. If this header is ever included from a C translation unit it will fail to compile. Prefer including<stdio.h>here, or wrap<cstdio>in#ifdef __cplusplusand use<stdio.h>otherwise.
#include "RialtoLogging.h"
#include <cstdio>
#ifdef __cplusplus
extern "C"
{
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (ipcController->Failed()) | ||
| { | ||
| RIALTO_CLIENT_LOG_ERROR("Failed to play due to '%s'", ipcController->ErrorText().c_str()); | ||
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to play due to '%s'", ipcController->ErrorText().c_str()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - WebAudioPlayerIpc", telemetryBuff); | ||
| return false; |
There was a problem hiding this comment.
In this error path ipcController->ErrorText() is evaluated twice (once for logging and again for telemetry), which can be unnecessarily expensive and can produce inconsistent output if ErrorText() is not purely accessor-like. Cache the error text in a local std::string (or const auto &) and reuse it for both the log and telemetry message.
| PlaybackService::~PlaybackService() | ||
| { | ||
| RIALTO_SERVER_LOG_DEBUG("PlaybackService is destructed"); | ||
| TELEMETRY_UNINIT(); |
There was a problem hiding this comment.
Calling TELEMETRY_UNINIT() from PlaybackService's destructor makes telemetry shutdown dependent on this object's lifetime and can result in double-uninit (main.cpp already uninitialises on some paths). Telemetry init/uninit should be owned by the process entry/exit path (or a dedicated guard), not a service component destructor.
| TELEMETRY_UNINIT(); |
| #define TELEMETRY_INIT(component) \ | ||
| do \ | ||
| { \ | ||
| t2_init(const_cast<char *>(component)); \ | ||
| } while (0) | ||
|
|
||
| #define TELEMETRY_UNINIT() \ | ||
| do \ | ||
| { \ | ||
| t2_uninit(); \ | ||
| } while (0) | ||
|
|
||
| #define TELEMETRY_EVENT_STRING(marker, value) \ | ||
| do \ | ||
| { \ | ||
| t2_event_s(const_cast<char *>(marker), const_cast<char *>(value)); \ | ||
| } while (0) | ||
|
|
||
| #define TELEMETRY_EVENT_FLOAT(marker, value) \ | ||
| do \ | ||
| { \ | ||
| t2_event_f(const_cast<char *>(marker), static_cast<double>(value)); \ | ||
| } while (0) | ||
|
|
||
| #define TELEMETRY_EVENT_INT(marker, value) \ | ||
| do \ | ||
| { \ | ||
| t2_event_d(const_cast<char *>(marker), static_cast<int>(value)); \ | ||
| } while (0) |
There was a problem hiding this comment.
RialtoTelemetry.h is structured to be includable from both C and C++ (#ifdef __cplusplus / extern "C"), but the macro implementations use C++-only casts (const_cast, static_cast). If any C translation unit includes RialtoLogging.h (which now includes this header), it will fail to compile. Make the macros C-compatible (use C-style casts in the macro body) or wrap the C++-only implementations in #ifdef __cplusplus and provide C equivalents.
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
507f494 to
9bbf1a5
Compare
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
9bbf1a5 to
a7ba470
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Rialto telemetry initialized"); | ||
| TELEMETRY_EVENT_STRING("Rialto - main", telemetryBuff); |
There was a problem hiding this comment.
This file uses snprintf(...) for the telemetry message but does not include <cstdio>/<stdio.h>. This can fail to compile depending on transitive includes; please include the appropriate header in this TU.
| } | ||
|
|
||
| TELEMETRY_INIT("rialto-server"); | ||
| RIALTO_SERVER_LOG_MIL("Rialto telemetry intialized"); |
There was a problem hiding this comment.
Typo in log message: "intialized" -> "initialized".
| RIALTO_SERVER_LOG_MIL("Rialto telemetry intialized"); | |
| RIALTO_SERVER_LOG_MIL("Rialto telemetry initialized"); |
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Could not initalise the shared memory"); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - ClientController", telemetryBuff); |
There was a problem hiding this comment.
Typo in telemetry message: "initalise" -> "initialise" (and consider aligning the log + telemetry strings).
|
|
||
| if (!appSessionServer->init(argc, argv)) | ||
| { | ||
| TELEMETRY_UNINIT(); |
There was a problem hiding this comment.
TELEMETRY_UNINIT() is called here on appSessionServer->init() failure, but ApplicationSessionServer owns a PlaybackService member whose destructor also calls TELEMETRY_UNINIT() (see media/server/service/source/PlaybackService.cpp:49). When init() returns false, appSessionServer will be destroyed on this return path, causing a double-uninit.
| TELEMETRY_UNINIT(); |
| PlaybackService::~PlaybackService() | ||
| { | ||
| RIALTO_SERVER_LOG_DEBUG("PlaybackService is destructed"); | ||
| TELEMETRY_UNINIT(); |
There was a problem hiding this comment.
PlaybackService::~PlaybackService() unconditionally calls TELEMETRY_UNINIT(), but telemetry is initialized in media/server/service/source/main.cpp. This makes telemetry lifetime dependent on PlaybackService lifetime and also causes double-uninit on the main.cpp init-failure path. Consider moving telemetry uninitialization to the same owner that initializes it (e.g., RAII guard in main or ApplicationSessionServer), and remove it from this destructor.
| TELEMETRY_UNINIT(); |
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to create the media keys capabilities module service, reason: %s", e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysCapabilitiesModuleService", telemetryBuff); |
There was a problem hiding this comment.
Telemetry marker/message uses "Rialto Client - MediaKeysCapabilitiesModuleService" in a server-side module service. Please use a server-specific marker prefix to avoid mixing client/server telemetry streams.
|
Coverage statistics of your commit: |
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
a7ba470 to
b187cc1
Compare
Summary: Integrating telemetry T2 markers into Rialto Type: Fix Test Plan: UT/CT, Fullstack Jira: RDKEMW-16516
Koky2701
force-pushed
the
feature/RDKEMW-16516
branch
from
b187cc1 to
836cc25
Compare
|
logging/source/RialtoTelemetry.cpp:41:0: style: The function 'TELEMETRY_EVENT_FLOAT' is never used. [unusedFunction] |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
logging/CMakeLists.txt:31
RialtoTelemetry.cppis added in this PR but it is not included inRialtoLogging_SOURCES, so whenRIALTO_TELEMETRY_SUPPORTis enabled the telemetry symbols will not be compiled intoRialtoLoggingand linking will fail. Add the new source file toRialtoLogging_SOURCES(and toRialtoEthanLogsources if that library is built separately).
set(RialtoLogging_SOURCES
source/EnvVariableParser.cpp
source/EnvVariableParser.h
source/LogFileHandle.cpp
source/LogFileHandle.h
source/RialtoLogging.cpp
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| snprintf(telemetryBuff, sizeof(telemetryBuff), | ||
| "Failed to create the media keys capabilities module service, reason: %s", e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysCapabilitiesModuleService", telemetryBuff); | ||
| } |
There was a problem hiding this comment.
Telemetry marker string uses the "Rialto Client - ..." prefix, but this is server-side code (media/server/ipc/...) and will be attributed incorrectly. Please adjust the marker/prefix to reflect server context.
| char telemetryBuff[128] = {0}; | ||
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Client app connected"); | ||
| TELEMETRY_EVENT_STRING("Rialto - main", telemetryBuff); |
There was a problem hiding this comment.
snprintf is used here but this translation unit does not include <cstdio>/<stdio.h>, which can cause compile errors depending on toolchain/transitive includes. Add the appropriate header.
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to create the media keys capabilities, reason: %s", | ||
| e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysCapabilities", telemetryBuff); | ||
| } |
There was a problem hiding this comment.
The telemetry marker string here says "Rialto Client - ...", but this code is under media/server/... and uses RIALTO_SERVER_LOG_*. This will misclassify server-side telemetry as client telemetry; please use a server-appropriate marker/prefix.
|
|
||
| void TELEMETRY_EVENT_STRING(const char* marker, const char* value) | ||
| { | ||
| RIALTO_SERVER_LOG_MIL("Telemetry string called"); |
There was a problem hiding this comment.
TELEMETRY_EVENT_STRING logs at milestone level every time a telemetry marker is sent. This is likely to be extremely noisy/hot-path and can negatively impact performance and log volume. Consider removing these logs entirely, or downgrading them to debug and/or gating behind a separate compile-time/runtime flag.
| RIALTO_SERVER_LOG_MIL("Telemetry string called"); |
| #include "RialtoLogging.h" | ||
|
|
||
| #ifdef RIALTO_TELEMETRY_SUPPORT | ||
|
|
||
| void TELEMETRY_INIT(const char* component) | ||
| { | ||
| RIALTO_SERVER_LOG_MIL("Telemetry initialized!"); | ||
| t2_init(const_cast<char*>(component)); | ||
| } |
There was a problem hiding this comment.
This file uses RIALTO_SERVER_LOG_MIL, but it only includes RialtoLogging.h (which does not define the RIALTO_SERVER_LOG_* macros). This will not compile; either include RialtoServerLogging.h or log via the generic RIALTO_LOG_MIL(RIALTO_COMPONENT_..., ...) API so the telemetry wrapper can be used from both client and server builds.
| #include "IApplicationSessionServer.h" | ||
| #include "IGstInitialiser.h" | ||
| #include "RialtoServerLogging.h" | ||
| #include "RialtoTelemetry.h" | ||
|
|
There was a problem hiding this comment.
snprintf is used in this file, but there is no direct include of <cstdio>/<stdio.h>. Relying on transitive includes is brittle and can break builds with different standard library/flags; add the appropriate header here.
| snprintf(telemetryBuff, sizeof(telemetryBuff), "Failed to create the media keys module service, reason: %s", | ||
| e.what()); | ||
| TELEMETRY_EVENT_STRING("Rialto Client - MediaKeysModuleService", telemetryBuff); |
There was a problem hiding this comment.
The telemetry marker string uses the "Rialto Client - ..." prefix, but this is server-side code (media/server/ipc/...) and will be attributed incorrectly in telemetry. Please adjust the marker/prefix to indicate server context.
Summary: Integrating telemetry T2 markers into Rialto
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-16516