smartdevicelink/sdl_evolution

[Accepted] Revise SDL-0296 Possibility to update video streaming capabilities during ignition cycle

jordynmackool opened this issue · 9 comments

jordynmackool commented

Hello SDL community,

The review of "Revise SDL-0296 Possibility to update video streaming capabilities during ignition cycle" begins now and continues through March 02, 2021.

This will be a review of proposed revisions to a previously accepted but not yet implemented proposal, SDL 0296.

The pull request outlining the revisions under review is available here:

#1124

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

#1125

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,
Jordyn Mackool

Program Manager - Livio
jordyn@livio.io

shiniwat commented

  1. If I understand the proposal correctly, it changes some existing APIs in VideoStreamManager and introduces new class VideoStreamingRange, so this should be a breaking change. There must be something in "Potential downside" and "Impact on existing code" sections. Author should explain about them.

  2. "Proposed solution" section states the author introduced VideoStreamingRange in order to align with iOS libraries. However AFAIK, there's no such class in iOS libraries (even in current develop branch). The proposal does not mention about impacted Platforms. Is author going to add the new class to both iOS and Android libraries?

joeljfischer commented

@shiniwat I think that both points can be answered by noting that this is a revision to an already accepted but not yet implemented proposal as noted in the title and initial post.

shiniwat commented

Hi @joeljfischer, Thanks for pointing that out. I found VideoStreamingRange in https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0296-Update-video-streaming-capabilities-during-ignition-cycle.md.

I know the revised proposal comes from PR review at smartdevicelink/sdl_java_suite#1434, and that's why the proposal appears to be focusing on sdl_java_suite only. iOS PR seems to be in review at smartdevicelink/sdl_ios#1756, so It's tough to identify if it is indeed aligned with iOS libraries without understanding the details in iOS changes.

joeljfischer commented

Hi @shiniwat, the reviewers of both the iOS PR and the reviewer of the Java Suite PR worked on this revision together (they are the new co-authors on the proposal) and are confident that this proposal will bring the PRs into alignment.

However, you should not need to understand the details of the iOS changes to see that this will assist alignment. If you go to the PR changes tab, you should be able to note that the public API changes have been brought into alignment. They moved methods into classes that are consistent for the two libraries, updated a method in the Java Suite library to allow for limiting portrait and/or landscape ranges (in a slightly different API but consistent functionality with iOS), etc. I don't think that you should need to look through the iOS / Java Suite in-progress library PRs to check this proposal revision.

shiniwat commented

Hi @joeljfischer, thanks for explaining the progress of PR reviews. Because the proposal says,

/// Removed API on the VideoStreamManager
public void startRemoteDisplayStream(Context context, Class<? extends SdlRemoteDisplay> remoteDisplayClass, VideoStreamingParameters parameters, final boolean encrypted, VideoStreamingRange streamingRange)

/// New API on the VideoStreamManager
public void startRemoteDisplayStream(Context context, Class<? extends SdlRemoteDisplay> remoteDisplayClass, VideoStreamingParameters parameters, final boolean encrypted, VideoStreamingRange supportedLandscapeStreamingRange, VideoStreamingRange supportedPortraitStreamingRange)

but I don't think both APIs exist right now, and the in-progress PR actually leaves the existing APIs remain for backward compatibility. So I am wondering if the proposal indeed up-to-date with current PRs.
Anyway, it sounds good that I don't have to look through in-progress PRs :)

joeljfischer commented

The proposal may not be up-to-date with current PRs. The "removed API" does not exist outside of this proposal.

shiniwat commented

Well, I was wondering why the revised proposal focuses on sdl_java_suite implementation, while both sdl_ios and sdl_java_suite PRs are still in review. However, @joeljfischer mentioned both sdl_java_suite and sdl_ios reviewers are involved as co-authors and public APIs seem to be consistent between two libraries, I have no objection to go ahead with the revised proposal.

jordynmackool commented

The Steering Committee voted on 2021-03-02 to accept the revised proposal.

jordynmackool commented

The proposal has been updated based on the Steering Committee's agreed-upon revisions. Comments have been left on implementation issues to reference revisions: