search

smartdevicelink / sdl_evolution

Tracking and proposing changes to SDL's public APIs.
https://smartdevicelink.github.io/sdl_evolution/
BSD 3-Clause "New" or "Revised" License
33 stars 122 forks source link

[Accepted with Revisions] SDL 0332 - Additional Video Streaming Capabilities Validation #1139

Closed theresalech closed 3 years ago

theresalech commented 3 years ago

Hello SDL community,

The review of "SDL 0332 - Additional Video Streaming Capabilities Validation" begins now and runs through May 18, 2021. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0332-additional-video-streaming-capabilities-validation.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

https://github.com/smartdevicelink/sdl_evolution/issues/1139

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you, Theresa Lech

Program Manager - Livio theresa@livio.io

joeljfischer commented 3 years ago

1. In "Validation of HMI Notification OnSystemCapabilityUpdated" you state:

No duplicate capabilities should be included in the set of the root capability and the additional capabilities.

I believe that you need to define how to determine what is a duplicate and what is not. Non-screen-size related parameters should not count toward duplicate types, such as preferredFPS, maxBitrate, supportedFormats, or hapticSpatialDataSupported. In other words, if two capabilities are the same except for preferredFPS, they should still be considered duplicates.

2. In "Validation of RPC OnAppCapabilityUpdated"

No root level capability should be included in the OnAppCapability notification. All supported resolutions should be included in the parameter additionalStreamingCapabilities.

I think this is a little vague. I think you mean that the OnAppCapabilityUpdated.appCapability.videoStreamingCapability should only include all of its data in .additionalVideoStreamingCapabilities, and not that the OnAppCapabilityUpdated should not include the root capability from the HMI's video streaming capabilities. I think providing an example response for the app capability might be helpful here.

Jack-Byrne commented 3 years ago

1.

@joeljfischer Could you please elaborate on this?

In other words, if two capabilities are the same except for preferredFPS, they should still be considered duplicates.

Why should preferredFPS and other non screen size capabilities not be counted as a unique to the capability?

  1. Thank you for the suggestion, I can update the proposal to be more specific in regards to the parameters I am referencing.
joeljfischer commented 3 years ago

1. My concern is for an HMI developer creating an identical screen size (combining all the relevant factors), but providing multiple preferredFPS, maxBitrate, etc. and those being considered by Core to not be duplicates. Each given screen size should provide one and only one preferredFPS, maxBitrate, etc.

Jack-Byrne commented 3 years ago
  1. Thank you. I would like to add this as a validation requirement to the proposal.

A unique screen size is determined by the following parameters: preferredResolution, diagonalScreenSize, pixelPerInch, and scale. Each given screen size should provide one and only one preferredFPS, maxBitrate, supportedFormats, and hapticSpatialDataSupported.

jordynmackool commented 3 years ago

Summarized agreed upon items for revision as discussed in the comments above:

  1. Add the following validation requirement "A unique screen size is determined by the following parameters: preferredResolution, diagonalScreenSize, pixelPerInch, and scale. Each given screen size should provide one and only one preferredFPS, maxBitrate, supportedFormats, and hapticSpatialDataSupported.
  2. Add example response and parameter details for OnAppCapability.
theresalech commented 3 years ago

The Steering Committee voted to accept this proposal with the revisions summarized in this comment.

theresalech commented 3 years ago

@JackLivio please advise when a new PR has been entered to update the proposal to reflect the agreed upon revisions. I'll then merge the PR so the proposal is up to date, and enter issues in impacted repositories for implementation. Thanks!

theresalech commented 3 years ago

Author has updated proposal, and implementation issues have been entered: