-
Notifications
You must be signed in to change notification settings - Fork 18
Feature/rdkemw 12692 #438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Feature/rdkemw 12692 #438
Changes from 4 commits
d8f1b1c
c0ec6dc
0c57f2f
3c0c5ae
d704fc2
c8e44b6
c6fa3c4
7732ba1
77701f6
2cdbf6b
fb0f618
a0c4f24
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -567,6 +567,38 @@ bool MediaPipelineIpc::setImmediateOutput(int32_t sourceId, bool immediateOutput | |
| return true; | ||
| } | ||
|
|
||
| bool MediaPipelineIpc::setReportDecodeErrors(int32_t sourceId, bool reportDecodeErrors) | ||
| { | ||
| if (!reattachChannelIfRequired()) | ||
| { | ||
| RIALTO_CLIENT_LOG_ERROR("Reattachment of the ipc channel failed, ipc disconnected"); | ||
| return false; | ||
| } | ||
|
|
||
| firebolt::rialto::ReportDecodeErrorsRequest request; | ||
|
|
||
| request.set_session_id(m_sessionId); | ||
| request.set_source_id(sourceId); | ||
| request.set_report_decode_errors(reportDecodeErrors); | ||
|
|
||
| firebolt::rialto::ReportDecodeErrorsResponse response; | ||
| auto ipcController = m_ipc.createRpcController(); | ||
| auto blockingClosure = m_ipc.createBlockingClosure(); | ||
| m_mediaPipelineStub->setReportDecodeErrors(ipcController.get(), &request, &response, blockingClosure.get()); | ||
|
|
||
| // wait for the call to complete | ||
| blockingClosure->wait(); | ||
|
|
||
| // check the result | ||
| if (ipcController->Failed()) | ||
| { | ||
| RIALTO_CLIENT_LOG_ERROR("failed to set report decode error due to '%s'", ipcController->ErrorText().c_str()); | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
Comment on lines
+570
to
+600
|
||
|
|
||
| bool MediaPipelineIpc::getImmediateOutput(int32_t sourceId, bool &immediateOutput) | ||
| { | ||
| if (!reattachChannelIfRequired()) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1227,6 +1227,18 @@ class IMediaPipeline | |||||
| */ | ||||||
| virtual bool setImmediateOutput(int32_t sourceId, bool immediateOutput) = 0; | ||||||
|
|
||||||
| /** | ||||||
| * @brief Sets the "Report Decode Errors" property for this source. | ||||||
| * | ||||||
| * 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 | ||||||
|
||||||
| * @param[in] immediateOutput : Set Report Decode Errors mode on the sink | |
| * @param[in] reportDecodeErrors : Set Report Decode Errors mode on the sink |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -494,6 +494,21 @@ class IGenericPlayerTaskFactory | |||||||||||||||||||||||||||||||||
| const firebolt::rialto::MediaSourceType &type, | ||||||||||||||||||||||||||||||||||
| bool immediateOutput) const = 0; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * @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 | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
| * @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 |
Outdated
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,49 @@ | ||||||||||||||||||
| /* | ||||||||||||||||||
| * If not stated otherwise in this file or this component's LICENSE file the | ||||||||||||||||||
| * following copyright and licenses apply: | ||||||||||||||||||
| * | ||||||||||||||||||
| * Copyright 2024 Sky UK | ||||||||||||||||||
|
||||||||||||||||||
| * Copyright 2024 Sky UK | |
| * Copyright 2025 Sky UK |
Outdated
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright year should be 2025, not 2024. This is inconsistent with other new files in this PR which use 2025.
| * Copyright 2024 Sky UK | |
| * Copyright 2025 Sky UK |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong year
Outdated
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright year is 2024, but other new files in this PR use 2026. This should be updated to 2026 for consistency with files like SetReportDecodeErrorsTest.cpp.
| * Copyright 2024 Sky UK | |
| * | |
| * Licensed under the Apache License, Version 2.0 (the "License"); | |
| * you may not use this file except in compliance with the License. | |
| * 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. |
Outdated
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Outdated
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -654,6 +654,16 @@ bool GstGenericPlayer::setImmediateOutput(const MediaSourceType &mediaSourceType | |||||||||||||||||
| return true; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| bool GstGenericPlayer::setReportDecodeErrors(const MediaSourceType &mediaSourceType, bool reportDecodeErrors) | ||||||||||||||||||
| { | ||||||||||||||||||
| if (!m_workerThread) | ||||||||||||||||||
| return false; | ||||||||||||||||||
|
|
||||||||||||||||||
| m_workerThread->enqueueTask( | ||||||||||||||||||
| m_taskFactory->createSetReportDecodeErrors(m_context, *this, mediaSourceType, reportDecodeErrors)); | ||||||||||||||||||
| return true; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| bool GstGenericPlayer::getImmediateOutput(const MediaSourceType &mediaSourceType, bool &immediateOutputRef) | ||||||||||||||||||
| { | ||||||||||||||||||
| bool returnValue{false}; | ||||||||||||||||||
|
|
@@ -1360,6 +1370,33 @@ bool GstGenericPlayer::setImmediateOutput() | |||||||||||||||||
| return result; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| bool GstGenericPlayer::setReportDecodeErrors(bool reportDecodeErrors) | ||||||||||||||||||
| { | ||||||||||||||||||
| bool result{false}; | ||||||||||||||||||
| GstElement *decoder = getDecoder(MediaSourceType::VIDEO); | ||||||||||||||||||
|
||||||||||||||||||
| 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); |
Copilot
AI
Feb 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Outdated
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"); |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,52 @@ | ||||||||||||||||||||||||||||||||||||||||
| /* | ||||||||||||||||||||||||||||||||||||||||
| * If not stated otherwise in this file or this component's LICENSE file the | ||||||||||||||||||||||||||||||||||||||||
| * following copyright and licenses apply: | ||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||
| * Copyright 2024 Sky UK | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
| * Copyright 2024 Sky UK | |
| * Copyright 2025 Sky UK |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright year should be 2025 to match the PR creation date and the pattern used in other new files in this PR (like SetReportDecodeErrorsTest.cpp). Using 2024 is inconsistent.
| * Copyright 2024 Sky UK | |
| * Copyright 2025 Sky UK |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright year should be 2025, not 2024. This is inconsistent with other new files in this PR which use 2025.
| * Copyright 2024 Sky UK | |
| * Copyright 2025 Sky UK |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright year is 2024, but other new files in this PR use 2026. This should be updated to 2026 for consistency with files like SetReportDecodeErrorsTest.cpp.
| * Copyright 2024 Sky UK | |
| * | |
| * Licensed under the Apache License, Version 2.0 (the "License"); | |
| * you may not use this file except in compliance with the License. | |
| * Copyright 2026 Sky UK | |
| * | |
| * Licensed under the Apache License, Version 2.0 (the "License"); | |
| * you may use this file except in compliance with the License. |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not correct and should be !m_context.pipeline. The log should be on WARN level.
Outdated
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Outdated
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation doesn't check if the type is VIDEO (like SetImmediateOutput does for its type parameter), and directly calls setReportDecodeErrors without storing it in a pending variable. This is inconsistent with the SetImmediateOutput task pattern. If report_decode_errors is only applicable to VIDEO sources, this should be validated. Additionally, if the decoder is not yet available when this is called, there's no pending mechanism to retry later, unlike the immediate-output property which uses pendingImmediateOutputForVideo.
| if (m_context.pipeline) | |
| { | |
| m_player.setReportDecodeErrors(m_reportDecodeErrors); | |
| } | |
| // report_decode_errors is only applicable to VIDEO sources, mirror SetImmediateOutput pattern | |
| if (m_type != MediaSourceType::VIDEO) | |
| { | |
| RIALTO_SERVER_LOG_WARN("SetReportDecodeErrors ignored for non-video source type: %s", | |
| common::convertMediaSourceType(m_type)); | |
| return; | |
| } | |
| if (!m_context.pipeline) | |
| { | |
| RIALTO_SERVER_LOG_DEBUG("Pipeline not available yet - cannot apply report-decode-errors setting"); | |
| return; | |
| } | |
| m_player.setReportDecodeErrors(m_reportDecodeErrors); |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new SetReportDecodeErrors task is missing unit test coverage. A test file should be added similar to SetImmediateOutputTest.cpp to verify the task's behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter name in the documentation is incorrect. It should be "reportDecodeErrors" instead of "immediateOutput" to match the actual parameter name.