Skip to content
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

feat: Add openrpc changes based on the requirements #254

Merged
merged 6 commits into from
Apr 16, 2024

Conversation

satlead
Copy link
Contributor

@satlead satlead commented Apr 5, 2024

No description provided.

"AudioOutput": {
"title": "AudioOutput",
"type": "string",
"enum": [

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these values need to be capitalized as per requirements doc?

"MONO",
"STEREO",
"SURROUND",
"SURROUND_7_1"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing SURROUND_5_1

"MimeType": {
"type": "string",
"description": "Enumeration of possible Mimetypes.",
"enum": [
Copy link

@alkalinecoffee alkalinecoffee Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do methods returning MIME types need to respond with the internal name (AUDIO_AC3) or value (audio/ac3)? I assume the latter but just checking.

},
{
"name": "videoFormatPossible",
"summary": "Get the possible video formats",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is a check, it does not return any video formats

},
{
"name": "audioFormatPossible",
"summary": "Check the possible video formats",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary is for video but this is an audio method

"params": [],
"result": {
"name": "height",
"summary": "Height of the display device in centimeters",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name and summary here are incorrect

"params": [],
"result": {
"name": "resolution",
"summary": "Height of the display device in centimeters",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The summary is incorrect; this returns a resolution

},
{
"name": "hdr",
"summary": "Returns an array of valid HDR profiles that the display supports",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this summary accurate? Its not just what the display supports, but what's supported given the supplied video resolution (if I'm understanding the requirements correctly).

@CLAassistant
Copy link

CLAassistant commented Apr 9, 2024

CLA assistant check
All committers have signed the CLA.

@alkalinecoffee alkalinecoffee force-pushed the feature/media-info-updates branch from 10fbf15 to 9dd77d2 Compare April 9, 2024 18:37
@satlead satlead marked this pull request as ready for review April 16, 2024 15:11
@satlead satlead merged commit 85c77a8 into feature/media-info Apr 16, 2024
3 of 4 checks passed
@satlead
Copy link
Contributor Author

satlead commented Apr 16, 2024

PR will be merged to the feature branch and work will progress in the main feature branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants