Fix stopping video without audio streaming

[AKalinich-Luxoft]

Fixes #3479

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

Reproduction Steps

1. App starts Video and Audio services and starts streaming Video
2. SDL sends requests to HMI:
   a) Navigation.StartStream
   b) Navigation.StartAudioStream
3. HMI responds to a) and doesn't respond to b)
4. SDL sends 3 more b) requests
5. HMI still doesn't respond

Expected Behavior

SDL does:

• stop Audio service
• not stop Video service
• continue Video streaming from App
• not unregister App with PROTOCOL_VIOLATION reason

Observed Behavior

SDL does:

• stop Video service
• stop Video streaming from App
• unregister App with PROTOCOL_VIOLATION reason if app continues streaming Video

Summary

Was added function EndService which stops streaming for only one service (audio or video) and checks the state of another service. In case if both services are stopped, SDL will unregister the application.

CLA

• I have signed the CLA

[AKalinich-Luxoft]

@theresalech this PR is ready for Livio review

[theresalech]

@AKalinich-Luxoft , thank you! We will include this in the upcoming release if time allows.

[jacobkeeler]

Video streaming is still interrupted if the Audio stream request is ignored. After stopping the audio stream, Core immediately closes the video stream as well and closes the application (but does not unregister it)

[theresalech]

Hi @AKalinich-Luxoft , can you please review and respond to Jacob's comment? We are looking to complete development for the Core 8.0 release soon, so will need to make progress on this PR quickly in order to ensure its inclusion in the release. Thank you!

[AKalinich-Luxoft]

Hi @AKalinich-Luxoft , can you please review and respond to Jacob's comment? We are looking to complete development for the Core 8.0 release soon, so will need to make progress on this PR quickly in order to ensure its inclusion in the release. Thank you!

@theresalech Luxoft is working on the fix and going to provide extra changes according to Livio comments above.

[jordynmackool]

Hi @AKalinich-Luxoft, Livio is going to begin Release candidate prep, starting on 2021-08-30. As mentioned in Theresa's comment above, we will need to make progress on this PR asap for it to be included in the upcoming release. Can you please provide timing on when Luxoft will have this PR ready for Livio review? Thanks!

[AKalinich-Luxoft]

Video streaming is still interrupted if the Audio stream request is ignored. After stopping the audio stream, Core immediately closes the video stream as well and closes the application (but does not unregister it)

@jacobkeeler we made some additional changes to resolve the problem you described as well as solve other issues.
The following changes were done:

• Stream stopping sequences for separate service types were isolated from each other (9df1dc8)
• Included fix https://github.com/smartdevicelink/sdl_core/pull/3763 (9df1dc8)
• Containers close_app_timer_pool_ and end_stream_timer_pool_ were replaced with a single one (9df1dc8)
• Added NaviServiceDescriptor struct to unify timers, services and app_id for navi_app_to_stop_ and navi_app_to_end_stream_ (9df1dc8)
• Replace a pair of bool flags in NaviServiceStatusMap with struct with more obvious field names (be18260)
• Do not bring the application to NONE in case a/v service start was rejected but there is still one more a/v service accepted (looks like this is the case you described above) - 481f8ae
• Do not unregister the application if it does not send EndServiceAck (make it optional), but stopped sending streaming data - 481f8ae. Please note that currently, the mobile library does not respond with EndServiceAck so the old SDL implementation was trying to unregister such application after the timeout. Probably, this also should be fixed in mobile libraries too.

Please let me know if you have any questions regarding these changes

[AKalinich-Luxoft]

Hi @AKalinich-Luxoft, Livio is going to begin Release candidate prep, starting on 2021-08-30. As mentioned in Theresa's comment above, we will need to make progress on this PR asap for it to be included in the upcoming release. Can you please provide timing on when Luxoft will have this PR ready for Livio review? Thanks!

@jordynmackool this PR is ready for Livio review

[jacobkeeler]

Do not unregister the application if it does not send EndServiceAck (make it optional), but stopped sending streaming data - 481f8ae. Please note that currently, the mobile library does not respond with EndServiceAck so the old SDL implementation was trying to unregister such application after the timeout. Probably, this also should be fixed in mobile libraries too.

Was this fixed in this commit? I don't recall seeing any PROTOCOL_VIOLATION unregistrations while testing this PR initially, or in recent memory on develop. If the mobile libraries are not sending EndServiceAck in any case, then I would've expected to see this issue in testing at some point, since this test case involves Core rejecting the audio service. I also assume you're referring to the SDL Java Suite in this case?

[AKalinich-Luxoft]

Do not unregister the application if it does not send EndServiceAck (make it optional), but stopped sending streaming data - 481f8ae. Please note that currently, the mobile library does not respond with EndServiceAck so the old SDL implementation was trying to unregister such application after the timeout. Probably, this also should be fixed in mobile libraries too.

Was this fixed in this commit? I don't recall seeing any PROTOCOL_VIOLATION unregistrations while testing this PR initially, or in recent memory on develop. If the mobile libraries are not sending EndServiceAck in any case, then I would've expected to see this issue in testing at some point, since this test case involves Core rejecting the audio service. I also assume you're referring to the SDL Java Suite in this case?

@jacobkeeler it is easy to reproduce on the current develop branch using the following steps:

1. SDL and HMI + deploy_server.sh are started
2. Navi application is registered and activated
3. Start video service and hit OK on HMI popup - HMI starts to stream video
4. Start audio service and do NOT click on HMI popup - SDL makes 3 more attempts and 1 second later unregisters application due to PROTOCOL_VIOLATION reason

See attached log file - SmartDeviceLinkCore_develop.zip

I used Ford SPT for the case above. I guess it is based on https://github.com/smartdevicelink/sdl_java_suite (yes, I mean this library). You can see that after sending EndService(10) back to the application it does not respond with EndServiceAck, so SDL kicks this application:

DEBUG [26 Aug 2021 21:01:17,008][139970585007872][Utils] /home/andrew/git/opensource/sdl_core/src/components/utils/src/timer.cc:214 Timer::TimerDelegate::threadMain: Timer has finished counting. Timeout (ms): 1000
TRACE [26 Aug 2021 21:01:17,008][139970585007872][ApplicationManager] /home/andrew/git/opensource/sdl_core/src/components/application_manager/src/application_manager_impl.cc:3943 ApplicationManagerImpl::CloseNaviApp: Enter
INFO  [26 Aug 2021 21:01:17,008][139970585007872][ApplicationManager] /home/andrew/git/opensource/sdl_core/src/components/application_manager/src/application_manager_impl.cc:3977 ApplicationManagerImpl::CloseNaviApp: App haven't answered for EndService. Unregister it.

The reaction of Ford SPT to EndService requests is simply to stop sending streaming data but do not send EndServiceAck.

To be more specific, this block of code is responsible for the app unregistration due to the mentioned reason and it was removed in the commit I mentioned above to solve that problem.

[jacobkeeler]

@AKalinich-Luxoft Hmmm, seems that it might have been fixed at some point in the sdl_java_suite, as I'm not able to reproduce the issue on develop with the steps you provided. The application is just closed, not unregistered. Perhaps it is only an SPT issue now?

Also, it seems that the following defect scripts fail because of the fact that app is no longer being unregistered for failing to respond:

FAILED	./test_scripts/Defects/6_1/3139_04_Stop_streaming_forcibly_app_does_not_send_EndServiceAck.lua	
FAILED	./test_scripts/Defects/7_0/3547_3_RetryStrategy_for_start_streaming_Audio_HMI_all_REJECTED.lua	
FAILED	./test_scripts/Defects/7_0/3547_4_RetryStrategy_for_start_streaming_Video_HMI_all_REJECTED.lua	
FAILED	./test_scripts/Defects/7_0/3547_5_RetryStrategy_for_start_streaming_Audio_HMI_no_response.lua	
FAILED	./test_scripts/Defects/7_0/3547_6_RetryStrategy_for_start_streaming_Video_HMI_no_response.lua	

Seems that #3139 might specifically include this behavior, as the test explicitly omits the EndServiceACK to trigger this unregistration. As for the #3547 tests, we could likely just have the app respond with EndServiceACK and remove the expectation for OnAppInterfaceUnregistered. That should cover both potential behaviors, and it doesn't look like the omission of EndServiceACK is intentional in those tests.

[dboltovskyi]

@jacobkeeler As for the scripts we have prepared a new PR recently: smartdevicelink/sdl_atf_test_scripts#2576. It includes new scripts and updates for existing ones.

Script for #3139 amended in a way App continues streaming and doesn't send EndServiceAck so the main goal of the script remains the same.

Scripts for #3547 changed in a way App is not unregistred since application doesn't send streaming data.

We think SDL has to forcibly unregister application only in case if app continue send streaming data when it's not allowed (e.g. A/V service hasn't started, HMI doesn't approve A/V streaming, app is in NONE hmi level etc.)