[Accepted with Revisions] SDL 0274 - Add preferred FPS to VideoStreamingCapability

[theresalech]

Hello SDL community,

The review of "SDL 0274 - Add preferred FPS to VideoStreamingCapability" begins now and runs through January 21, 2020. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0274-add-preferred-FPS.md

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

#913

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:

• Is the problem being addressed significant enough to warrant a change to SDL?
• Does this proposal fit well with the feel and direction of SDL?
• If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
• How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
  Please state explicitly whether you believe that the proposal should be accepted into SDL.

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

[kshala-ford]

Thank you for the proposal. I believe that it is a valid extension to the SDL API allowing head units to avoid excessive stress to the hardware. I have a few comments to the proposal:

1. Please note that for sdl_ios there are two places where the FPS is configured. You already described the setting for the video encoder. Please have a look at https://github.com/smartdevicelink/sdl_ios/blob/6.5.0/SmartDeviceLink/SDLStreamingVideoLifecycleManager.m#L449 where a frame rate is configured to a display link. This method allows syncing with the phones display and the proposal should note that both, the encoder and the display link, should take the proposed parameter into account.
2. Please note that in practice the frame rate is rather low on iOS and sometimes it can be around or even lower than 15 fps... This can be due to the app inefficient capturing methods or the phones limited hardware capabilities. With that said, configuring a higher expected frame rate than the phone is capable of, I believe it may cause a degraded image quality as the video encoder tries to meet the settings with the cost of encoding quality. I would suggest that the proposed parameter is defined as the head unit's preferred FPS but allowing the phone to operate on a lower frame rate if the phone's hardware is constrained.
3. In the mobile API you propose the maxvalue of 2^31-1. I would suggest to specify a lower maxvalue. 60 might be a more practical value but quite restrictive though. Alternatively 144 could make sense as more modern displays support this refresh rate.

[joeljfischer]

1. I think he does account for this when he states:

videoEncoderSettings used in SDLStreamingVideoLifecycleManager,
capture rate of CADisplayLink#preferredFramePerSecond (available in iOS10 or higher)

2. This is a significant issue. If the head unit returns 30 fps to an iOS device, this can have wildly different experiences from bad to completely unusable while pegging the CPU to 100% the entire time. I agree with Kujtim's possible solution here.

3. I don't see a huge issue with the maxvalue. In practice, setting this above 144 may never happen, but it's good to not need to modify the parameter in the future.

4. The developer can also set their preferred frame rate. How should this mix with the developer's custom frame rate?

[joeygrover]

5. Javascript Suite must be added as an affected platform as it has to add the RPC changes to be in compliance with the RPC spec that introduces these additions.

[shiniwat]

1. I think he does account for this when he states:

videoEncoderSettings used in SDLStreamingVideoLifecycleManager,
capture rate of CADisplayLink#preferredFramePerSecond (available in iOS10 or higher)

Exactly right. sdl_ios has the ability to specify expected frame rate in videoEncoderSettings.
We can specify the value when preferred FPS is specified by head unit.

2. I would suggest that the proposed parameter is defined as the head unit's preferred FPS but allowing the phone to operate on a lower frame rate if the phone's hardware is constrained.

Understood the concern. How about specifying MOBILE_API's description as follows:

<description>Preferred frame rate per second specified by head unit. Mobile application should take this value into account for capturing and encoding video frame. While preferredFPS is specified by head unit, mobile application can take account of the case where mobile hardware is constrained</description>

3. While I understand the @kshala-ford 's suggestion, preferredFPS value would be lower or equal to 60 in practice. I believe maxvalue here does not matter.

4. The developer can also set their preferred frame rate. How should this mix with the developer's custom frame rate?

I don't understand what the developer's custom frame rate would be. Is that the value specified by head unit or is it the custom value in mobile app?

5. Javascript Suite must be added as an affected platform

Understood. I probably have to add Javascript Suite as the affected platform and add something to the detailed design section.

[joeljfischer]

