search

smartdevicelink / sdl_evolution

Tracking and proposing changes to SDL's public APIs.
https://smartdevicelink.github.io/sdl_evolution/
BSD 3-Clause "New" or "Revised" License
33 stars 122 forks source link

[Accepted with Revisions] SDL 0297 - Add function to transmit audio data of AudioStream in time division #988

Closed theresalech closed 3 years ago

theresalech commented 4 years ago

Hello SDL community,

The review of "SDL 0297 - Add function to transmit audio data of AudioStream in time division" begins now and runs through October 6, 2020. The original review of this proposal took place April 1 - 14, 2020. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0297-Add-function-to-transmit-audio-data.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

https://github.com/smartdevicelink/sdl_evolution/issues/988

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you, Theresa Lech

Program Manager - Livio theresa@livio.io

joeljfischer commented 4 years ago
  1. Is the code provided intended for a full code-review at this point? If that is the case, I would ask that this proposal be deferred until after the releases. If this is "sample code" to show that it works, it would be better to provide a PR with these changes so that we could test them without copy-pasting. Either way, I don't think that the current way of pasting this code into the proposal is helpful.
  2. Has this been tested on existing head units that support audio streaming or only your own HMI? Because this is a mobile-library only change, it will work with existing head units, but has it been determined that existing head units support this solution without flaw?
Yuki-Shoda-Nexty commented 4 years ago

@joeljfischer -san

  1. The codes provided in the proposal are sample codes.
  2. We have confirmed the operation with existing HU.
joeljfischer commented 4 years ago

2. Can I ask which existing head units?

Shohei-Kawano commented 4 years ago

@joeljfischer -san

  1. We used TOYOTA head unit.
joeljfischer commented 4 years ago

1. Because the "sample code" provided here is so extensive I would ask that this proposal be deferred until such a time that the iOS and Java_Suite teams are capable of reviewing it in more detail.

2. I think that before this could be accepted, it would have to be tested on all existing head units for potential regressions because it is such an aggressive change to video streaming. Could you provide an example fork or otherwise accessible code of the sdl_ios and sdl_java_suite repositories with this example code integrated and an example app that plays audio in this manner in order for OEMs to test this change with their head units?

theresalech commented 4 years ago

A quorum was not present during the 2020-04-07 Steering Committee Meeting, so this proposal will remain in review until our meeting on 2020-04-14.

Yuki-Shoda-Nexty commented 4 years ago

@joeljfischer -san 2.Regarding the sample application, it does not exist anymore because it was an old environment. However, we can provide to you the source code for iOS: Base version 6.3.1, Android: Base version 4.7.2 but how can I share it?

joeljfischer commented 4 years ago

I think the best way to share this would be to create a fork of the SDL_iOS and SDL_Java_Suite repositories, then to make the required changes for this proposal in the library so that everyone can verify the changes and build it into a sample app to test. Please use the latest master branch of the sdl_ios and sdl_java_suite libraries. You can then share a link to the fork. Because this proposal involves changing how data is sent and when, I believe it would be best for all SDLC members with hardware to then test the changes and ensure that their hardware properly handles it before voting to accept this proposal.

theresalech commented 4 years ago

The Steering Committee voted to defer this proposal. The author will follow the instructions provided by the Project Maintainer in this comment to build sample applications implementing the proposed feature. SDLC Members will then test the sample applications on their own hardware to ensure it will work with their existing head units. Once that testing is completed, this proposal can be brought back into review for Steering Committee vote.

theresalech commented 4 years ago

Steering Committee Representatives, please find information from the author regarding their Android sample application and request for assistance testing below. More information to follow regarding the iOS sample application.


We have prepared Proxy and Sample App to check “Add function to transmit audio data of AudioStream in time division”.

Please install Sample App or create an application using Proxy, and check if it works on the HU.

Proxy: https://github.com/zx5656/sdl_java_suite

Sample App: https://github.com/zx5656/sdl_video_streaming_android_sample

When SDL App starts, music will play.

Then tap the screen to play another music.

If the sound is played normally and if it is switched to another sound after tapping, it is operating normally.
theresalech commented 4 years ago

Steering Committee Representatives, below is the information from the author regarding testing the iOS sample application. As noted in the original Steering Committee decision for this proposal, SDLC Members should now test the sample applications on their own hardware to ensure it will work with their existing head units. Once that testing is completed, this proposal can be brought back into review for Steering Committee vote.

We have prepared Proxy and Sample App to check “Add function to transmit audio data of AudioStream in time division”.

Please install Sample App or create an application using Proxy, and check if it works on the HU.

Proxy: https://github.com/zx5656/sdl_ios

Sample App: https://github.com/zx5656/sdl_video_streaming_iOS_sample

When SDL App starts, music will play.

Then tap the screen to play another music.

If the sound is played normally and if it is switched to another sound after tapping, it is operating normally.
theresalech commented 4 years ago

Sample App feedback provided by Xevo team (@shiniwat):

theresalech commented 4 years ago

The author has updated both the iOS and Android sample applications to address the feedback received:

iOS: https://github.com/zx5656/sdl_video_streaming_iOS_sample

Android: https://github.com/zx5656/sdl_video_streaming_android_sample

theresalech commented 4 years ago

Updated Sample App feedback provided by Xevo team:

theresalech commented 4 years ago

Author has provided updated iOS Sample App for testing: https://github.com/zx5656/sdl_video_streaming_iOS_sample

theresalech commented 4 years ago

Closing as inactive. The issue will remain in a deferred status and unlocked so that SDLC members can comment with testing results when available. The proposal will be brought back into review once testing is completed.

shiniwat commented 3 years ago

I think we (Xevo team) have provided the feedback already, but here's the summary of our test results:

Just a minor concern is that sdl_java_suite library in the sample app is based on 4.11.1, while the latest release is 4.12.0. Likewise, iOS library in the same app is based on 6.6.0, while the latest release is 6.7.0. However it won't be a big deal for sdl_evolution process. So, as far as I have tested, I am OK to bring this issue back to in-review state.

Shohei-Kawano commented 3 years ago

@theresalech -san How many more members should test to move on to the next phase? I'm afraid that this will not be tested and will remain on hold forever. If there are members who can be tested, would you please make a request individually?

theresalech commented 3 years ago

@Shohei-Kawano, we understand your concern! I will bring the following to the Steering Committee during our next meeting (2020-09-22), so that we can move to bring this proposal back into review:

Request for SDLC members to complete testing by 2020-09-29. During the 2020-09-29 Steering Committee meeting, if there is no additional feedback or dissension to bringing the proposal back into review, we will do so. The proposal will then be voted upon during the 2020-10-06 Steering Committee meeting.

mrapitis commented 3 years ago

Please find the summary below of the Ford team's test results:

iOS sample app - video and audio streaming are working well, the audio playback is fluent. Android sample app - video and audio streaming are working well, the audio playback is also fluent.

We share the minor concern that Xevo has raised regarding testing with the latest iOS and JavaSuite libraries, however we also agree that this should not be a blocking issue for an evolution proposal. From our perspective the issue can be brought back into a review state.

theresalech commented 3 years ago

As SDLC members have tested the sample apps provided by the author, and provided feedback on this review issue, they have now voted to bring this proposal back into review. This proposal will remain in review until 2020-10-06.

kshala-ford commented 3 years ago

I am unsure if I could evaluate the android implementation but the iOS one is looking good so far.

Note: There is a big overlap with another proposal I am working on #1013 which is deprecating the audio streaming manager and replacing it with another manager supporting reliable input and output streaming. It's likely that your proposal will get accepted before mine which means I will need to make sure your concept is applied to the new manager as well if my proposal deprecates the current audio manager.

3. The proposal's iOS implementation contains static const NSInteger PerSecondVoiceData = 32000; which I would change to use pcmStreamCapability from the system capability manager. The audio streaming manager has no direct access to the capabilities to the capability manager. I would recommend changing the interface to the parent manager (audio lifecycle manager) adding access to the capability manager. In fact the audio converter is also using constant data and doesn't respect system capabilities. Something the PM should consider as a bug of the current library.

4. In my proposal I am referring to an issue I found with Ford IVIs which occurs if the system's input buffer empties and new data is received at the same time. There's a race condition where the system detects the input stream being empty, it'll shut down the prompt playback which takes a few milliseconds but if the app is sending audio at this short time slot it can happen that this audio data won't be played until the next audio data is send.

Let me try to explain with the below diagrams. The below diagram shows the happy path with the current behavior where new data is sent if the audio file (or in your case the chunk) is being played that results in a small time slot where the buffer is empty.

image

In the second diagram below I want to demonstrate the issue. The system got caught in the race condition, shutting down the audio playback but at the same time new data is being send. The system won't play the audio because it's not checking it's status again. After the third data transfer, the system recovers and ramps up the playback dequeuing now all data.

image

In the third diagram you see the attempt of my proposal preventing the buffer from being empty if the app has data in the pipeline.

image

I was able to reproduce the issue with the current library implementation. I assume your proposal won't change the behavior but cause more cases when the buffer becomes empty. I'm afraid the issue could occur more often.

Long story short: I just want to make you aware of this proposed change. My proposal will have impact to how the manager behaves to fill the system's audio buffer and it may have impact to your implementation. However your motivation and your concept won't be affected.

Akihiro-Miyazaki commented 3 years ago

@kshala-ford -san, I took over this popposal from @Yuki-Shoda-Nexty .

Thank you for your consideration and support. It was helpful for us. By the way, since these sample software which we provided are sample software for the functional confirmation, they do not suitable for mass production. Also, Since our proposal is the concept proposal, regarding to the implementation, we would like to leave to them who can implement very well. Thank you.

theresalech commented 3 years ago

In preparation for tonight's Steering Committee meeting, I'd like to propose accepting this proposal with revisions. It's our suggestion that the author revise to include references to the sample apps, and state that the code included in the proposal is sample code.

theresalech commented 3 years ago

The Steering Committee voted to accept this proposal with revisions. The author will revise the proposal to include references to the sample apps provided in the comments, and state that the code included in the proposal is sample code.

theresalech commented 3 years ago

@Akihiro-Miyazaki, please advise when a new PR has been entered to update the proposal to reflect the agreed upon revisions. I'll then merge the PR so the proposal is up to date, and enter issues in impacted repositories for implementation. Thanks!

theresalech commented 3 years ago

Proposal has been updated and implementation issues have been entered: