Conversation
Summary: 'report_decode_errors' property missing from RialtoMSEVideoSink
Type: Feature
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-12692
…linemodule.proto
Summary: 'report_decode_errors' property missing from RialtoMSEVideoSink
Type: Feature
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-12692
Summary: 'report_decode_errors' property missing from RialtoMSEVideoSink
Type: Feature
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-12692
Summary: 'report_decode_errors' property missing from RialtoMSEVideoSink
Type: Feature
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-12692
|
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. |
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for the 'report_decode_errors' property for RialtoMSEVideoSink by implementing a complete feature path from client to GStreamer player. The feature allows enabling or disabling decode error reporting for video sources.
Changes:
- Added
setReportDecodeErrorsAPI across all layers (client IPC, server service, and GStreamer player) - Implemented SetReportDecodeErrors task in the GStreamer player task system
- Added protocol buffer definitions for setReportDecodeErrors RPC and GetQueuedFrames RPC (the latter appears unimplemented)
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/mediapipelinemodule.proto | Added ReportDecodeErrors and GetQueuedFrames message definitions and RPC methods |
| media/public/include/IMediaPipeline.h | Added setReportDecodeErrors interface method |
| media/client/main/include/MediaPipeline.h | Added setReportDecodeErrors method declaration |
| media/client/main/include/MediaPipelineProxy.h | Added setReportDecodeErrors proxy method |
| media/client/main/source/MediaPipeline.cpp | Implemented client-side setReportDecodeErrors |
| media/client/ipc/interface/IMediaPipelineIpc.h | Added IPC interface for setReportDecodeErrors |
| media/client/ipc/include/MediaPipelineIpc.h | Added IPC implementation method declaration |
| media/client/ipc/source/MediaPipelineIpc.cpp | Implemented IPC client for setReportDecodeErrors |
| media/server/ipc/include/MediaPipelineModuleService.h | Added server IPC method declaration |
| media/server/ipc/source/MediaPipelineModuleService.cpp | Implemented server IPC handler |
| media/server/service/include/IMediaPipelineService.h | Added service interface method |
| media/server/service/source/MediaPipelineService.h | Added service method declaration |
| media/server/service/source/MediaPipelineService.cpp | Implemented service-layer routing |
| media/server/main/include/MediaPipelineServerInternal.h | Added server internal method declarations |
| media/server/main/source/MediaPipelineServerInternal.cpp | Implemented main thread coordination |
| media/server/gstplayer/interface/IGstGenericPlayer.h | Added GStreamer player interface method |
| media/server/gstplayer/include/IGstGenericPlayerPrivate.h | Added private interface for worker thread |
| media/server/gstplayer/include/GstGenericPlayer.h | Added method declarations |
| media/server/gstplayer/source/GstGenericPlayer.cpp | Implemented GStreamer player methods |
| media/server/gstplayer/include/tasks/IGenericPlayerTaskFactory.h | Added task factory interface for SetReportDecodeErrors |
| media/server/gstplayer/include/tasks/generic/GenericPlayerTaskFactory.h | Added task factory method declaration |
| media/server/gstplayer/source/tasks/generic/GenericPlayerTaskFactory.cpp | Implemented task factory method |
| media/server/gstplayer/include/tasks/generic/SetReportDecodeErrors.h | New task class header |
| media/server/gstplayer/source/tasks/generic/SetReportDecodeErrors.cpp | New task class implementation |
| media/server/gstplayer/CMakeLists.txt | Added SetReportDecodeErrors.cpp to build |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * This method is asynchronous, it will set the "Report Decode Errors" property | ||
| * | ||
| * @param[in] sourceId : The source id. Value should be set to the MediaSource.id returned after attachSource() | ||
| * @param[in] immediateOutput : Set Report Decode Errors mode on the sink |
There was a problem hiding this comment.
The parameter name in the documentation is incorrect. It should be "reportDecodeErrors" instead of "immediateOutput" to match the actual parameter name.
| * @param[in] immediateOutput : Set Report Decode Errors mode on the sink | |
| * @param[in] reportDecodeErrors : Set Report Decode Errors mode on the sink |
| * This method is asynchronous, it will set the "Report Decode Errors" property | ||
| * | ||
| * @param[in] sourceId : The source id. Value should be set to the MediaSource.id returned after attachSource() | ||
| * @param[in] immediateOutput : Set Report Decode Errors mode on the sink |
There was a problem hiding this comment.
The parameter name in the documentation is incorrect. It should be "reportDecodeErrors" instead of "immediateOutput" to match the actual parameter name.
| * @param[in] immediateOutput : Set Report Decode Errors mode on the sink | |
| * @param[in] reportDecodeErrors : Set Report Decode Errors mode on the sink |
| * @brief Creates a setReportDecodeErrors task. | ||
| * | ||
| * @param[in] context : The GstPlayer context | ||
| * @param[in] player : The GstPlayer instance | ||
| * @param[in] type : The media source type | ||
| * @param[in] immediateOutput : the value to set for report decode error | ||
| * | ||
| * @retval the new ProcessAudioGap task instance. |
There was a problem hiding this comment.
The return value documentation is incorrect. It should describe what task instance is being returned (SetReportDecodeErrors task) rather than stating "the new ProcessAudioGap task instance". This appears to be copy-pasted from another method without being updated.
| * @brief Creates a setReportDecodeErrors task. | |
| * | |
| * @param[in] context : The GstPlayer context | |
| * @param[in] player : The GstPlayer instance | |
| * @param[in] type : The media source type | |
| * @param[in] immediateOutput : the value to set for report decode error | |
| * | |
| * @retval the new ProcessAudioGap task instance. | |
| * @brief Creates a SetReportDecodeErrors task. | |
| * | |
| * @param[in] context : The GstPlayer context | |
| * @param[in] player : The GstPlayer instance | |
| * @param[in] type : The media source type | |
| * @param[in] reportDecodeErrors: the value to set for report decode errors | |
| * | |
| * @retval the new SetReportDecodeErrors task instance. |
| #ifndef FIREBOLT_RIALTO_SERVER_TASKS_GENERIC_SET_REPORT_DECODE_ERROR_H_ | ||
| #define FIREBOLT_RIALTO_SERVER_TASKS_GENERIC_SET_REPORT_DECODE_ERROR_H_ |
There was a problem hiding this comment.
The header guard name is inconsistent. It uses "SET_REPORT_DECODE_ERROR_H_" (singular) but should be "SET_REPORT_DECODE_ERRORS_H_" (plural) to match the class name SetReportDecodeErrors and follow the naming convention seen in other files.
| }; | ||
| } // namespace firebolt::rialto::server::tasks::generic | ||
|
|
||
| #endif // FIREBOLT_RIALTO_SERVER_TASKS_GENERIC_SET_REPORT_DECODE_ERROR_H_ |
There was a problem hiding this comment.
The header guard name is inconsistent. It uses "SET_REPORT_DECODE_ERROR_H_" (singular) but should be "SET_REPORT_DECODE_ERRORS_H_" (plural) to match the class name SetReportDecodeErrors and follow the naming convention seen in other files.
| /** | ||
| * @fn void getQueuedFrames(int session_id, int source_id, uint32_t &queued_frames) | ||
| * @brief Gets the "Queued Frames" property for this source. | ||
| * | ||
| * @param[in] session_id The id of the A/V session. | ||
| * @param[in] source_id The id of the media source. | ||
| * @param[out] queued_frames Get queued frames on the decoder | ||
| * | ||
| */ | ||
| message GetQueuedFramesRequest { | ||
| optional int32 session_id = 1 [default = -1]; | ||
| optional int32 source_id = 2 [default = -1]; | ||
| } | ||
| message GetQueuedFramesResponse { | ||
| optional uint32 queued_frames = 1 [default = -1]; | ||
| } |
There was a problem hiding this comment.
The GetQueuedFramesRequest and GetQueuedFramesResponse message definitions are added but there is no corresponding implementation in the server IPC layer, service layer, or client. This appears to be an incomplete feature addition. Either the implementation should be provided or these proto definitions should be removed.
| if (m_type != MediaSourceType::VIDEO) | ||
| { | ||
| RIALTO_SERVER_LOG_ERROR("SetReportDecodeErrors not currently supported for non-video"); | ||
| } | ||
|
|
||
| if (m_context.pipeline) | ||
| { | ||
| m_player.setReportDecodeErrors(m_reportDecodeErrors); | ||
| } |
There was a problem hiding this comment.
The function logs an error when the media source type is not VIDEO but continues execution and calls m_player.setReportDecodeErrors anyway. This appears to be a logic error. If the feature is not supported for non-video sources, the function should return early after logging the error, similar to how validation is typically handled.
| * @param[in] context : The GstPlayer context | ||
| * @param[in] player : The GstPlayer instance | ||
| * @param[in] type : The media source type | ||
| * @param[in] immediateOutput : the value to set for report decode error |
There was a problem hiding this comment.
The parameter name in the documentation is incorrect. It should be "reportDecodeErrors" instead of "immediateOutput" to match the actual parameter name.
| * @param[in] context : The GstPlayer context | |
| * @param[in] player : The GstPlayer instance | |
| * @param[in] type : The media source type | |
| * @param[in] immediateOutput : the value to set for report decode error | |
| * @param[in] context : The GstPlayer context | |
| * @param[in] player : The GstPlayer instance | |
| * @param[in] type : The media source type | |
| * @param[in] reportDecodeErrors: the value to set for report decode errors |
| * This method is asynchronous | ||
| * | ||
| * @param[in] sourceId : The source id. Value should be set to the MediaSource.id returned after attachSource() | ||
| * @param[in] reportDecodeErrors : The desired immediate output mode on the sink |
There was a problem hiding this comment.
The parameter description is incorrect. It states "The desired immediate output mode on the sink" but should state something like "The desired report decode errors mode on the sink" or similar to accurately describe the reportDecodeErrors parameter.
| * @param[in] reportDecodeErrors : The desired immediate output mode on the sink | |
| * @param[in] reportDecodeErrors : The desired report decode errors mode on the sink |
| } | ||
| else | ||
| { | ||
| RIALTO_SERVER_LOG_DEBUG("Pending an report_decode_errors, decoder is NULL"); |
There was a problem hiding this comment.
Grammar error in the log message. It should be "Pending a report_decode_errors" instead of "Pending an report_decode_errors" since "report" starts with a consonant sound.
| RIALTO_SERVER_LOG_DEBUG("Pending an report_decode_errors, decoder is NULL"); | |
| RIALTO_SERVER_LOG_DEBUG("Pending a report_decode_errors, decoder is NULL"); |
Koky2701
force-pushed
the
feature/RDKEMW-12692
branch
from
418abb7 to
50520a8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 25 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool MediaPipelineService::setReportDecodeErrors(int sessionId, int32_t sourceId, bool reportDecodeErrors) | ||
| { | ||
| RIALTO_SERVER_LOG_INFO("MediaPipelineService requested to setReportDecodeErrors, session id: %d", sessionId); | ||
|
|
||
| std::lock_guard<std::mutex> lock{m_mediaPipelineMutex}; | ||
| auto mediaPipelineIter = m_mediaPipelines.find(sessionId); | ||
| if (mediaPipelineIter == m_mediaPipelines.end()) | ||
| { | ||
| RIALTO_SERVER_LOG_ERROR("Session with id: %d does not exists", sessionId); | ||
| return false; | ||
| } | ||
| return mediaPipelineIter->second->setReportDecodeErrors(sourceId, reportDecodeErrors); | ||
| } |
There was a problem hiding this comment.
The new setReportDecodeErrors method is missing unit test coverage. Tests should be added similar to the existing setImmediateOutput tests to verify both success and failure cases.
| bool MediaPipelineService::getQueuedFrames(int sessionId, int32_t sourceId, uint32_t &queuedFrames) | ||
| { | ||
| RIALTO_SERVER_LOG_INFO("MediaPipelineService requested to getQueuedFrames, session id: %d", sessionId); | ||
|
|
||
| std::lock_guard<std::mutex> lock{m_mediaPipelineMutex}; | ||
| auto mediaPipelineIter = m_mediaPipelines.find(sessionId); | ||
| if (mediaPipelineIter == m_mediaPipelines.end()) | ||
| { | ||
| RIALTO_SERVER_LOG_ERROR("Session with id: %d does not exists", sessionId); | ||
| return false; | ||
| } | ||
| return mediaPipelineIter->second->getQueuedFrames(sourceId, queuedFrames); | ||
| } |
There was a problem hiding this comment.
The new getQueuedFrames method is missing unit test coverage. Tests should be added to verify both success and failure cases.
| bool MediaPipelineServerInternal::setReportDecodeErrors(int32_t sourceId, bool reportDecodeErrors) | ||
| { | ||
| RIALTO_SERVER_LOG_DEBUG("entry:"); | ||
|
|
||
| bool result; | ||
| auto task = [&]() { result = setReportDecodeErrorsInternal(sourceId, reportDecodeErrors); }; | ||
|
|
||
| m_mainThread->enqueueTaskAndWait(m_mainThreadClientId, task); | ||
| return result; | ||
| } | ||
|
|
||
| bool MediaPipelineServerInternal::setReportDecodeErrorsInternal(int32_t sourceId, bool reportDecodeErrors) | ||
| { | ||
| if (!m_gstPlayer) | ||
| { | ||
| RIALTO_SERVER_LOG_ERROR("Failed - Gstreamer player has not been loaded"); | ||
| return false; | ||
| } | ||
| auto sourceIter = std::find_if(m_attachedSources.begin(), m_attachedSources.end(), | ||
| [sourceId](const auto &src) { return src.second == sourceId; }); | ||
| if (sourceIter == m_attachedSources.end()) | ||
| { | ||
| RIALTO_SERVER_LOG_ERROR("Failed - Source not found"); | ||
| return false; | ||
| } | ||
|
|
||
| return m_gstPlayer->setReportDecodeErrors(sourceIter->first, reportDecodeErrors); | ||
| } |
There was a problem hiding this comment.
The new setReportDecodeErrors and setReportDecodeErrorsInternal methods are missing unit test coverage. Tests should be added to verify the threading model, source validation, and error handling.
| * | ||
| * This method is sychronous, it gets the queued frames property | ||
| * | ||
| * @param[in] sourceId : The source id. Value should be get to the MediaSource.id returned after attachSource() |
There was a problem hiding this comment.
Spelling error in parameter description: "get" should be "set" (copy-paste error from the getter above).
| * @param[in] sourceId : The source id. Value should be get to the MediaSource.id returned after attachSource() | |
| * @param[in] sourceId : The source id. Value should be set to the MediaSource.id returned after attachSource() |
| // check the result | ||
| if (ipcController->Failed()) | ||
| { | ||
| RIALTO_CLIENT_LOG_ERROR("failed to get immediate-output due to '%s'", ipcController->ErrorText().c_str()); |
There was a problem hiding this comment.
Error message is incorrect: This function gets queued frames, not immediate-output. The error message should be "failed to get queued frames due to '%s'".
| RIALTO_CLIENT_LOG_ERROR("failed to get immediate-output due to '%s'", ipcController->ErrorText().c_str()); | |
| RIALTO_CLIENT_LOG_ERROR("failed to get queued frames due to '%s'", ipcController->ErrorText().c_str()); |
| bool MediaPipelineServerInternal::getQueuedFrames(int32_t sourceId, uint32_t &queuedFrames) | ||
| { | ||
| RIALTO_SERVER_LOG_DEBUG("entry:"); | ||
|
|
||
| bool result; | ||
| auto task = [&]() { result = getQueuedFramesInternal(sourceId, queuedFrames); }; | ||
|
|
||
| m_mainThread->enqueueTaskAndWait(m_mainThreadClientId, task); | ||
| return result; | ||
| } | ||
|
|
||
| bool MediaPipelineServerInternal::getQueuedFramesInternal(int32_t sourceId, uint32_t &queuedFrames) | ||
| { | ||
| if (!m_gstPlayer) | ||
| { | ||
| RIALTO_SERVER_LOG_ERROR("Failed - Gstreamer player has not been loaded"); | ||
| return false; | ||
| } | ||
| auto sourceIter = std::find_if(m_attachedSources.begin(), m_attachedSources.end(), | ||
| [sourceId](const auto &src) { return src.second == sourceId; }); | ||
| if (sourceIter == m_attachedSources.end()) | ||
| { | ||
| RIALTO_SERVER_LOG_ERROR("Failed - Source not found"); | ||
| return false; | ||
| } | ||
| return m_gstPlayer->getQueuedFrames(sourceIter->first, queuedFrames); | ||
| } |
There was a problem hiding this comment.
The new getQueuedFrames and getQueuedFramesInternal methods are missing unit test coverage. Tests should be added to verify the threading model, source validation, and error handling.
| bool MediaPipeline::setReportDecodeErrors(int32_t sourceId, bool reportDecodeErrors) | ||
| { | ||
| return m_mediaPipelineIpc->setReportDecodeErrors(sourceId, reportDecodeErrors); | ||
| } | ||
|
|
||
| bool MediaPipeline::getQueuedFrames(int32_t sourceId, uint32_t &queuedFrames) | ||
| { | ||
| return m_mediaPipelineIpc->getQueuedFrames(sourceId, queuedFrames); | ||
| } |
There was a problem hiding this comment.
The new setReportDecodeErrors and getQueuedFrames methods are missing unit test coverage. Tests should be added to verify the forwarding to the IPC layer.
| /** | ||
| * @brief Sets the "Report Decode Error" property for this source. | ||
| * | ||
| * @param[in] mediaSourceType : The media source type | ||
| * @param[in] reportDecodeErrors : Set report decode error | ||
| * | ||
| * @retval true on success. | ||
| */ | ||
| virtual bool setReportDecodeErrors(const MediaSourceType &mediaSourceType, bool reportDecodeErrors) = 0; | ||
|
|
||
| /** | ||
| * @brief Gets the queued frames for this source. | ||
| * | ||
| * @param[in] mediaSourceType : The media source type | ||
| * @param[out] queuedFrames : Get queued frames mode on the decoder | ||
| * | ||
| * @retval true on success. | ||
| */ | ||
| virtual bool getQueuedFrames(const MediaSourceType &mediaSourceType, uint32_t &queuedFrames) = 0; | ||
|
|
There was a problem hiding this comment.
The GstGenericPlayerMock is missing MOCK_METHOD declarations for the new setReportDecodeErrors and getQueuedFrames methods. These need to be added to maintain consistency with the interface and enable proper unit testing.
| bool setReportDecodeErrors(int32_t sourceId, bool reportDecodeErrors) override; | ||
|
|
||
| bool getQueuedFrames(int32_t sourceId, uint32_t &queuedFrames) override; | ||
|
|
There was a problem hiding this comment.
The MediaPipelineServerInternalMock is missing MOCK_METHOD declarations for the new setReportDecodeErrors and getQueuedFrames methods. These need to be added after the setImmediateOutput/getImmediateOutput methods to maintain consistency with the interface and enable proper unit testing.
| bool GstGenericPlayer::setReportDecodeErrors(bool reportDecodeErrors) | ||
| { | ||
| bool result{false}; | ||
| GstElement *decoder = getDecoder(MediaSourceType::VIDEO); |
There was a problem hiding this comment.
The implementation hardcodes MediaSourceType::VIDEO, but the public API accepts a mediaSourceType parameter. This creates an inconsistency where the caller can specify any source type, but it will always be applied to the VIDEO decoder. Either the implementation should respect the mediaSourceType parameter, or the API should not accept it at all.
| bool GstGenericPlayer::setReportDecodeErrors(bool reportDecodeErrors) | |
| { | |
| bool result{false}; | |
| GstElement *decoder = getDecoder(MediaSourceType::VIDEO); | |
| bool GstGenericPlayer::setReportDecodeErrors(MediaSourceType mediaSourceType, bool reportDecodeErrors) | |
| { | |
| bool result{false}; | |
| GstElement *decoder = getDecoder(mediaSourceType); |
Summary: 'queued_buffers' property missing from RialtoMSEVideoSink
Type: Feature
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-12692
Koky2701
force-pushed
the
feature/RDKEMW-12692
branch
from
50520a8 to
d704fc2
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
tests/componenttests/server/tests/mediaPipeline/PipelinePropertyTest.cpp:350:0: style: The function 'setReportDecodeErrorsFailure' is never used. [unusedFunction] |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 62 out of 62 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| struct SetReportDecodeErrors | ||
| { | ||
| using RequestType = ::firebolt::rialto::SetReportDecodeErrorsRequest; | ||
| using ResponseType = ::firebolt::rialto::SetReportDecodeErrorsResponse; | ||
| using Stub = ::firebolt::rialto::MediaPipelineModule_Stub; | ||
| static constexpr auto m_kFunction{&Stub::setReportDecodeErrors}; | ||
| }; | ||
|
|
||
| struct GetQueuedFrames | ||
| { | ||
| using RequestType = ::firebolt::rialto::GetQueuedFramesRequest; | ||
| using ResponseType = ::firebolt::rialto::GetQueuedFramesResponse; | ||
| using Stub = ::firebolt::rialto::MediaPipelineModule_Stub; | ||
| static constexpr auto m_kFunction{&Stub::getQueuedFrames}; | ||
| }; |
There was a problem hiding this comment.
ActionTraits::SetReportDecodeErrors uses SetReportDecodeErrorsRequest/Response, but the proto currently defines ReportDecodeErrorsRequest/Response. Align these types (either by renaming the proto messages or updating the trait).
| constexpr bool kReportDecodeErrorsVal{false}; | ||
| constexpr bool kQueuedFramesVal{123}; |
There was a problem hiding this comment.
kQueuedFramesVal is declared as bool but used as a queued-frame count (set to 123 and written into a uint32_t &). This truncates the test value and can mask issues; it should be a uint32_t constant.
| { | ||
| firebolt::rialto::SetReportDecodeErrorsRequest request; | ||
| firebolt::rialto::SetReportDecodeErrorsResponse response; | ||
|
|
||
| request.set_session_id(kHardcodedSessionId); | ||
| request.set_immediate_output(kReportDecodeErrorsVal); | ||
|
|
||
| m_service->setReportDecodeErrors(m_controllerMock.get(), &request, &response, m_closureMock.get()); |
There was a problem hiding this comment.
These helpers use SetReportDecodeErrorsRequest/Response and set an immediate_output field, but the proto defines ReportDecodeErrorsRequest with a report_decode_errors field. As written, this won't compile and also won't exercise the correct request field; update the request/response types and set report_decode_errors (and source_id if the other tests do).
| uint32_t queuedFrames; | ||
| mainThreadWillEnqueueTaskAndWait(); | ||
| EXPECT_CALL(*m_gstPlayerMock, getQueuedFrames(_, _)).WillOnce(DoAll(SetArgReferee<1>(123), Return(true))); | ||
| EXPECT_TRUE(m_mediaPipeline->getQueuedFrames(videoSourceId, queuedFrames)); | ||
| EXPECT_EQ(queuedFrames, true); | ||
|
|
||
| mainThreadWillEnqueueTaskAndWait(); | ||
| EXPECT_CALL(*m_gstPlayerMock, getQueuedFrames(_, _)).WillOnce(DoAll(SetArgReferee<1>(123), Return(true))); | ||
| EXPECT_TRUE(m_mediaPipeline->getQueuedFrames(videoSourceId, queuedFrames)); | ||
| EXPECT_EQ(queuedFrames, false); | ||
| } |
There was a problem hiding this comment.
The test sets queuedFrames to 123 via the mock, but then compares it to true/false. This will fail (123 != true) and doesn't validate the correct behavior. Compare against the expected numeric values (and set different values for the two calls if you want to test both cases).
| message ReportDecodeErrorsRequest { | ||
| optional int32 session_id = 1 [default = -1]; | ||
| optional int32 source_id = 2 [default = -1]; | ||
| optional bool report_decode_errors = 3 [default = false]; | ||
| } | ||
|
|
||
| message ReportDecodeErrorsResponse { |
There was a problem hiding this comment.
The newly added proto messages are named ReportDecodeErrorsRequest/Response, but the rpc is setReportDecodeErrors and the rest of this proto uses the SetXxxRequest/Response naming convention (e.g., SetImmediateOutputRequest). This mismatch is already reflected in the tests/helpers referencing SetReportDecodeErrorsRequest, causing compile failures; rename the messages (or update all call sites) so the API is consistent end-to-end.
| message ReportDecodeErrorsRequest { | |
| optional int32 session_id = 1 [default = -1]; | |
| optional int32 source_id = 2 [default = -1]; | |
| optional bool report_decode_errors = 3 [default = false]; | |
| } | |
| message ReportDecodeErrorsResponse { | |
| message SetReportDecodeErrorsRequest { | |
| optional int32 session_id = 1 [default = -1]; | |
| optional int32 source_id = 2 [default = -1]; | |
| optional bool report_decode_errors = 3 [default = false]; | |
| } | |
| message SetReportDecodeErrorsResponse { |
| if (m_context.pipeline) | ||
| { | ||
| RIALTO_SERVER_LOG_DEBUG("Pipeline not available yet - cannot apply report_decode_errors setting"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The pipeline check is inverted: if (m_context.pipeline) currently logs 'Pipeline not available yet' and returns, which prevents applying the setting when the pipeline actually exists. This should only early-return when the pipeline is not available, and should call m_player.setReportDecodeErrors(...) when it is.
| void GenericTasksTestsBase::shouldSetReportDecodeErrors() | ||
| { | ||
| EXPECT_CALL(testContext->m_gstPlayer, setReportDecodeErrors()).WillOnce(Return(true)); | ||
| } |
There was a problem hiding this comment.
shouldSetReportDecodeErrors() sets an expectation on setReportDecodeErrors() with no arguments, but the mocked method takes a bool reportDecodeErrors parameter. This will not compile; update the expectation to include the expected bool value (and align with the task trigger).
| void MediaPipelineServiceTests::getQueuedFramesShouldSucceed() | ||
| { | ||
| uint32_t queuedFr; | ||
| EXPECT_TRUE(m_sut->getQueuedFrames(kSessionId, kSourceId, queuedFr)); | ||
| EXPECT_TRUE(immOp); | ||
| } |
There was a problem hiding this comment.
getQueuedFramesShouldSucceed() references immOp, which is not declared in this function (likely a copy/paste from getImmediateOutputShouldSucceed). This will not compile and also doesn't validate the returned queued frame count; assert on queuedFr (e.g., expected 123) instead.
| ::firebolt::rialto::SetReportDecodeErrorsRequest createSetReportDecodeErrorsRequest(int sessionId, int sourceId, | ||
| bool reportDecodeErrors); | ||
| ::firebolt::rialto::GetQueuedFramesRequest createGetQueuedFramesRequest(int sessionId, int sourceId); |
There was a problem hiding this comment.
This header declares/returns ::firebolt::rialto::SetReportDecodeErrorsRequest, but the proto defines ReportDecodeErrorsRequest (and the service signature uses ReportDecodeErrorsRequest). This mismatch will break compilation; update the builder to use the actual generated proto type (or rename the proto message to match the builder).
| TEST_F(GstGenericPlayerPrivateTest, shouldFailToSetReportDecodeErrorsIfSinkIsNull) | ||
| { | ||
| EXPECT_CALL(*m_glibWrapperMock, gObjectGetStub(_, StrEq(kVideoSinkStr), _)) | ||
| .WillOnce(Invoke( | ||
| [&](gpointer object, const gchar *first_property_name, void *element) | ||
| { | ||
| GstElement **elementPtr = reinterpret_cast<GstElement **>(element); | ||
| *elementPtr = nullptr; | ||
| })); | ||
| EXPECT_FALSE(m_sut->setReportDecodeErrors()); | ||
| } | ||
|
|
||
| TEST_F(GstGenericPlayerPrivateTest, shouldFailToSetReportDecodeErrorsIfPropertyDoesntExist) | ||
| { | ||
| expectGetDecoder(m_element); | ||
|
|
||
| expectPropertyDoesntExist(m_glibWrapperMock, m_gstWrapperMock, m_realElement, kReportDecodeErrorsStr); | ||
| EXPECT_CALL(*m_gstWrapperMock, gstObjectUnref(m_realElement)).Times(1); | ||
| EXPECT_FALSE(m_sut->setReportDecodeErrors()); | ||
| } | ||
|
|
||
| TEST_F(GstGenericPlayerPrivateTest, shouldSetReportDecodeErrors) | ||
| { | ||
| expectGetDecoder(m_element); | ||
|
|
||
| expectSetProperty(m_glibWrapperMock, m_gstWrapperMock, m_realElement, kReportDecodeErrorsStr, true); | ||
| EXPECT_CALL(*m_gstWrapperMock, gstObjectUnref(m_realElement)).Times(1); | ||
| EXPECT_TRUE(m_sut->setReportDecodeErrors()); |
There was a problem hiding this comment.
These new tests call m_sut->setReportDecodeErrors() with no arguments, but IGstGenericPlayerPrivate::setReportDecodeErrors was added with a bool reportDecodeErrors parameter. As written, this won't compile; pass the expected bool value (and adjust the expectations accordingly).
|
Hi @Koky2701 : Please use the name "Comcast Cable Communications Management, LLC" for the Comcast company name in the 4 files: SetReportDecodeErrorsTest.cpp (3 files) and GetQueuedFramesTest.cpp. Thank you. |
Koky2701
force-pushed
the
feature/RDKEMW-12692
branch
from
1fe58a5 to
b3811c1
Compare
|
tests/componenttests/server/tests/mediaPipeline/PipelinePropertyTest.cpp:350:0: style: The function 'setReportDecodeErrorsFailure' is never used. [unusedFunction] |
Koky2701
force-pushed
the
feature/RDKEMW-12692
branch
from
b3811c1 to
40ff73a
Compare
|
tests/componenttests/server/tests/mediaPipeline/PipelinePropertyTest.cpp:350:0: style: The function 'setReportDecodeErrorsFailure' is never used. [unusedFunction] |
Koky2701
force-pushed
the
feature/RDKEMW-12692
branch
from
a670f7a to
08fac3c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 62 out of 62 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * This method is asynchronous | ||
| * | ||
| * @param[in] sourceId : The source id. Value should be set to the MediaSource.id returned after attachSource() | ||
| * @param[in] queuedFrames : Number of queued frames |
There was a problem hiding this comment.
The doxygen tag for queuedFrames is incorrect: it’s documented as @param[in] queuedFrames, but the parameter is a non-const reference used to return the value. Update it to @param[out] queuedFrames (and consider clarifying that this call returns the queued frame count).
| * @param[in] queuedFrames : Number of queued frames | |
| * @param[out] queuedFrames : Returns the number of queued frames |
| EXPECT_CALL(*m_mediaPipelineIpcMock, getQueuedFrames(m_kSourceId, _)) | ||
| .WillOnce(DoAll(SetArgReferee<1>(123), Return(true))); | ||
| uint32_t QueuedFrames; | ||
| EXPECT_TRUE(m_mediaPipeline->getQueuedFrames(m_kSourceId, QueuedFrames)); | ||
| EXPECT_TRUE(QueuedFrames); | ||
| } |
There was a problem hiding this comment.
The success test is asserting EXPECT_TRUE(QueuedFrames), which will fail if queued_frames is legitimately 0 and doesn’t verify the expected value (123) that the mock sets. Also, the local variable name QueuedFrames doesn’t match the project’s usual lowerCamelCase style. Prefer asserting the exact expected value (e.g., 123) using a consistently named queuedFrames variable.
|
Coverage statistics of your commit: |
Summary: server_component_tests failed
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-12692
Koky2701
force-pushed
the
feature/RDKEMW-12692
branch
from
08fac3c to
2cdbf6b
Compare
|
Coverage statistics of your commit: |
| * If not stated otherwise in this file or this component's LICENSE file the | ||
| * following copyright and licenses apply: | ||
| * | ||
| * Copyright 2024 Sky UK |
| m_context.pendingReportDecodeErrorsForVideo = m_reportDecodeErrors; | ||
| } | ||
|
|
||
| if (m_context.pipeline) |
There was a problem hiding this comment.
I think it's not correct and should be !m_context.pipeline. The log should be on WARN level.
| } | ||
| else | ||
| { | ||
| m_context.pendingReportDecodeErrorsForVideo = m_reportDecodeErrors; |
There was a problem hiding this comment.
Is it used anywhere? The idea behind caching gstreamer properties is that the decoders or sinks may not have been created yet when this call occurs, so the properties are cached and then applied in SetupElement.cpp when the decoder or sink is attached to the pipeline.
See m_context.pendingBufferingLimit as example.
|
|
||
| bool GstGenericPlayer::getQueuedFrames(const MediaSourceType &mediaSourceType, uint32_t &queuedFrames) | ||
| { | ||
| if (mediaSourceType != MediaSourceType::VIDEO) |
There was a problem hiding this comment.
Do we actually need MediaSourceType in such case? If we will need to expand to audio in the future, we will just expand the API.
| { | ||
| RIALTO_SERVER_LOG_DEBUG("Executing SetReportDecodeErrors for %s source", common::convertMediaSourceType(m_type)); | ||
|
|
||
| if (m_type != MediaSourceType::VIDEO) |
There was a problem hiding this comment.
Do we actually need MediaSourceType in such case? If we will need to expand to audio in the future, we will just expand the API.
Koky2701
force-pushed
the
feature/RDKEMW-12692
branch
from
7420f7c to
103876f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 63 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uint32_t QueuedFrames; | ||
| EXPECT_TRUE(m_mediaPipeline->getQueuedFrames(m_kSourceId, QueuedFrames)); | ||
| EXPECT_EQ(kExpectedQueuedFrames, QueuedFrames); | ||
| } | ||
|
|
||
| /** | ||
| * Test that getQueuedFrames returns failure if the IPC API fails. | ||
| */ | ||
| TEST_F(RialtoClientMediaPipelineGetQueuedFramesTest, GetQueuedFramesFailure) | ||
| { | ||
| EXPECT_CALL(*m_mediaPipelineIpcMock, getQueuedFrames(m_kSourceId, _)).WillOnce(Return(false)); | ||
| uint32_t QueuedFrames; | ||
| EXPECT_FALSE(m_mediaPipeline->getQueuedFrames(m_kSourceId, QueuedFrames)); |
There was a problem hiding this comment.
Variable naming inconsistency: QueuedFrames uses PascalCase, but should use camelCase (queuedFrames) to match the codebase conventions seen throughout the project.
| MOCK_METHOD(void, scheduleAllSourcesAttached, (), (override)); | ||
| MOCK_METHOD(bool, setVideoSinkRectangle, (), (override)); | ||
| MOCK_METHOD(bool, setImmediateOutput, (), (override)); | ||
| MOCK_METHOD(bool, setReportDecodeErrors, (bool reportDecodeErrors), (override)); |
There was a problem hiding this comment.
The mock method signature has a parameter bool reportDecodeErrors, but the interface method IGstGenericPlayerPrivate::setReportDecodeErrors should not take any parameters, following the same pattern as setImmediateOutput(). The implementation in GstGenericPlayer::setReportDecodeErrors() (private method) also doesn't take parameters. The parameter should be removed from this mock declaration.
| * | ||
| * @retval true on success. | ||
| */ | ||
| virtual bool setReportDecodeErrors() = 0; |
There was a problem hiding this comment.
The interface method signature declares a parameter bool reportDecodeErrors, but this should not take any parameters following the same pattern as setImmediateOutput(). The implementation in GstGenericPlayer::setReportDecodeErrors() (private method on line 1380 of GstGenericPlayer.cpp) also doesn't use parameters - it reads the value from m_context.pendingReportDecodeErrorsForVideo. The parameter should be removed from this interface declaration.
| virtual bool setReportDecodeErrors() = 0; | |
| virtual bool setReportDecodeErrors() = 0; |
| if (m_context.pendingReportDecodeErrorsForVideo.has_value()) | ||
| { | ||
| m_player.setReportDecodeErrors(); | ||
| } |
There was a problem hiding this comment.
The setReportDecodeErrors() method is called when the element is a video sink (line 274 checks isVideoSink), but the actual implementation in GstGenericPlayer::setReportDecodeErrors() (line 1394 in GstGenericPlayer.cpp) sets the property on the video decoder, not the sink. This logic should be moved to the decoder setup section (after line 301 where isAudioDecoder is checked) to check for video decoder instead.
Koky2701
force-pushed
the
feature/RDKEMW-12692
branch
from
103876f to
e6c95f0
Compare
Koky2701
force-pushed
the
feature/RDKEMW-12692
branch
from
e6c95f0 to
a33e411
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 63 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| reportDecodeErrors = m_context.pendingReportDecodeErrorsForVideo.value(); | ||
| } | ||
|
|
||
| GstElement *decoder = getDecoder(MediaSourceType::VIDEO); |
There was a problem hiding this comment.
The PR description mentions "report_decode_errors property missing from RialtoMSEVideoSink" which suggests this property should be on the video sink. However, the implementation sets this property on the video decoder (via getDecoder(MediaSourceType::VIDEO) in GstGenericPlayer.cpp line 1394), not the sink. This creates confusion about where the property actually belongs. Please verify whether the property should be on the decoder or the sink, and update either the PR description or the implementation accordingly.
| uint32_t QueuedFrames; | ||
| EXPECT_TRUE(m_mediaPipeline->getQueuedFrames(m_kSourceId, QueuedFrames)); | ||
| EXPECT_EQ(kExpectedQueuedFrames, QueuedFrames); | ||
| } | ||
|
|
||
| /** | ||
| * Test that getQueuedFrames returns failure if the IPC API fails. | ||
| */ | ||
| TEST_F(RialtoClientMediaPipelineGetQueuedFramesTest, GetQueuedFramesFailure) | ||
| { | ||
| EXPECT_CALL(*m_mediaPipelineIpcMock, getQueuedFrames(m_kSourceId, _)).WillOnce(Return(false)); | ||
| uint32_t QueuedFrames; | ||
| EXPECT_FALSE(m_mediaPipeline->getQueuedFrames(m_kSourceId, QueuedFrames)); |
There was a problem hiding this comment.
Variable name QueuedFrames should use camelCase naming convention (queuedFrames) to be consistent with the codebase conventions. For example, in GetImmediateOutputTest.cpp, the similar variable is named immediateOutput (lowercase first letter).
|
|
||
| /** | ||
| * @fn void getImmediateOutput(int session_id, int source_id, bool &immediate_output) | ||
| * @brief Gets the "Immediate Output" property for this source. |
There was a problem hiding this comment.
There is an extra blank line here. The proto file uses a consistent pattern of one blank line between messages, but there are two blank lines here (lines 697 and 698). Remove the extra blank line for consistency.
| /** | |
| * @fn void getImmediateOutput(int session_id, int source_id, bool &immediate_output) | |
| * @brief Gets the "Immediate Output" property for this source. | |
| /** | |
| * @fn void getImmediateOutput(int session_id, int source_id, bool &immediate_output) | |
| * @brief Gets the "Immediate Output" property for this source. | |
| * @brief Gets the "Immediate Output" property for this source. |
Koky2701
force-pushed
the
feature/RDKEMW-12692
branch
from
a33e411 to
9daca68
Compare
Koky2701
force-pushed
the
feature/RDKEMW-12692
branch
from
9daca68 to
5be2aba
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 64 out of 64 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void MediaPipelineServiceTests::mediaPipelineWillGetQueuedFrames() | ||
| { | ||
| EXPECT_CALL(m_mediaPipelineMock, getQueuedFrames(_, _)).WillOnce(DoAll(SetArgReferee<1>(123), Return(true))); | ||
| } | ||
|
|
There was a problem hiding this comment.
This expectation hard-codes 123 when a kQueuedFrames constant already exists in this fixture. Using the constant avoids duplication and keeps the test aligned if the value changes.
| ::google::protobuf::Closure *done) | ||
| { | ||
| RIALTO_SERVER_LOG_DEBUG("entry:"); | ||
| uint32_t queuedFramesNumber; |
There was a problem hiding this comment.
queuedFramesNumber is declared without an initial value. Even though it’s only used on the success path, initializing it (e.g., to 0) makes the code more robust against accidental misuse and can avoid static-analysis warnings about potentially uninitialized output values.
| uint32_t queuedFramesNumber; | |
| uint32_t queuedFramesNumber{0}; |
| .WillOnce(DoAll(SetArgReferee<1>(123), Return(true))); | ||
| uint32_t QueuedFrames; | ||
| EXPECT_TRUE(m_mediaPipeline->getQueuedFrames(m_kSourceId, QueuedFrames)); | ||
| EXPECT_EQ(kExpectedQueuedFrames, QueuedFrames); |
There was a problem hiding this comment.
Local variable QueuedFrames uses UpperCamelCase, which is inconsistent with surrounding lowerCamelCase variable naming in tests (e.g., queuedFrames). Renaming improves readability and consistency.
Koky2701
force-pushed
the
feature/RDKEMW-12692
branch
from
5be2aba to
19f5878
Compare
Summary: Pending was not used.
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-12692
Koky2701
force-pushed
the
feature/RDKEMW-12692
branch
from
19f5878 to
fb0f618
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 63 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * This method is synchronous, it gets the queued frames property | ||
| * | ||
| * @param[in] sourceId : The source id. Value should be set to the MediaSource.id returned after attachSource() | ||
| * @param[out] queuedFrames : Get queued frames on the decoder |
There was a problem hiding this comment.
The documentation says "Get queued frames on the decoder" but this should be more clear about the parameter direction. It should read "The number of queued frames on the decoder" or "Returns the number of queued frames on the decoder" to match the style of similar getter documentation in this interface.
| * @param[out] queuedFrames : Get queued frames on the decoder | |
| * @param[out] queuedFrames : The number of queued frames on the decoder |
| * This method is synchronous, it gets the queued frames property | ||
| * | ||
| * @param[in] sourceId : The source id. Value should be set to the MediaSource.id returned after attachSource() | ||
| * @param[out] queuedFrames : Get queued frames on the decoder |
There was a problem hiding this comment.
The documentation says "Get queued frames on the decoder" but should be more descriptive for an output parameter. It should read "The number of queued frames on the decoder" or "Returns the number of queued frames on the decoder" to match the documentation style for similar getter methods.
| * @param[out] queuedFrames : Get queued frames on the decoder | |
| * @param[out] queuedFrames : The number of queued frames on the decoder |
| @@ -333,7 +337,6 @@ void SetupElement::execute() const | |||
| { | |||
| m_gstWrapper->gstBaseParseSetPtsInterpolation(GST_BASE_PARSE(m_element), FALSE); | |||
| } | |||
There was a problem hiding this comment.
This deletion of a blank line appears to be an unintentional change unrelated to the feature being added. The blank line separation between the conditional block and the gstObjectUnref call improves code readability and should be retained.
| } | |
| } |
| * If not stated otherwise in this file or this component's LICENSE file the | ||
| * following copyright and licenses apply: | ||
| * | ||
| * Copyright 2026 Sky UK |
There was a problem hiding this comment.
The copyright year is inconsistent across the new files. This file has "Copyright 2026 Sky UK" while other new files in this PR have "Copyright 2025 Sky UK" and "Copyright 2024 Sky UK". Since the PR is being created in early 2025 (based on context), the copyright year should be "2025" to match other recently created files in the PR.
| * Copyright 2026 Sky UK | |
| * Copyright 2025 Sky UK |
| message ReportDecodeErrorsResponse { | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
There is an extra blank line here that is inconsistent with the formatting of other message definitions in this file. Message pairs (Request/Response) should have only one blank line between them and the next message, as seen with other definitions like SetImmediateOutputResponse and getImmediateOutput.
| * If not stated otherwise in this file or this component's LICENSE file the | ||
| * following copyright and licenses apply: | ||
| * | ||
| * Copyright 2024 Sky UK |
There was a problem hiding this comment.
The copyright year "2024" is inconsistent with other new test files in this PR that use "2025". Since this is a new file being added in early 2025, it should have "Copyright 2025 Sky UK" to match files like SetReportDecodeErrorsTest.cpp and GetQueuedFramesTest.cpp in the same directory.
|
Coverage statistics of your commit: |
Summary: Redundant argument.
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-12692
4 participants
Summary: 'report_decode_errors' property missing from RialtoMSEVideoSink
Type: Feature
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-12692