4. It's a custom value specified in the app and sent to the library via SDLStreamingMediaConfiguration.customVideoEncoderSettings

[shiniwat]

4. It's a custom value specified in the app and sent to the library via SDLStreamingMediaConfiguration.customVideoEncoderSettings

Thanks for the update. So far I have no good idea, but I will investigate if developer can mix this value with preferredFPS value.

[kshala-ford]

First I want to apologize that I missed the statement to the display link. I'm afraid I missed this piece reviewing the proposal.

Regarding 4. Not sure if this is too easy approach but I believe that the smallest preferred framerate should be used. The requirement overall sounds that everyone wants the framerate not higher than specified. Example: If the developers specifies 30 as the framerate but the head unit returns 24 fps then 24 should be used. Still the ultimate decision comes to the phone hardware performance. Let's say if the phone isn't able to perform more then 15 frames then there's nothing we can do about it than accept the fact.

[shiniwat]

4. Regarding 4. Not sure if this is too easy approach but I believe that the smallest preferred framerate should be used.

I think this makes sense. Thanks for the suggestion, @kshala-ford.

[theresalech]

The Steering Committee voted to keep this proposal in review, to allow the author time to investigate the suggested changes and respond to the comments on the review issue.

[joeljfischer]

I'm okay with that.

[shiniwat]

5. Javascript Suite must be added as an affected platform

Regarding JavaScript Suite, as I look into VideoStreamingParameter.js, the constructor takes the frameRate as the parameter. Because update function takes VideoStreamingCapability, we can take preferredFPS into account in that (update) function.
Having said though, I am not certain how JavaScript suite actually encodes/sends the video streaming --- is video streaming part implemented?? So, while I understand JavaScript suite needs to be added as affected platform, I am not very certain what things this proposal need to add.

[kshala-ford]

5. There's no video encoding or streaming with JavaScript Suite. The only change would be to add the new param as per the proposed RPC change.

[shiniwat]

There's no video encoding or streaming with JavaScript Suite.

@kshala-ford, thanks for the comments, but let me confirm:
you meant "there's no video encoding/streaming with JavaScript suite right now, but the feature will be added in the future", right?

If JavaScript suite won't support video streaming even in the future, I don't think we have to take account of video streaming parameters in that platform.

[joeljfischer]

@shiniwat we can’t say definitively what features will or won’t be on a given platform in the future, but we can definitively say that all platforms must have access to all RPCs. That is a requirement of the SDL project.

[joeygrover]

Impacted Platforms: [Core / iOS / Java Suite / RPC]

Will need to be changed to:

Impacted Platforms: [Core / iOS / Java Suite / JavaScript Suite /RPC]

That's the only thing that is being requested. The Javascript library must contain all RPCs and any updates to them regardless of if it uses them or not and since the proposal is making an RPC change, the JavaScript library is affected.

[shiniwat]

Understood. @joeygrover, thanks for clarification.

[shiniwat]

Thanks all for the comments/suggestions. So far, here's what I need to do:

1. FPS configuration in sdl_ios:
   No action is required.

2. referredFPS parameter should take account of the case where phone's hardware is constrained:
   Need to explain about that in description of MOBILE_API.xml

3. maxValue of preferredFPS.
   I think current value (2^31-1) would be ok, even though it is much lower (like 60) in practice.

4. Take account of SDLStreamingMediaConfiguration.customVideoEncoderSettings
   Need to take care of the value, and take the smallest frame rate value approach.

5. Impacted platform
   Need to change to
   Impacted Platforms: [Core / iOS / Java Suite / JavaScript Suite /RPC]

[theresalech]

The Steering Committee voted to accept this proposal with revisions. The revisions will include the items listed in this comment from the author.

[theresalech]

@shiniwat 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 the respective repositories for implementation. Thanks!

[theresalech]

Author has updated proposal to reflect agreed upon revisions, and implementation issues have been entered:

• RPC
• Core
• iOS
• Java Suite
• JavaScript Suite

[theresalech]

HMI Issues:

• SDL HMI
• Generic HMI