RDKEMW-15078: Audio/Video decoder capabilities from YAML config - #467
RDKEMW-15078: Audio/Video decoder capabilities from YAML config#467skywojciechowskim wants to merge 46 commits into
Conversation
|
Pull request title must follow the pattern: Pull request description must follow the Commit message format for RDK-E:
< JIRA TICKET >: < one line summary of change less than 65 characters > |
There was a problem hiding this comment.
Pull request overview
Feature adds YAML-backed audio/video decoder capabilities and exposes them through server/client capabilities APIs and the IPC layer.
Changes:
- Introduces
YamlCppWrapperto parse decoder capabilities from YAML config files and wires it into the wrappers factory/accessor system. - Adds public capability data types (
AudioDecoderCapabilities,VideoDecoderCapabilities) and new IPC/proto RPCs for retrieving them. - Updates server/client implementations plus unit/component tests and CI dependencies to cover the new APIs.
Reviewed changes
Copilot reviewed 60 out of 60 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| wrappers/source/YamlCppWrapperAccessor.cpp | Factory accessor glue for the new YAML wrapper factory. |
| wrappers/source/YamlCppWrapper.cpp | YAML parsing + construction of audio/video capability structures. |
| wrappers/source/FactoryAccessor.cpp | Adds yamlCppWrapperFactory() creation to the wrapper factory accessor. |
| wrappers/interface/IYamlCppWrapper.h | New wrapper interface for reading decoder capabilities from YAML. |
| wrappers/interface/IFactoryAccessor.h | Extends factory accessor interface to provide IYamlCppWrapperFactory. |
| wrappers/include/YamlCppWrapper.h | Concrete wrapper/factory declarations. |
| wrappers/include/FactoryAccessor.h | Adds m_yamlCppWrapperFactory + accessor override. |
| wrappers/CMakeLists.txt | Adds yaml-cpp dependency and builds new wrapper sources. |
| tests/unittests/media/server/service/mediaPipelineService/MediaPipelineServiceTestsFixture.h | Adds test helpers for new capabilities calls. |
| tests/unittests/media/server/service/mediaPipelineService/MediaPipelineServiceTestsFixture.cpp | Implements expectations + assertions for new capabilities calls. |
| tests/unittests/media/server/service/mediaPipelineService/MediaPipelineServiceTests.cpp | Adds unit tests for audio/video capabilities retrieval. |
| tests/unittests/media/server/service/CMakeLists.txt | Adds matchers include dir for new matcher usage. |
| tests/unittests/media/server/mocks/service/MediaPipelineServiceMock.h | Extends service mock with new capabilities methods. |
| tests/unittests/media/server/mocks/main/MediaPipelineCapabilitiesMock.h | Extends main capabilities mock with new methods. |
| tests/unittests/media/server/mocks/gstplayer/GstCapabilitiesMock.h | Extends gst capabilities mock with new methods. |
| tests/unittests/media/server/main/mediaPipelineCapabilities/MediaPipelineCapabilitiesTest.cpp | Adds tests for new server-side capabilities API. |
| tests/unittests/media/server/main/CMakeLists.txt | Adds matchers include dir for new matcher usage. |
| tests/unittests/media/server/ipc/mediaPipelineCapabilitiesModule/MediaPipelineCapabilitiesModuleServiceTestsFixture.h | Adds IPC service fixture helpers for new RPCs. |
| tests/unittests/media/server/ipc/mediaPipelineCapabilitiesModule/MediaPipelineCapabilitiesModuleServiceTestsFixture.cpp | Implements new RPC request/response helpers and expectations. |
| tests/unittests/media/server/ipc/mediaPipelineCapabilitiesModule/MediaPipelineCapabilitiesModuleServiceTests.cpp | Adds IPC module service tests for new RPCs. |
| tests/unittests/media/server/gstplayer/genericPlayer/GstCapabilitiesTest.cpp | Updates GstCapabilities tests for YAML wrapper dependency + new getters. |
| tests/unittests/media/client/mocks/ipc/MediaPipelineCapabilitiesIpcMock.h | Extends client IPC mock with new methods. |
| tests/unittests/media/client/main/mediaPipelineCapabilities/MediaPipelineCapabilitiesTest.cpp | Adds client-side capabilities tests for new getters. |
| tests/unittests/media/client/ipc/mediaPipelineCapabilitiesIpc/MediaPipelineCapabilitiesIpcTest.cpp | Adds IPC client tests for new capabilities RPCs. |
| tests/componenttests/server/tests/mediaPipelineCapabilities/MediaPipelineCapabilitiesTest.cpp | Adds component tests for new capabilities RPCs. |
| tests/componenttests/server/fixtures/RialtoServerComponentTest.h | Adds YAML wrapper mocks to server component-test fixture. |
| tests/componenttests/server/fixtures/RialtoServerComponentTest.cpp | Wires YAML wrapper mocks and expected config responses. |
| tests/componenttests/server/common/MessageBuilders.h | Adds request builders for new capabilities RPCs. |
| tests/componenttests/server/common/MessageBuilders.cpp | Implements request builders for new capabilities RPCs. |
| tests/componenttests/server/common/Constants.h | Adds constants for expected audio/video capabilities in CTs. |
| tests/componenttests/server/common/ActionTraits.h | Adds CT action traits for new RPC methods. |
| tests/componenttests/client/tests/mse/MediaPipelineCapabilitiesTest.cpp | Extends client CT flow to call new capabilities APIs. |
| tests/componenttests/client/tests/base/MediaPipelineTestMethods.h | Adds CT helper declarations for new capabilities calls. |
| tests/componenttests/client/tests/base/MediaPipelineTestMethods.cpp | Implements CT helper expectations + assertions for new capabilities calls. |
| tests/componenttests/client/mocks/MediaPipelineCapabilitiesModuleMock.h | Extends CT protobuf stub mock with new RPCs + response builders. |
| tests/common/matchers/MediaPipelineStructureMatchers.h | Adds matcher for comparing decoder capabilities structures. |
| tests/common/externalLibraryMocks/YamlCppWrapperMock.h | Adds mocks for IYamlCppWrapper and its factory. |
| proto/mediapipelinecapabilitiesmodule.proto | Adds new RPCs + messages/enums for audio/video capabilities transport. |
| media/server/service/source/MediaPipelineService.h | Exposes new capabilities getters on server service. |
| media/server/service/source/MediaPipelineService.cpp | Implements service getters delegating to capabilities component. |
| media/server/service/include/IMediaPipelineService.h | Extends service interface with new capabilities getters. |
| media/server/main/source/MediaPipelineCapabilities.cpp | Exposes new getters delegating to IGstCapabilities. |
| media/server/main/include/MediaPipelineCapabilities.h | Extends server capabilities class interface. |
| media/server/ipc/source/MediaPipelineCapabilitiesModuleService.cpp | Adds RPC implementations + conversions to/from proto for capabilities. |
| media/server/ipc/include/MediaPipelineCapabilitiesModuleService.h | Declares new RPC handler methods. |
| media/server/gstplayer/source/GstCapabilities.cpp | Injects YAML wrapper, loads capabilities at construction, adds getters. |
| media/server/gstplayer/interface/IGstCapabilities.h | Adds new capabilities getters to gst capabilities interface. |
| media/server/gstplayer/include/GstCapabilities.h | Adds YAML wrapper dependency + stores capabilities in members. |
| media/public/include/VideoDecoderCapabilities.h | New public types for video decoder capabilities. |
| media/public/include/MediaCommon.h | Adds DecoderCapabilitiesStatus enum for config read result. |
| media/public/include/IMediaPipelineCapabilities.h | Extends public capabilities interface with new getters. |
| media/public/include/AudioDecoderCapabilities.h | New public types for audio decoder capabilities. |
| media/public/CMakeLists.txt | Installs the new public headers. |
| media/client/main/source/MediaPipelineCapabilities.cpp | Adds client-facing getters delegating to IPC implementation. |
| media/client/main/include/MediaPipelineCapabilities.h | Declares client-facing getters. |
| media/client/ipc/source/MediaPipelineCapabilitiesIpc.cpp | Implements new IPC calls + proto-to-struct conversions. |
| media/client/ipc/include/MediaPipelineCapabilitiesIpc.h | Declares new IPC getters. |
| .github/workflows/valgrind_ut.yml | Adds yaml-cpp dev package to CI dependencies. |
| .github/workflows/native_rialto_build.yml | Adds yaml-cpp dev package to native build workflow dependencies. |
| .github/workflows/actions/init_ut/action.yml | Adds yaml-cpp dev package to UT init action dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
|
Coverage statistics of your commit: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 60 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Coverage statistics of your commit: |
|
Coverage statistics of your commit: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 70 changed files in this pull request and generated no new comments.
Suppressed comments (5)
wrappers/source/YamlCppWrapper.cpp:646
getVideoDecoderCapabilities()appends tocapabilities.capabilitieswithout clearing it first. If the sameVideoDecoderCapabilitiesinstance is reused (or if an exception occurs mid-parse), callers can end up with stale/partial data even when returning an error status. Reset the output struct at the beginning of the function to keep it in a known state on failure.
wrappers/source/YamlCppWrapper.cpp:613getAudioDecoderCapabilities()appends tocapabilities.capabilitieswithout clearing it first. If the sameAudioDecoderCapabilitiesinstance is reused (or if an exception occurs mid-parse), callers can end up with stale/partial data even when returning an error status. Reset the output struct at the beginning of the function to keep it in a known state on failure.
openspec/changes/hfp-schema-v1-migration/specs/audio-decoder-capabilities/spec.md:6- This spec says all
AudioProfileCapabilityfields areuint32_t, but the implementation inmedia/public/include/AudioDecoderCapabilities.hdefinesmaxBitrateInBpsasuint64_t(and the YAML/proto parsing also uses 64-bit). The spec should match the public API types to avoid misleading consumers.
The system SHALL define an `AudioProfileCapability` struct containing `maxBitrateInBps`,
`maxChannels`, `maxSampleRateInHz`, and `maxBitDepth`, all as `uint32_t` (required, not
optional).
media/public/include/MediaCommon.h:120
MediaSourceStatus::OK's documentation is currently broken: the stray/**< Config file read successfully */comment is unrelated toMediaSourceStatusand the actual OK description is detached onto the next line. This will generate confusing/incorrect API docs for a public header.
OK,
/**< Config file read successfully */ /**< Source data provided without error. */
EOS, /**< Source reached the end of stream. */
ERROR, /**< There was an error providing source data. */
openspec/changes/hfp-schema-v1-migration/specs/video-decoder-capabilities/spec.md:7
- This spec describes
profilesanddynamicRangesas vectors of strings, but the public API inmedia/public/include/VideoDecoderCapabilities.huses typed profile structs (e.g.H264Profile) andstd::vector<DynamicRange>(enum). Update the spec wording to match the actual API surface so readers don't assume string-based types.
The system SHALL define per-codec capability structs (`Mpeg2CodecCapability`,
`H264CodecCapability`, `H265CodecCapability`, `Vp9CodecCapability`,
`Av1CodecCapability`), each containing a `profiles` vector and a `dynamicRanges`
vector of strings.
|
Coverage statistics of your commit: |
Add all remaining audio codec types (MpegAudio, RealAudio, Usac, Dts, Avs) and expand video codec profiles to cover all H264/H265/VP9/AV1/MPEG2 profile types, levels, and all DynamicRange values (HLG, HDR10PLUS, DOLBY_VISION). MediaPipelineCapabilitiesIpc.cpp coverage: 42.5% -> 92.9% lines, 100% functions. Overall PR coverage: 84.6% lines / 93.2% functions (above master 84.4% / 92.7%)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 70 changed files in this pull request and generated no new comments.
Suppressed comments (7)
wrappers/source/YamlCppWrapper.cpp:641
YAML::LoadFile()throwsYAML::BadFilewhen the capabilities file is missing/unreadable. The current catch-all maps this toSCHEMA_VALIDATION_FAILED, which prevents callers (e.g.GstCapabilities) from treating missing HFP config as the expectedCONFIG_NOT_FOUNDcase.
wrappers/source/YamlCppWrapper.cpp:674- Same as audio: a missing/unreadable file will throw
YAML::BadFileand is currently reported asSCHEMA_VALIDATION_FAILEDinstead ofCONFIG_NOT_FOUND. This makes the server treat an expected absence of HFP config as a real failure.
media/server/ipc/source/MediaPipelineCapabilitiesModuleService.cpp:702 - Same unused-variable issue as in
getSupportedAudioCapabilities():ipcControlleris assigned but never used after the type check.
RIALTO_SERVER_LOG_DEBUG("entry:");
auto ipcController = dynamic_cast<firebolt::rialto::ipc::IController *>(controller);
if (!ipcController)
{
RIALTO_SERVER_LOG_ERROR("ipc library provided incompatible controller object");
controller->SetFailed("ipc library provided incompatible controller object");
done->Run();
return;
}
tests/unittests/wrappers/YamlCppWrapperTest.cpp:80
- Same as the audio file-not-found test: missing video YAML should be treated as
CONFIG_NOT_FOUND(expected on platforms without HFP config), not as a schema validation failure.
media/public/include/MediaCommon.h:123 MediaSourceStatus::OKhas a stray/incorrect Doxygen comment on the next line ("Config file read successfully"), which appears unrelated to this enum and breaks the existing per-enum-value documentation formatting.
OK,
/**< Config file read successfully */ /**< Source data provided without error. */
EOS, /**< Source reached the end of stream. */
tests/unittests/wrappers/YamlCppWrapperTest.cpp:74
- These file-not-found tests currently expect
SCHEMA_VALIDATION_FAILED, butGstCapabilities(and the newDecoderCapabilitiesStatusenum) distinguish missing config (CONFIG_NOT_FOUND) from schema errors. OnceYamlCppWrappermapsYAML::BadFiletoCONFIG_NOT_FOUND, this expectation should be updated.
This issue also appears on line 76 of the same file.
media/server/ipc/source/MediaPipelineCapabilitiesModuleService.cpp:679
ipcControlleris only used to validate the controller type, but the local variable itself is never used afterwards. This can trigger-Wunused-variablewarnings (and potential build failures if warnings are treated as errors).
This issue also appears on line 694 of the same file.
RIALTO_SERVER_LOG_DEBUG("entry:");
auto ipcController = dynamic_cast<firebolt::rialto::ipc::IController *>(controller);
if (!ipcController)
{
RIALTO_SERVER_LOG_ERROR("ipc library provided incompatible controller object");
controller->SetFailed("ipc library provided incompatible controller object");
done->Run();
return;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 70 changed files in this pull request and generated no new comments.
Suppressed comments (5)
wrappers/source/YamlCppWrapper.cpp:614
getAudioDecoderCapabilities()appends to the provided output struct without clearing it first. If the sameAudioDecoderCapabilitiesinstance is reused, results will accumulate/duplicate unexpectedly.
wrappers/source/YamlCppWrapper.cpp:641- Missing/absent YAML file is likely to throw from
YAML::LoadFile(...)(rather than returning a null node), which means the function will currently returnSCHEMA_VALIDATION_FAILEDinstead ofCONFIG_NOT_FOUND. This also makesDecoderCapabilitiesStatus::CONFIG_NOT_FOUNDeffectively unreachable here.
wrappers/source/YamlCppWrapper.cpp:647 getVideoDecoderCapabilities()appends to the provided output struct without clearing it first. Reusing aVideoDecoderCapabilitiesinstance would accumulate/duplicate entries unexpectedly.
wrappers/source/YamlCppWrapper.cpp:674- Missing/absent YAML file is likely to throw from
YAML::LoadFile(...), so this currently returnsSCHEMA_VALIDATION_FAILEDinstead ofCONFIG_NOT_FOUNDon file-not-found.
media/public/include/MediaCommon.h:123 MediaSourceStatus::OKhas a stray/incorrect doc comment ("Config file read successfully") and the doxygen comment for OK is no longer attached to the enumerator, which makes the public API docs misleading.
OK,
/**< Config file read successfully */ /**< Source data provided without error. */
EOS, /**< Source reached the end of stream. */
ERROR, /**< There was an error providing source data. */
CODEC_CHANGED, /**< The codec has changed and the decoder must be reconfigured */
|
Coverage statistics of your commit: |
…dec coverage Add all audio codec types (AAC/MP3/ALAC/SBC/DolbyAC3/DolbyAC4/DolbyEac3/ DolbyTruehd/FLAC/Vorbis/Opus/MpegAudio/RealAudio/USAC/DTS/AVS) and all video codec types (H264/H265/VP9/AV1/MPEG2) to the server IPC capabilities module test fixture. Add GstCapabilities tests for CONFIG_NOT_FOUND and error status paths. Unit-test-only coverage: 84.4% lines / 92.9% functions (>= master 84.4% / 92.7%)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 70 changed files in this pull request and generated no new comments.
Suppressed comments (6)
wrappers/source/YamlCppWrapper.cpp:641
YAML::LoadFile()throwsYAML::BadFilewhen the YAML file is missing/unreadable, so the currentcatch (std::exception)path will incorrectly map a missing config toSCHEMA_VALIDATION_FAILED(makingCONFIG_NOT_FOUNDeffectively unreachable for the common "file not present" case). CatchYAML::BadFileexplicitly and returnCONFIG_NOT_FOUND.
wrappers/source/YamlCppWrapper.cpp:674- Same issue as audio:
YAML::LoadFile()will throwYAML::BadFileif the video capabilities file is missing, but the code currently returnsSCHEMA_VALIDATION_FAILEDfor all exceptions. This prevents callers from distinguishing missing config vs. schema errors.
openspec/changes/hfp-schema-v1-migration/specs/audio-decoder-capabilities/spec.md:6 - This requirement says all
AudioProfileCapabilityfields areuint32_t, but the implementation and the protobuf contract useuint64/uint64_tformaxBitrateInBps(e.g.AudioProfileCapability::maxBitrateInBpsandmax_bitrate_in_bps). Please align the spec text with the actual types to avoid confusion for API consumers.
The system SHALL define an `AudioProfileCapability` struct containing `maxBitrateInBps`,
`maxChannels`, `maxSampleRateInHz`, and `maxBitDepth`, all as `uint32_t` (required, not
optional).
wrappers/CMakeLists.txt:55
yaml-cppis linked unconditionally (seetarget_link_libraries(... yaml-cpp)), butfind_package(yaml-cpp REQUIRED)is currently only called in the non-UnitTests/non-ComponentTests branch. This can make test builds fail at configure/link time depending on the toolchain setup. Either link yaml-cpp conditionally, or (simplest) makefind_package(yaml-cpp REQUIRED)unconditional so it matches the unconditional link dependency.
media/public/include/MediaCommon.h:119MediaSourceStatus::OKhas a garbled/incorrect doxygen comment ("Config file read successfully") that is unrelated to media source reads and also breaks the one-enumerator-per-line formatting. This looks like an accidental paste and will generate confusing API docs.
enum class MediaSourceStatus
{
OK,
/**< Config file read successfully */ /**< Source data provided without error. */
EOS, /**< Source reached the end of stream. */
openspec/changes/hfp-schema-v1-migration/tasks.md:9
- Task 2.1 states all
AudioProfileCapabilityfields areuint32_t, but the code/proto useuint64_t/uint64formaxBitrateInBps. Update this task text to match the implemented types so the migration checklist stays accurate.
- [x] 2.1 Add `AudioProfileCapability` struct with `maxBitrateInBps`, `maxChannels`, `maxSampleRateInHz`, `maxBitDepth` (all `uint32_t`, not optional)
|
Coverage statistics of your commit: |
|
Coverage statistics of your commit: |
|
Coverage statistics of your commit: |
RDKEMW-15078: Audio/Video decoder capabilities from YAML config
Reason for change: Add AudioDecoderCapabilities and VideoDecoderCapabilities with YAML config parsing, IPC serialization and unit test coverage.
Test Procedure: https://jira.rdkcentral.com/jira/browse/RDKEMW-15078