Skip to content

Conversation

@noah-livio
Copy link
Contributor

@noah-livio noah-livio commented Apr 5, 2022

Fixes #1803

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 (These are Android only changes)

Unit Tests

Ran Existing CI Tests

Core Tests

Tested on Samsung Galaxy S21 5G, Android 12 using sdl_test_suite_java

  1. Connect an Android SDL Navigation app to SDL Core with either sdl_hmi or generic_hmi
  2. Start streaming a video to the HMI
  3. Switch the video stream to a resolution lower than 320x240 (ex: 400x100)
  4. Attempt to stream at a supported resolution

Expected results: The low resolution video fails to play, an error is printed. Afterwards, the supported resolution plays successfully

Observed results: The low resolution video fails to play, an error is printed. Afterwards, the supported resolution plays successfully

Core version / branch / commit hash / module tested against: SDL Core release/8.1.0 cbc9d3928fb3c4a8db6608e84bad67d2398a52e7
HMI name / version / branch / commit hash / module tested against: SDL HMI release/5.7.0 cd2d250cba100d1e0dd354172d2e24c0bd3449fc

Summary

Adds a check in VirtualDisplayEncoder for unsupported resolutions that makes the virtual display encoder fail to start, prints a more helpful error message, and propagates an exception to the video stream manager. Changes in VideoStreamManager now kill the video stream service when the encoder fails to start, allowing for subsequent streams to start after an encoder error.

Changelog

Breaking Changes

None

Enhancements

None

Bug Fixes

Fixes #1803

CLA

Adds a check in VirtualDisplayEncoder for unsupported resolutions that makes the virtual display encoder fail to start, prints a more helpful error message, and propagates an exception to the video stream manager
Changes in VideoStreamManager now kill the video stream service when the encoder fails to start, allowing for subsequent streams to start after an encoder error
@codecov
Copy link

codecov bot commented Apr 5, 2022

Codecov Report

Merging #1804 (523e8da) into 5.4.0_RC (a624a06) will decrease coverage by 0.00%.
The diff coverage is 30.43%.

Impacted file tree graph

@@              Coverage Diff               @@
##             5.4.0_RC    #1804      +/-   ##
==============================================
- Coverage       54.05%   54.05%   -0.01%     
- Complexity       5519     5522       +3     
==============================================
  Files             562      562              
  Lines           25723    25742      +19     
  Branches         3372     3376       +4     
==============================================
+ Hits            13904    13914      +10     
- Misses          10562    10570       +8     
- Partials         1257     1258       +1     
Impacted Files Coverage Δ
...tdevicelink/managers/video/VideoStreamManager.java 22.09% <0.00%> (-0.31%) ⬇️
...smartdevicelink/encoder/VirtualDisplayEncoder.java 45.79% <58.33%> (+0.52%) ⬆️
...ink/managers/screen/BaseTextAndGraphicManager.java 64.58% <0.00%> (+0.41%) ⬆️
...rtdevicelink/streaming/video/SdlRemoteDisplay.java 52.43% <0.00%> (+2.43%) ⬆️

@noah-livio noah-livio marked this pull request as ready for review April 5, 2022 21:40
Copy link
Contributor

@JulianKast JulianKast left a comment

Choose a reason for hiding this comment

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

When sdl_hmi ask if you would like to start video streaming, you have to hit cancel if the stream is not supported, to be able to stream again. Will open issue to look into that for next release

@JulianKast JulianKast merged commit c163c17 into 5.4.0_RC Apr 11, 2022
@JulianKast JulianKast deleted the bugfix/issue_1803 branch April 11, 2022 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants