Fixed video/audio end services sometimes not being sent when secondary transport shuts down #1566

NicoleYarroch · 2020-02-21T17:04:11Z

Fixes #1551, #1561, #1471, #1479

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).

Unit Tests

Unit tests added/fixed for the streaming media manager classes.

Core Tests

Primary Transport Only Tests (Tested on SYNC 4.0 and SYNC 3.0)

Test putting app on device in background and then bringing it back to the foreground while video is streaming.
Test turning off ignition and restarting ignition while video is streaming.
Test turning off ignition and restarting ignition while device app is in background (i.e. app is open but not streaming video).
Test switching between two navigation apps (i.e. make sure video streaming resumes after the app's hmiLevel goes to BACKGROUND and back to FULL).
Test switching between the navigation and non-video streaming app ((i.e. make sure video streaming resumes after the app's hmiLevel goes to LIMITED and back to FULL).
Test disconnecting the transport while the device app is foregrounded and reconnecting transport when device app is foregrounded.
Test disconnecting the transport while the device app is foregrounded and reconnecting transport when device app is backgrounded.
Test disconnecting the transport while the device app is backgrounded and reconnecting transport when device app is foregrounded.
Test disconnecting the transport while the device app is backgrounded and reconnecting transport when device app is backgrounded.

Secondary Transport Tests (Tested on SYNC 4.0)

Test turning off Wifi on device and reconnecting to the system Wifi while video is streaming.
Test putting app on device in background and then bringing it back to the foreground while video is streaming.
Test turning off ignition and restarting ignition while video is streaming.
Test turning off ignition and restarting ignition while device app is in background (i.e. app is open but not streaming video).
Test switching between two navigation apps (i.e. make sure video streaming resumes after the app's hmiLevel goes to BACKGROUND and back to FULL).
Test switching between the navigation and non-video streaming app ((i.e. make sure video streaming resumes after the app's hmiLevel goes to LIMITED and back to FULL).
Test disconnecting the BT transport while the device app is foregrounded and reconnecting transport when device app is foregrounded.
Test disconnecting the BT transport while the device app is foregrounded and reconnecting transport when device app is backgrounded.
Test disconnecting the BT transport while the device app is backgrounded and reconnecting transport when device app is foregrounded.
Test disconnecting the BTtransport while the device app is backgrounded and reconnecting transport when device app is backgrounded.

Smoke tests performed with:
Core version / branch / commit hash / module tested against: SYNC 4 (20016_DEVTEST) and SYNC 3.0
HMI name / version / branch / commit hash / module tested against: SYNC 4 (20016_DEVTEST) and SYNC 3.0

Summary

Fixed race condition with ending a video and/or audio service on the secondary transport where a video and/or audio end service control frame was not sent before the secondary transport was destroyed.
Fixed the hmiLevel being reset when the secondary transport closed. This is not necessary as the primary transport is still open.
The streaming video and audio lifecycle managers were not reset correctly when transport between device and module was destroyed, so sometimes an end video service request was sent incorrectly when a new session opened.

Changelog

Bug Fixes

The pixel buffer pool in the SDLH264VideoEncoder class sometimes randomly invalidates itself when the device app is backgrounded. Handling was added to reset the pixel buffer pool when it becomes nil.
If the response to the video start service comes after the app has been put in the background, the app now transitions to the suspended state instead of attempting to send video frames.
Fixed a race condition causing the secondary transport to not always properly end the video / audio services.
The streaming video and audio lifecycle managers were not reset correctly when transport between device and module was destroyed, so sometimes an end video service request was sent incorrectly when a new session opened.

CLA

I have signed the CLA

Co-Authored-By: Joel Fischer <joeljfischer@gmail.com>

…transport' of https://github.com/smartdevicelink/sdl_ios into bugfix/issue_1551_video_end_service_not_sent_secondary_transport # Conflicts: # SmartDeviceLink/SDLSecondaryTransportManager.m

…ent_secondary_transport # Conflicts: # SmartDeviceLink/SDLLifecycleManager.m # SmartDeviceLink/SDLStreamingAudioLifecycleManager.m # SmartDeviceLink/SDLStreamingMediaManager.h # SmartDeviceLink/SDLStreamingMediaManager.m # SmartDeviceLinkTests/DevAPISpecs/SDLStreamingAudioLifecycleManagerSpec.m # SmartDeviceLinkTests/DevAPISpecs/SDLStreamingVideoLifecycleManagerSpec.m

SmartDeviceLink/SDLSecondaryTransportManager.m

joeljfischer · 2020-03-27T17:08:26Z

SmartDeviceLink/SDLTCPTransport.m

    [self sdl_cancelIOThread];

+    if (self.ioThread == nil) {


Would it ever be nil here? I don't think that it can be since the only place it's set to nil is below in the performSelector for sdl_disconnect, and we already checked for nil above.

I removed this code after testing. It does not appear to be necessary anymore.

joeljfischer · 2020-03-27T17:11:17Z

SmartDeviceLink/SDLTCPTransport.m

+    // The `ioThread` could not be woken up to be cancelled. Switch to the `ioThread` and perform the cleanup of the input/output streams
+    [self performSelector:@selector(sdl_disconnect) onThread:self.ioThread withObject:nil waitUntilDone:NO];


This doesn't seem correct. If the ioThread is cancelled, it's not automatically nil right? So this will always run?

I removed this code after testing. It does not appear to be necessary anymore.

SmartDeviceLinkTests/DevAPISpecs/SDLStreamingAudioLifecycleManagerSpec.m

joeljfischer · 2020-03-31T16:12:31Z

I did as much other testing as was possible. I had some issues with a Sync 3.4 Bluetooth setup where I was getting SetAppIcon rejected and ListFiles disallowed, so I was unable to do extensive testing of this particular case. All other cases seem to be working very well.

Co-Authored-By: Joel Fischer <joeljfischer@gmail.com>

SmartDeviceLink/SDLTCPTransport.m

NicoleYarroch added 30 commits January 30, 2020 09:53

video streaming manager stop method fixed

1fb8081

Added logs

2fabda5

Fixed nil pixel buffer pool

21c3fb6

Added log to car window

e4d0a25

Removed hmilevel check

0e1cd3f

Fixed documentation

630517c

Removed hmi level check

cbf99f7

Fixed pragma markers

92fd023

Remove setting pool to nil

6919c06

fixed video state for starting video when app is in bg

2174537

Added error handling to CarWindow

902e556

Added more detail to error messages

ba7627a

Cleaned up serviceType check

25ee756

Removed reset

cf15791

Fixed SDL log messages

53121ae

Refactored compression session resetting

13562fb

Cleaned up documentation

66138dd

Refactoring the video lifecycle manager

0780c1e

Fixed returned value

7ee19e2

Fixing audio state stop method

28a06ec

Added error message

df13cf6

Cleaned up audio stream manager

56e793a

Removed newlines

618464d

Added test cases for audio manager

8c272fa

Added test cases for stopping streaming

106c0cd

Fixed resetting the pixelBufferPool

484f193

connectedVehicleMake now reset on disconnect

e5b1543

Fix backgrounding breaking the compression session

7495445

TCP transport only stopped after video stops

af768ef

Added background task and fixes

d62883c

NicoleYarroch and others added 10 commits March 20, 2020 08:51

Made public variable private

245ba0c

Made public methods private

ac992e8

Apply suggestions from code review

631c9b4

Co-Authored-By: Joel Fischer <joeljfischer@gmail.com>

Refactored dispatch_group code

6636b87

Merge branch 'bugfix/issue_1551_video_end_service_not_sent_secondary_…

788b74d

…transport' of https://github.com/smartdevicelink/sdl_ios into bugfix/issue_1551_video_end_service_not_sent_secondary_transport # Conflicts: # SmartDeviceLink/SDLSecondaryTransportManager.m

Added documentation

e227bf4

Added documentation to stream managers

81b4c34

Streams now opened and closed on same thread

ef9adb3

Added weakSelf/strongSelf handling

244b540

NicoleYarroch requested a review from joeljfischer March 24, 2020 20:14

joeljfischer suggested changes Mar 31, 2020

View reviewed changes

NicoleYarroch and others added 4 commits March 31, 2020 12:55

Apply suggestions from code review

7b5c862

Co-Authored-By: Joel Fischer <joeljfischer@gmail.com>

Added documentation to method

9cfcbd2

Refactored cancel timer method

b77c02c

Removed unnecessary shutdown code

fa2aaaa

NicoleYarroch requested a review from joeljfischer March 31, 2020 20:13

joeljfischer suggested changes Apr 1, 2020

View reviewed changes

SmartDeviceLink/SDLTCPTransport.m Show resolved Hide resolved

SmartDeviceLink/SDLTCPTransport.m Show resolved Hide resolved

Added logs to TCPTransport start/stop funcs

6d0ca85

v6.6.0 automation moved this from Waiting for Review to Reviewer approved Apr 1, 2020

joeljfischer approved these changes Apr 1, 2020

View reviewed changes

joeljfischer merged commit 2038694 into develop Apr 1, 2020

v6.6.0 automation moved this from Reviewer approved to Done Apr 1, 2020

joeljfischer deleted the bugfix/issue_1551_video_end_service_not_sent_secondary_transport branch April 1, 2020 18:06

joeljfischer mentioned this pull request Apr 15, 2020

Fix HMI status is cleared #1528

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed video/audio end services sometimes not being sent when secondary transport shuts down #1566

Fixed video/audio end services sometimes not being sent when secondary transport shuts down #1566

NicoleYarroch commented Feb 21, 2020 •

edited

joeljfischer Mar 27, 2020

NicoleYarroch Mar 31, 2020

joeljfischer Mar 27, 2020

NicoleYarroch Mar 31, 2020

joeljfischer commented Mar 31, 2020

		// The `ioThread` could not be woken up to be cancelled. Switch to the `ioThread` and perform the cleanup of the input/output streams
		[self performSelector:@selector(sdl_disconnect) onThread:self.ioThread withObject:nil waitUntilDone:NO];

Fixed video/audio end services sometimes not being sent when secondary transport shuts down #1566

Fixed video/audio end services sometimes not being sent when secondary transport shuts down #1566

Conversation

NicoleYarroch commented Feb 21, 2020 • edited

Risk

Testing Plan

Unit Tests

Core Tests

Primary Transport Only Tests (Tested on SYNC 4.0 and SYNC 3.0)

Secondary Transport Tests (Tested on SYNC 4.0)

Summary

Changelog

Bug Fixes

CLA

joeljfischer Mar 27, 2020

Choose a reason for hiding this comment

NicoleYarroch Mar 31, 2020

Choose a reason for hiding this comment

joeljfischer Mar 27, 2020

Choose a reason for hiding this comment

NicoleYarroch Mar 31, 2020

Choose a reason for hiding this comment

joeljfischer commented Mar 31, 2020

NicoleYarroch commented Feb 21, 2020 •

edited