[SDL 0292] implement VirtualDisplayEncoder improvement for stable frame rate

[shiniwat]

Fixes #1361

This PR is ready for review.

Risk

This PR makes minor API changes.

Testing Plan

• I have verified that I have not introduced new warnings in this PR (or explain why below)
• I have run the unit tests with this PR
• I have tested this PR against Core and verified behavior (if applicable, if not applicable, explain why below).
• I have tested Android, (but not with Java SE, and Java EE)

Unit Tests

This PR includes an unit test for Grafika part

Core Tests

I have run my SDL VPM app to make sure VPM runs with consistent frame rate.

Core version / branch / commit hash / module tested against: Core 6.1 + preferredFPS integrated (see sdl_core PR at smartdevicelink/sdl_core#3437)
HMI name / version / branch / commit hash / module tested against: our own HMI with VPM capability

Summary

This implements [SDL 0292] Improve VirtualDisplayEncoder for stable frame rate.

Changelog

Breaking Changes

• N/A

Enhancements

• This PR improve the VPM quality.

Bug Fixes

• issue [SDL 0292] Improve VirtualDisplayEncoder for stable frame rate #1361

Tasks Remaining:

• N/A

CLA

• I have signed the CLA

[codecov]

Codecov Report

Merging #1389 (37427ef) into develop (def46af) will decrease coverage by 1.10%.
The diff coverage is 12.41%.

@@              Coverage Diff              @@
##             develop    #1389      +/-   ##
=============================================
- Coverage      56.09%   54.99%   -1.11%     
- Complexity      5021     5035      +14     
=============================================
  Files            515      523       +8     
  Lines          22297    22864     +567     
  Branches        2777     2824      +47     
=============================================
+ Hits           12508    12574      +66     
- Misses          8848     9328     +480     
- Partials         941      962      +21     

Impacted Files	Coverage Δ	Complexity Δ
...main/java/com/android/grafika/gles/Drawable2d.java	0.00% <0.00%> (ø)	0.00 <0.00> (?)
...n/java/com/android/grafika/gles/FullFrameRect.java	0.00% <0.00%> (ø)	0.00 <0.00> (?)
...src/main/java/com/android/grafika/gles/GlUtil.java	0.00% <0.00%> (ø)	0.00 <0.00> (?)
...ava/com/android/grafika/gles/Texture2dProgram.java	0.00% <0.00%> (ø)	0.00 <0.00> (?)
...n/java/com/android/grafika/gles/WindowSurface.java	0.00% <0.00%> (ø)	0.00 <0.00> (?)
...smartdevicelink/encoder/VirtualDisplayEncoder.java	18.18% <6.95%> (-9.66%)	8.00 <0.00> (ø)
...link/streaming/video/VideoStreamingParameters.java	45.23% <11.11%> (-5.69%)	20.00 <1.00> (+1.00)	⬇️
.../java/com/android/grafika/gles/EglSurfaceBase.java	21.42% <21.42%> (ø)	3.00 <3.00> (?)
...rc/main/java/com/android/grafika/gles/EglCore.java	37.06% <37.06%> (ø)	7.00 <7.00> (?)
...ava/com/android/grafika/gles/OffscreenSurface.java	60.00% <60.00%> (ø)	1.00 <1.00> (?)
... and 9 more

[RHenigan]

Hello @shiniwat would it be possible to share your VPM app to test this PR?

[RHenigan]

Hello @shiniwat during testing how were you able to confirm that the video stream ran at a consistent frame rate?

[shiniwat]

Hi @RHenigan,
I have confirmed the frame rate by paying attention to how often onOutputBufferAvailable callback gets called by MediaCodec. It's been done by adding some custom debug message in Proxy.
Because you asked for test VPM app, I am preparing the test app. I'll let you know the app later.

[shiniwat]

Hi @RHenigan, my apologies for slow update.
The test app for your request is available at https://github.com/shiniwat/HelloMap. Please let me know if you run into any issues for running the test app.

[RHenigan]

Hello @shiniwat Can you resolve the merge conflicts, some of the code cleanup going on as part of the 5.0 release is creating some minor conflicts.
VirtualDisplayEncoder and VideoStreamingParameters seem to have conflicts as a result of this code cleanup
VideoStreamingCapability however has been impacted by changes to make the setters chainable (please see issue #1421)

While testing I have been unable to validate the video is running at a consistent frame rate. Thank you for sharing the test app but I have trouble with it crashing when the stream starts.

[shiniwat]

Hi @RHenigan,
I have merged the current develop branch into my PR branch, so that conflict should have been fixed.
I have updated my HelloMap app as well, and confirmed that VPM works with our IVI system. Please let me know if you still run into crashes. Thanks.

[RHenigan]

Hello @shiniwat
Thanks again for your work on this PR! As we've identified SDL 0274 as a dependency for this feature, we will not be able to merge this PR for SDL 0292 until code contributions for SDL 0274 are also reviewed and merged. We've noticed that you've completed some development for SDL 0274 already, so would suggest raising the feature to the Steering Committee as priority for the SDL 2021 Roadmap.

[shiniwat]

Hi @RHenigan,
Thanks for your advise. I thought SDL 0274 was planned for SDL Core 7.0 before, but as I checked the project state at https://github.com/smartdevicelink/sdl_core/projects/17, it is not included in Core 7.0 scope. I will make sure it gets prioritized in 2021 roadmap.

[kboskin]

@shiniwat Hi! There was found a corner case (by @mrapitis) that if the user starts the video streaming with custom VideoStreamingParameters the getCapability(VIDEO_STREAMING) is ignored. I've made the suggestion to your code, you can see it here. Please, review the suggestions and let me know if there should be some additions

[shiniwat]

Hi @KostyaBoss, thanks for the suggestion.
I have reviewed your suggested changes, and have a couple of concerns:

1. You introduced an additional parameter to VideoStreamManager#startRemoteStream, which is basically a breaking change for existing apps.
2. You revealed the potential problematic case where VideoStreamingParameters is explicitly specified. In that case, as you mentioned, VideoStreamingCapability was ignored, but you suggest that VideoStreamingCapability should be respected and should be merged with given parameters. That sounds make sense to me, but probably needs some review from others.
3. It's a little bit unclear to me the difference between CapabilitiesMergeType.SOFT and CapabilitiesMergeType.HARD. Rather than letting developer choose the merge type, I think we can basically take the SOFT merge option for all cases.

[kboskin]

@shiniwat Hi! Thanks for your reply. The points 1-3 seem reasonable for us, could you please give more feedback on the point 2 issue when you have some? Thanks

[joeygrover]

@shiniwat I would advise against merging any of the suggested changes from that PR as they stand, these changes were not present in the evolution proposal and would need further review before being accepted and merged. This may cause undue work onto you if merged.

@KostyaBoss please note that the changes suggested create breaking changes as well as introducing public API changes, these types of changes must have proper review and follow the defined processes.

[shiniwat]

Hi @joeygrover,
Understood your suggestion. As you mentioned, we need another evolution process (i.e. revising the proposal, and then evolution review process) if we take the suggested change.

@KostyaBoss, please let me know the importance of your proposed changes. Sounds like merging VideoStreamingCapability is kind of edge case, right?

[kboskin]

@shiniwat I don't think that this is considered to be an edge case, but let me share the reproduction steps there:

• Set up on the HMI some custom parameters (scale 2 for e.x)
• Hardcode the videoStreaming parameters on the mobile side
• Call startRemoteDisplay with previously hardcoded params
• Observe the behaviour

AR

• The scale will be ignored same as all other parameters from the HMI

ER

• HMI params should not be ignored

I think that's the major scenario

[shiniwat]

Hi @KostyaBoss, Understood your point. Because we need to take care of not only preferredFPS, but also other VideoStreamingParameters to cope with VideoStreamingCapability, I believe that issue should be addressed in another proposal rather than revising this one (SDL 0292).
I also need to confirm this issue happens on iOS as well as on Android, right?

[kboskin]

@shiniwat Hi! I didn't check the IOS side but should work in the same way

[shiniwat]

I double-checked my PR at smartdevicelink/sdl_ios#1613, and just recalled the fact where there's no such VideoStreamingParameters in sdl_ios. Therefore, we don't have to worry about collisions between VideoStreamingParameters and VideoStreamingCapabilities in iOS, i.e. the issue should be specific to sdl_java_suite.
I am thinking about submitting a new proposal that addresses sdl_java_suite specific issue.

[shiniwat]

Sorry for not updating previous comment, but the issue raised by @KostyaBoss is discussed in another proposal at SDL 0323.
As you see in the proposal, we should take lower value approach for maxBitrate and preferredFPS. Current sdl_java_suite already takes care of aligning other parameters, e.g. scale and preferred resolutions are taken from capability, and we should NOT modify other parameters.
Please provide any comment to currently in-review sdl_evolution at smartdevicelink/sdl_evolution#1094. Thanks.

[shiniwat]

Because SDL 0323 (Aligning VideoStreamingParameters with VideoStreamingCapability) implementation heavily depends on this PR, I have added another commit (3adc0ea), which covers implementing SDL 0323.

[joeljfischer]

Hi @shiniwat, unfortunately, this PR cannot be reviewed until SDL-0274 (#1268) is implemented, but 0274 is dependent on 0323. Therefore, this PR still cannot be reviewed. Please revert your commit adding changes for 0323 and provide those in a separate PR for review. Thank you.

[shiniwat]

Understood. I have reverted the commit for SDL 0323 (3adc0ea). I will submit yet another PR to address SDL 0323.

[shiniwat]

Regarding SDL 0323, I have submitted another PR at #1576. Thanks.

[RHenigan]

Hello @shiniwat,
I am in the process of reviewing this pull request along with PR #1576
Because these PRs are dependant on each other I have created an integration branch to be able to test these PRs together before they are merged into develop.
Can you update this PR to point to the new branch integration/stable_frame_rate instead of develop?

[shiniwat]

Hi @RHenigan, according to your request, I have changed the base branch into integration/stable_frame_rate.
BTW, I have noticed that codecov/patch complains some lack of tests (for one of grafika classes). I am going to fix that failure. Thanks.

[RHenigan]

Hello @shiniwat, I do not believe we need to add unit tests for the added grafika classes and I will continue my review of this PR