Implement SDL 0323: Align video streaming parameters with VideoStreamingCapability

[shiniwat]

Fixes #1569

This PR is ready for review.

Risk

This PR makes no 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, Java SE, and Java EE

Unit Tests

This PR does not include additional Unit Tests.

Core Tests

Based on SDL 0274 enabled Core and HMI, verified that video streaming works correctly.

Core version / branch / commit hash / module tested against: Core feature/issue-3243 branch; commit #3934b0d8804fc658ccca96f5ce558b9987255f4e.
HMI name / version / branch / commit hash / module tested against: Xevo custom HMI

Summary

Implement SDL 0323: Align vide streaming parameters with VideoStreamingCapability

Changelog

Breaking Changes

• No breaking changes.

Enhancements

• This implements issue [SDL 0323] - Align video streaming parameters with VideoStreamingCapability #1569.

Bug Fixes

• No bug fixes.

Tasks Remaining:

None

CLA

• I have signed the CLA

[codecov]

Codecov Report

Merging #1576 (9c6136a) into integration/stable_frame_rate (204318a) will decrease coverage by 2.34%.
The diff coverage is 59.88%.

@@                         Coverage Diff                         @@
##             integration/stable_frame_rate    #1576      +/-   ##
===================================================================
- Coverage                            56.22%   53.88%   -2.35%     
+ Complexity                            5034     5017      -17     
===================================================================
  Files                                  516      534      +18     
  Lines                                22386    23263     +877     
  Branches                              2797     2875      +78     
===================================================================
- Hits                                 12587    12535      -52     
- Misses                                8846     9687     +841     
- Partials                               953     1041      +88     

Impacted Files	Coverage Δ	Complexity Δ
...tdevicelink/managers/video/VideoStreamManager.java	31.48% <0.00%> (+0.18%)	21.00 <0.00> (ø)
...a/com/smartdevicelink/managers/BaseSdlManager.java	50.00% <ø> (ø)	37.00 <0.00> (ø)
...rs/screen/choiceset/PresentChoiceSetOperation.java	41.21% <0.00%> (-0.26%)	17.00 <0.00> (ø)
...com/smartdevicelink/proxy/rpc/ShowConstantTbt.java	100.00% <ø> (ø)	24.00 <0.00> (ø)
...elink/managers/lifecycle/BaseLifecycleManager.java	14.71% <16.66%> (-0.10%)	11.00 <1.00> (ø)
...link/streaming/video/VideoStreamingParameters.java	43.28% <21.73%> (-7.65%)	21.00 <14.00> (+2.00)	⬇️
...n/java/com/android/grafika/gles/WindowSurface.java	26.31% <26.31%> (ø)	1.00 <1.00> (?)
.../java/com/android/grafika/gles/EglSurfaceBase.java	32.14% <32.14%> (ø)	6.00 <6.00> (?)
...rc/main/java/com/android/grafika/gles/EglCore.java	43.96% <43.96%> (ø)	11.00 <11.00> (?)
...src/main/java/com/android/grafika/gles/GlUtil.java	44.92% <44.92%> (ø)	6.00 <6.00> (?)
... and 192 more

[RHenigan]

Hello @shiniwat,
I am in the process of reviewing this pull request along with PR #1389
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,
I have changed the base branch as per your request. Thanks.

[RHenigan]

Hello @shiniwat have these changed been tested with both RAW and RTP formats?
While testing I was able to confirm both formats work with PR #1389 but on this PR I am unable to stream RTP.
I believe this to be a result of removing the null check for the parameters from VideoStreamManager.startRemoteDisplayStream() when the update() method reaches the format check it returns when it finds a match between the formats thats the unit is capable of but this then has the potention to ignore the format the developer declares in the parameters. (i.e. developer declares RTP format for the stream but the manager tries to stream using RAW)

[shiniwat]

Hi @RHenigan, I understood your problem.
update(VideoStreamingCapability capability, String vehicleMake) method should NOT overwrite the format if the given format is in supported list.
I have made another commit at 1e2b923 to fix your case.
Thanks for pointing that out.

[RHenigan]

Hello @shiniwat, I noticed that there are changes included that impact the rpc_spec, these changes should not be included in this PR and should be reverted.

These changes seem to be included already as part of this PR https://github.com/smartdevicelink/rpc_spec/pull/229/files

[RHenigan]

Hello @shiniwat I have completed the review of #1389, can you please resolve the merge conflicts between this PR and the integration branch

[shiniwat]

Hi @RHenigan, sorry for the inconvenience.
I have resolved the VideoStreamManager.java, and made sure there's no other conflicts with the integration branch.
Regarding the commit 19e10a2 you mentioned, all I did at that time was to update the submodule commit hash (for rpc_spec submodule). But if you still think that specific commit should be reverted, I will do so.

[RHenigan]

Hello @shiniwat, thank you for resolving the merge conflicts, I added a few more comments.
In regards to the rpc_spec changes, that commit should still be reverted for this PR.

[RHenigan]

Hello @shiniwat, can you please revert the rpc_spec changes?

[shiniwat]

Hi @RHenigan, I have reverted the rpc_spec submodule as you requested.

[RHenigan]

Hello @shiniwat, per the proposal the update(VideoStreamingCapability capability, String vehicleMake) method in VideoStreamingParameters should be updated to take the lower value for bitrate and preferredFPS

public void update(VideoStreamingCapability capability, String vehicleMake) {
	...
    if (capability.getMaxBitrate() != null) {
        this.bitrate = Math.min(this.bitrate, capability.getMaxBitrate() * 1000);
    } // NOTE: the unit of maxBitrate in getSystemCapability is kbps.
	
	...
	if (capability.getPreferredFPS() != null) {
        this.frameRate = Math.min(this.frameRate, capability.getPreferredFPS());
    }
    ...
}

[shiniwat]

Hi @RHenigan, thanks for pointing that out. That code path was anyhow overlooked, and fixed now.