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

Feature/0296 possibility to update video streaming capabilities #1434

Conversation

kboskin
Copy link
Contributor

@kboskin kboskin commented Aug 5, 2020

Fixes #1410

This PR is [ready] for review.

Related PR's

Core
HMI

Risk

This PR makes [minor] API changes.

Testing Plan

  • I have run the unit tests with this PR
  • I have tested this PR against Core and verified the behavior
  • I have tested Android

Unit Tests

[Describe the unit tests and behaviors added in this PR]

Summary

  • Add restart of video service on VIDEO_STREAMING notification
  • Implement view resize for streaming on new params
  • Implement additional state for StreamingStateMachine
  • Add callback for developers to provide streaming constraints

CLA

kboskin added 29 commits April 30, 2020 15:35
    - Fix Button capabilities logging
    - Improve lifecycle management and version retrieving for RAI
    - Fix NPE for responses if SDLVersion is passed as null
    - Fix Button capabilities logging
    - Improve lifecycle management and version retrieving for RAI
    - Fix NPE for responses if SDLVersion is passed as null
    - Hardcoded display size to 2k
    - Add video config fields
    - Fix bug with default bad scale
    - Add video config fields
    - Fix bug with default bad scale
    - Add service restart
    - Implement video streaming POC for android
    - Update onServiceEnded with proper handling
# Conflicts:
#	android/sdl_android/src/main/java/com/smartdevicelink/managers/video/VideoStreamManager.java
    - Remove unused logs
    - Remove dependency on start/stopVideoService
    - Move logic to VideoStreamManager
    - Extend with callback to give third-party devs control on view resize process
    - Implement new "Paused" state for state machine
    - Implement new "Paused" state for state machine
    - Implement developer-provided info about resolution
    - Updated according to latest specification
    - Update according to new proposal revision
    - Add new notification
    - Fix compilation error
    - Move mocks to make easier pending implementation
    - Fix logic error with unused ranges provided by developer
    - Provide builder for SupportedStreamingRange
    - Add share prefs for saving ip on edit
    - Add button to kill app and all it's components
…s_update

# Conflicts:
#	android/hello_sdl_android/src/main/java/com/sdl/hellosdlandroid/SdlService.java
#	android/sdl_android/src/main/java/com/smartdevicelink/encoder/VirtualDisplayEncoder.java
#	android/sdl_android/src/main/java/com/smartdevicelink/managers/SdlManager.java
#	android/sdl_android/src/main/java/com/smartdevicelink/streaming/video/SdlRemoteDisplay.java
#	base/src/main/java/com/smartdevicelink/proxy/SystemCapabilityManager.java
    - Fix bugs after develop merge
    - Fix tests
    - Fix Version
    - Rename according to proposal
    - Fix wrong diagonal crash
    - Add validation for inconsistent capabilities
    - Fix filtration algorithm
    - Fix edge case when notification contains only scale parameter
    - Align resolutions according to QA needs
    - Fix notifications flow
    - Fix "only scale in notification" wrong processing
    - Add caching of previous parameters
    - Refactor filtering to methods
    - Use methods to simplify code and decrease complexity
…streaming_capabilities

# Conflicts:
#	android/sdl_android/src/main/java/com/smartdevicelink/proxy/SdlProxyBase.java
     - Fix compilation errors on merge
@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #1434 (5e29005) into develop (1118f39) will decrease coverage by 0.27%.
The diff coverage is 28.14%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #1434      +/-   ##
=============================================
- Coverage      54.30%   54.03%   -0.28%     
- Complexity      5261     5298      +37     
=============================================
  Files            549      555       +6     
  Lines          24215    24496     +281     
  Branches        3034     3086      +52     
=============================================
+ Hits           13151    13237      +86     
- Misses          9931    10110     +179     
- Partials        1133     1149      +16     
Impacted Files Coverage Δ Complexity Δ
...rtdevicelink/streaming/video/SdlRemoteDisplay.java 51.85% <0.00%> (-9.02%) 5.00 <0.00> (ø)
...elink/managers/lifecycle/BaseLifecycleManager.java 14.28% <0.00%> (ø) 11.00 <0.00> (ø)
...tdevicelink/managers/video/VideoStreamManager.java 22.74% <1.82%> (-8.75%) 21.00 <0.00> (ø)
...link/streaming/video/VideoStreamingParameters.java 41.09% <15.38%> (-2.94%) 21.00 <0.00> (ø)
...managers/video/resolution/VideoStreamingRange.java 52.72% <52.72%> (ø) 13.00 <13.00> (?)
...a/com/smartdevicelink/proxy/rpc/AppCapability.java 58.33% <58.33%> (ø) 5.00 <5.00> (?)
...devicelink/proxy/rpc/VideoStreamingCapability.java 98.11% <95.00%> (+4.17%) 25.00 <5.00> (+6.00)
...managers/video/resolution/ImageResolutionKind.java 100.00% <100.00%> (ø) 2.00 <2.00> (?)
...vicelink/managers/video/resolution/Resolution.java 100.00% <100.00%> (ø) 3.00 <3.00> (?)
...com/smartdevicelink/protocol/enums/FunctionID.java 94.16% <100.00%> (+0.04%) 12.00 <0.00> (ø)
... and 12 more

Copy link
Contributor

@RHenigan RHenigan left a comment

Choose a reason for hiding this comment

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

Hello @KostyaBoss can you resolve merge conflicts as well

kboskin added 2 commits February 25, 2021 22:13
…streaming_capabilities

# Conflicts:
#	android/sdl_android/src/androidTest/java/com/smartdevicelink/test/TestValues.java
- Fix pr comments
@kboskin
Copy link
Contributor Author

kboskin commented Feb 25, 2021

@RHenigan I've processed the PR comments and merged the latest dev branch, could you please check if everything is fine?

- Fix pr comments
@kboskin
Copy link
Contributor Author

kboskin commented Mar 1, 2021

@RHenigan I've processed comments, please, review the PR. I've noticed there is an issue with the CI, could you please restart it?

- Fix pr comments
@kboskin
Copy link
Contributor Author

kboskin commented Mar 3, 2021

@RHenigan could you please check? I've applied the changes

Copy link
Contributor

@RHenigan RHenigan left a comment

Choose a reason for hiding this comment

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

Hello @KostyaBoss, there were a few items that came up while doing alignment testing with iOS

- Fix pr comments
@kboskin
Copy link
Contributor Author

kboskin commented Mar 4, 2021

@RHenigan Hi! I've processed the comments, please review

- Fix pr comments
- Add tests
- Add tests
- Fix npes
@kboskin
Copy link
Contributor Author

kboskin commented Mar 8, 2021

@RHenigan Hi! I've added missing tests and processed the comment.

During the implementation, I've found additional edge cases, (inRangeMethods of ImageResolution could throw NPEs) and fixed them. Could you please check the latest commit and see, if the fix is suitable?

@kboskin
Copy link
Contributor Author

kboskin commented Mar 8, 2021

@RHenigan I've checked the CI logs, and it seems it was unable to build the project. On the other hand, the lines, the CI is pointing to are not present in the PR and in the branch and the projects are building locally. Could you please check this and restart the CI?

@RHenigan
Copy link
Contributor

RHenigan commented Mar 8, 2021

Hello @KostyaBoss I believe this will be resolved when develop is merged into this branch and any errors are resolved, one of the other features added ISdl in the SdlManager, this overrides startVideoService which is missing the parameter that was added.

If you merge develop and correct any conflicts/errors this should fix the CI

kboskin added 2 commits March 8, 2021 23:06
@kboskin
Copy link
Contributor Author

kboskin commented Mar 8, 2021

@RHenigan Thanks for the suggestion. I've tried this, but unfortunately, it didn't make a trick

The CI says, that the project is not compilable, but It compiles locally. Additionally, it states that

 /Users/runner/work/sdl_java_suite/sdl_java_suite/javaSE/javaSE/src/main/java/com/smartdevicelink/managers/SdlManager.java:225: error: method startVideoService in interface ISdl cannot be applied to given types;
            lifecycleManager.getInternalInterface(SdlManager.this).startVideoService(parameters,encrypted);

but, in line 225 there is JavaDoc only

Could you please check?

@RHenigan
Copy link
Contributor

RHenigan commented Mar 8, 2021

Hello @KostyaBoss thank you for merging develop in, it looks like the CI is still failing because the SdlManager in the JavaSE and JavaEE projects needs to be updated as well

- Fix PR comments
@kboskin
Copy link
Contributor Author

kboskin commented Mar 8, 2021

@RHenigan Thanks for your help, it seems, now everything is fine, could you please check?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
manager-streaming-video Relating to the manager layer - video streaming proposal Accepted SDL Evolution Proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants