[Accepted with Revisions] SDL 0109 - SetAudioStreamingIndicator RPC

[theresalech]

Hello SDL community,

The review of "SetAudioStreamingIndicator RPC" begins now and runs through November 14, 2017. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0109-set-audio-streaming-indicator.md

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

#334

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:

• Is the problem being addressed significant enough to warrant a change to SDL?
• Does this proposal fit well with the feel and direction of SDL?
• If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
• How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
  Please state explicitly whether you believe that the proposal should be accepted into SDL.

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
[email protected]

[Jack-Byrne]

If the purpose of this proposal is to ensure that the play / pause button is being displayed properly, then this new rpc is redundant since the HMI can use the setMediaClockTimer RPC to properly display the correct status of play or pause.

If it is desired to have more options for the possible state of playback (stopped, buffering, etc), then I would suggest adding the new enum to the onHMIStatus notification instead of creating a whole new RPC. Also onHMIStatus already has a parameter related to audio streaming (audioStreamingState) so defining playback level within this RPC would make sense and would avoid the need to define a new RPC.

[joeljfischer]

If it is desired to have more options for the possible state of playback (stopped, buffering, etc), then I would suggest adding the new enum to the onHMIStatus notification instead of creating a whole new RPC. Also onHMIStatus already has a parameter related to audio streaming (audioStreamingState) so defining playback level within this RPC would make sense and would avoid the need to define a new RPC.

The onHMIStatus and audioStreamingState go from the head unit to the phone and indicates head unit status, whereas this RPC needs to go from phone to head unit.

[joeljfischer]

I would like to see a few changes to this proposal based on Alternatives 2 and @JackLivio comments. First, I believe that the proposed solution may be missing a "buffering" element? Second, I don't believe this proposal really gives anything new with the audio indicator only being play/pause, play, pause, and stop. As @JackLivio noted, all of these can be done by the head unit observing the media clock timer's updateMode property. If the clock is running, it should be pause, if it's paused, it's play, if it's cleared, it's stopped.

Instead, I think alternative 2, but adding an audioIndicator property as in the proposal (but adding a buffering status which would be very useful to the user) would work very well. When the app sends the media clock timer it will update the status. This would keep us from adding a whole new RPC to the spec and instead fits it to an RPC that media app devs are familiar with. Most of the time they will update the clock when they update the status.

[ghost]

Just to be clear. In order to be able to indicate the action of the play/pause button SetMediaClockTimer.updateMode Is not an option (see alternative considered section). The head unit cannot clearly identify why the app has chosen to pause the media clock.

• Did the user pause the playback? Use „Play“ icon
• Is the app non-audible due to TTS (or similar)? Keep „Pause“ or „Stop“ icon (because it‘s an intermediate state)
• Are there issues in buffering audio from the internet? Keep „Pause“ or „Stop“ icon (it‘s also an intermediate state)

But we can use the RPC and add the audioStreamingIndicator parameter to it instead of having a new RPC. WIth regards to naming we should be careful audioStreamingIndicator Is referring to the audio stream (of the app). We might want to call it audioPlaybackIndicator but not audioIndicator („Audio“ only could be anything from volume to system tone, phone audio etc.).

Buffering is not an indicator but a playback status. This information should be provided by the playback status enum instead. An option would be to add this parameter as well but it‘s not important to actually solve the problem of not knowing the action of the play/pause button for a user.

[theresalech]

The Steering Committee voted to defer this proposal, keeping it in review until our next meeting on 2017-11-21, to allow the author more time to respond to the questions in the comments, and give members additional time to review.

[joeygrover]

I would suggest using the first alternative that has the app send AudioPlaybackStatusto the core module. I believe a headunit can infer how to handle the audio streaming indicator based on the playback status of the app. It could be possible to create guidelines for how the streaming indicator should behave based on the received playbacks status as well. For example, if the module receives PLAYING from the app, the module can ensure the audio streaming indicator displays the pause indicator as defined by the spec. This would also help create a consistent experience between apps and modules on how to handle the streaming indicator for situations like BUFFERING.

[joeljfischer]

For example, if the module receives PLAYING from the app, the module can ensure the audio streaming indicator displays the pause indicator as defined by the spec.

@joeygrover I think the issue with that is that the app may not want a pause button, but instead, a stop button to be displayed while PLAYING.

@kshala-ford I think BUFFERING could be an indicator, in that an indication should be displayed on the head unit. The buffering indicator could be within the play / pause button for example, but even if it is not, I think the play / pause button would be either "pause" or somehow disabled while buffering. So, it can be a sort of indication.

[ghost]

@joeygrover: @joeljfischer is right with Pause and Stop while PLAYING. The head unit would silently guess what icon to use. Whatever icon is shown on the button the head unit would still send an OK button press notification (it should be PLAY_PAUSE but that's another story). I'm afraid of scenarios where the indicator does not match the action if the Play/Pause button gets pressed.

We'll have the same problem with Pause and Stop when adding Buffering to the list. Again: It's a playback status; not an indicator. All three indicators (Play, Pause, Stop) would apply while buffering. Best practice on a button press while buffering should be to pause or stop the playback but some media players actually start to play whatever was buffered until that moment.

[joeljfischer]

@kshala-ford Okay, then my only requested change is to put the indicator struct you have defined into MediaClockTimer instead of a separate RPC. Can you think of a reason not to put it in MediaClockTimer? Buffering can be done using Show by the app itself.

[ghost]

<function name="SetMediaClockTimer" messagetype="request">
  <param name="audioStreamingIndicator" type="AudioStreamingIndicator" mandatory="false" />
   :
</function>

? I'm OK with that .

[joeljfischer]

Yep, that's what I'm thinking.

[theresalech]

The Steering Committee voted to accept this proposal with the revisions outlined in the last comment from kshala-ford:

<function name="SetMediaClockTimer" messagetype="request">
  <param name="audioStreamingIndicator" type="AudioStreamingIndicator" mandatory="false" />
   :
</function>

The Steering Committee also discussed the scenario when a head unit doesn't receive an update from the app, and it was determined that the head unit would fall back to old behavior in this case.

[theresalech]

@kshala-ford 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 the respective repositories for implementation. Thanks!

[theresalech]

@kshala-ford checking in on this. Can you please advise once PR with revisions has been entered? Thanks!

[theresalech]

Proposal has been updated to reflect agreed upon revisions, and issues have been entered:

Android
iOS
Core
RPC Spec
SDL HMI