rdkemw 6645 ds setting vendor support #169
Conversation
Reason for change: This ensures that platform-dependent configurations are centralized across different HW platforms. Test Procedure:NA Risks: High Signed-off-by:gsanto722 <grandhi_santoshkumar@comcast.com>
|
Addressed review comments of below PR and created new from develop. |
srinivasgtl
left a comment
srinivasgtl
left a comment
There was a problem hiding this comment.
I have made a few comments and some comments may need to be handled at different places too.
include/dsAVDTypes.h
Outdated
| * compressions, stereo modes, and connected video output ports. | ||
| * | ||
| * @var dsAudioPortId_t typeid | ||
| * Type of video port (e.g. HDMI, SCART, etc.). |
There was a problem hiding this comment.
Can we set the structure dsAudioPortType_t clearly instead of just giving as example)
There was a problem hiding this comment.
Updated. Please check new code changes.
include/dsAVDTypes.h
Outdated
| * Type of video port (e.g. HDMI, SCART, etc.). | ||
| * | ||
| * @var const char *name | ||
| * Name of the audio port type. |
There was a problem hiding this comment.
What is the naming that we follow here? Just to avoid different names by different platforms, still referring the same port type.
include/dsAVDTypes.h
Outdated
| * Number of supported audio encodings. | ||
| * | ||
| * @var audioFeatures_t::supportedEncodings | ||
| * Array of supported audio encodings. The size of the array is defined by dsAUDIO_ENC_MAX. |
There was a problem hiding this comment.
Are we using the structure dsAudioEncoding_t here? if yes, then we need to reflect this in the spec.
| dsAudioStereoMode_t supportedStereoModes[dsAUDIO_STEREO_MAX]; | ||
| size_t numConnectedVOPs; | ||
| dsVideoPortPortId_t connectedVOPs[dsVIDEOPORT_TYPE_MAX]; | ||
| }dsAudioFeatures_t; |
There was a problem hiding this comment.
how does this datatype differ from dsAudioTypeConfig_t except for the ConnectedVOPs. Do we really need this? We can discuss more on the requirement.
There was a problem hiding this comment.
This dsVideoPortFeature_t struct is combine of dsAudioTypeConfig_t and dsAudioPortConfig_t struct since both struct has few common element members.
| dsAudioStereoMode_t supportedStereoModes[dsAUDIO_STEREO_MAX]; | ||
| size_t numConnectedVOPs; | ||
| dsVideoPortPortId_t connectedVOPs[dsVIDEOPORT_TYPE_MAX]; | ||
| }dsAudioFeatures_t; |
There was a problem hiding this comment.
How are the other capabilities like MS12 variants support, ARC/eARC support, different stereo modes supports etc are derived from this audio capability structure?
There was a problem hiding this comment.
There are separate capabilites APIs are available and can use same to get capabilities.
Ex:
dsError_t dsGetAudioCapabilities(intptr_t handle, int *capabilities);
| * @brief Retrieves the list of supported audio output port types and the number of audio ports available. | ||
| * | ||
| * @param[out] kSupportedPortTypes Pointer to an array where the supported audio port types will be stored. | ||
| * The caller must ensure the array is properly allocated to hold the data. |
There was a problem hiding this comment.
To make this more precise, why can't we use a two call format. First call should o with kSupportedportTypes as NULL so that we only get the number of Audio ports supporte and later call with a properly allocated array to read the SupportedPortTypes.
There was a problem hiding this comment.
It is better idea. Will discusses about feasibility.
include/dsAVDTypes.h
Outdated
| * Specifies the restricted resolution for the video port (-1 if none). | ||
| * | ||
| * @var int8_t defaultResIndex | ||
| * Index of the default resolution in the resolutions array. |
There was a problem hiding this comment.
Can you point to the resolution array here?
There was a problem hiding this comment.
Update the data type and details for variable.
const char * defaultResIndex; // the default resolution stirng
| int8_t defaultResIndex; // Index of the default resolution | ||
| size_t numconnectedAOPs; // Number of connected audio output ports | ||
| uint8_t connectedAOP[dsAUDIOPORT_TYPE_MAX]; // Array of connected audio output ports | ||
| } dsVideoPortFeatures_t; |
There was a problem hiding this comment.
how do we represent different Video formats supported, different video content type supported etc as part of capabilities?
There was a problem hiding this comment.
this dsVideoPortFeatures_t struct using for configuration load in MW side.
As per my understanding, the video capabilities and select resolution APIs are using for video content.
dsColorDepthCapabilities(), dsSetResolution().
Correct me, If I am wrong.
include/dsAudio.h
Outdated
| * @return dsError_t Returns a status code indicating success or the type of error encountered. | ||
| * | ||
| * Possible values include: | ||
| * - dsErrorNone: Operation succeeded. |
There was a problem hiding this comment.
Can we follow the same method of defining the error code as done in the other APIs. Eg: Like from line 181 to 185 dsAudioPortTerm API.
include/dsFPD.h
Outdated
| * @note The caller is responsible for allocating memory for the indicatorColors | ||
| * array before calling this function. | ||
| */ | ||
| dsError_t dsFPGetSupportedLEDIndicatorColours (dsFPDColorConfig_t *indicatorColors, int* indicatorCount); |
There was a problem hiding this comment.
Supported colors are already part of dsFPDIndicatorConfig_t (dsFPDColorConfig_t * supportedColors; // Supported Colors of the FPD LEDs ). Do we need to have a separate API for this?
Signed-off-by: grandhi_santoshkumar <grandhi_santoshkumar@comcast.com>
Ulrond
left a comment
Ulrond
left a comment
There was a problem hiding this comment.
Need to review the why of this API changes.
No description provided.