[Accepted] SDL 0292 - Improve VirtualDisplayEncoder for stable frame rate

[theresalech]

Hello SDL community,

The review of the revised proposal "SDL 0292 - Improve VirtualDisplayEncoder for stable frame rate" begins now and runs through April 28, 2020. The original review of this proposal took place March 17 - March 24, 2020. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0292-improve-VDE-for-stable-frame-rate.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/969

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 theresa@livio.io

[Sohei-Suzuki-Nexty]

A part of sdl_java_suite library utilizes VirtualDisplay and MediaEncoder to produce the video projection stream.

It says "A part of" What is the method of video streaming other than using VirtualDisplay?

[shiniwat]

A part of sdl_java_suite library utilizes VirtualDisplay and MediaEncoder to produce the video projection stream.

The sentence above states VirtualDisplay and MediaEncoder are a part of sdl_java_suite library, which should be true, but does not mention anything about available video streaming methods.

In fact, VirtualDisplay extends Presentation, and I believe many SDL NAVIGATION apps utilize VirtualDisplay. But some apps may use OpenGL surface directly for video projection (instead of using VirtualDisplay), which is totally up to user application. The point is that this proposal addresses the quality of video streaming that utilizes VirtualDisplay and MediaEncoder, but does not address other methods.

[joeygrover]

1. The proposal wants to include a new dependency of Grafika. However directly from the README of that project:

It's not stable. It's not polished or well tested. Expect the UI to be ugly and awkward. It's not intended as a demonstration of the proper way to do things. The code may handle edge cases poorly or not at all. Logging is often left enabled at a moderately verbose level. It's barely documented. It's not part of the Android Open Source Project. We cannot accept contributions to Grafika, even if you have an AOSP CLA on file. It's NOT AN OFFICIAL GOOGLE PRODUCT. It's just a bunch of stuff that got thrown together on company time and equipment. It's generally just not supported.

I don't see how we as the SDLC could accept bringing in a library that clearly states such poor status. For this reason alone the proposal should be rejected.

2. The code snippet is copied from the README of Grafika; has the author tried to actually implement this? How will the library code be altered to accept this?

3. Developers and the library should have a way to enable/disable this feature. Not all head units need to support a constant frame rate, and for bandwidth reasons should be allowed to prevent that excess in traffic.

[shiniwat]

1. I don't see how we as the SDLC could accept bringing in a library that clearly states such poor status.

The README certainly states the things you listed, but as far as I tested a part of the classes of Grafika this proposal refers to, they are rather solid and stable. I am not sure what the overall status of Grafika project, however. It is Apache 2 license, and I believe we don't have to import the whole Grafika project at all. I would like SDLC to evaluate the code quality rather than the caveat listed in the README.

2. The code snippet is copied from the README of Grafika; has the author tried to actually implement this? How will the library code be altered to accept this?

The code snippet is based on what I actually tried. I didn't alter the Grafika classes itself; instead, I tired to use some Grafika classes (Eglcore, OffscreenSurface, WindowSurface, etc) in VirtualDisplayEncoder, and verify how its concept works.

3. Developers and the library should have a way to enable/disable this feature. Not all head units need to support a constant frame rate

Well, I think stable frame rate is beneficial for all head unit, even if some head unit has high performance to accept variable frame rate. However, if some developer indeed wants to disable this feature, I will make this feature to allow developers to enable/disable.

[theresalech]

The Steering Committee voted to return this proposal for revisions. The author will revise the proposal to take the ideas that Grafika introduces instead of using Grafika itself. The revisions will include pointing out the specific files needed from Grafika and including unit tests and test cases around the new classes we're taking from Grafika that would be required for feature implementation. This would therefore allow us to implement those classes into the project and not create the Grafika dependency. The author will also revise the proposal to give developers the option to enable or disable the feature.

[theresalech]

The author has updated this proposal to incorporate the agreed upon revisions noted in this comment, and the revised proposal is now in review. The proposal will remain in review until 2020-04-28.

[joeygrover]

1. The author did not provide an alternative to using the aforementioned library. I don't believe we will come to an agreement during this review period. I would request an explicit vote from the SDLC to accepting a dependency with the warnings laid out above. I would advise against using any project with such warnings for a project that we are asking other developers to implement.

4.

Because this approach does not change an existing API,

While it doesn't change the RPC or Protocols spec, it does make API changes through the VideoStreamingParameters.stableFrameRate param and therefore this comment should be revised to reflect that.

5. I believe we need confirmation from OEM members that their implementations can not only support the stable/constant framerate (which should be fine), but also that their implementations do not suffer from performance degradation. In order to test this it might be best to implement some sample that OEMs can test on their systems.

[shiniwat]

1. The author did not provide an alternative to using the aforementioned library. I don't believe we will come to an agreement during this review period

As mentioned in the revised proposal, what we need is some wrapper classes of Android OpenGL ES classes. During the previous review period, we discussed the approach about implementing our own solution with the same ideas as some part of Grafica does. However, the helper classes of Grafica are just a thin wrapper of OpenGL ES stuff, which are exactly what we want to use. Because they are thin wrapper, I don't think they have quality concern. While we take the same idea that Grafica provides, the integration with our own Virtual Display Encoder is still unique to SDL. While we may be able to use plain vanilla OpenGL ES stuff directly without using the wrapper classes, it suffers the code readability. It is kind of open source philosophical question: suppose some external helper classes are very essential, is it still bad to use the helper classes for implementing our own solution?

4. While it doesn't change the RPC or Protocols spec, it does make API changes through the VideoStreamingParameters.stableFrameRate param and therefore this comment should be revised to reflect that.

The suggestion makes sense. Because "Impact on existing code" section already mentions about VideoStreamingParameters.stableFrameRate parameter, I will remove the phrase: "Because this approach does not change an existing API,".

5. I believe we need confirmation from OEM members that their implementations can not only support the stable/constant framerate (which should be fine), but also that their implementations do not suffer from performance degradation

I don't understand what would be the potential concern here. In general, constant/stable frame rate provides better user experience than variable frame rate. Because SDL-0274 allows OEM's head unit to specify the preferred FPS in VideoStreamingCapability, it is up to OEM to specify the preferred FPS. However, because SDL VirtualDisplayEncoder cannot provide stable frame rate, which prevents OEM from deciding the preferred FPS when running with Android mobile devices. This is exactly what this proposal is trying to solve. It makes sense to provide some test app for OEM to determine the preferred FPS, however.

[joeygrover]

1. Members must make a decision if they are willing to accept the terms and language used in the repo for such a wrapper or not. It is not wise to simply adopt open source code because it is convenient. The project maintainer is making sure that the members are aware of, understand, and can accept the possible technical debt that would be added into the project by creating a dependency with the outlined language. This is not a philosophical question, it is a business decision.

5. IVI systems that are already in production will not be able to specify their preferred FPS. If the feature is accepted and the library is updated, apps that have the updated library will still connect to older IVI systems that do not have the feature. Because of this, OEMs must be proactive to understand if this will have an impact on their systems or not.

[shiniwat]

1. This is not a philosophical question, it is a business decision.

Okay, if it is business decision, then I will follow the decision of steering committee. As mentioned, the alternative solution would be to directly use OpenGL ES classes in VirtualDisplayEncoder class instead of using helper (wrapper) classes.

5. IVI systems that are already in production will not be able to specify their preferred FPS.

Understood that you're referring to the existing IVI systems. Because VideoStreamingParameters class specifies default frame rate to 30, that default frame rate (=30) will be used if IVI system does not specify preferred FPS params. I am not very certain how the default frame rate is decided, but it might be a good chance to use the same default frame rate with iOS library, which is 15.

[theresalech]

During the Steering Committee meeting, the Project Maintainer outlined their concerns (item 1) with the proposed feature using helper (wrapper) classes from the Grafika library, which includes the following in its README:

It's not stable. It's not polished or well tested. Expect the UI to be ugly and awkward. It's not intended as a demonstration of the proper way to do things. The code may handle edge cases poorly or not at all. Logging is often left enabled at a moderately verbose level. It's barely documented. It's not part of the Android Open Source Project. We cannot accept contributions to Grafika, even if you have an AOSP CLA on file. It's NOT AN OFFICIAL GOOGLE PRODUCT. It's just a bunch of stuff that got thrown together on company time and equipment. It's generally just not supported.

The author acknowledged the concerns with Grafika, but defended that using the library's wrapper classes are useful for implementing the solution. However, they did agree to implement logic with OpenGL ES classes if the Steering Committee votes to go that route.

The PM and author also requested input from Steering Committee members on this proposal's impact on IVI systems already in production (item 5), and noted while they don't anticipate negative impacts, OEMs should be aware of this change.

The Steering Committee voted to keep this proposal in review, to consider the items in question (above) and provide their feedback, joining the next meeting prepared to vote.

[Sohei-Suzuki-Nexty]

1. Could you please tell me the pros and cons to each of the method using the Grafika library and using the OpenGL ES class directly?

[shiniwat]

Could you please tell me the pros and cons to each of the method using the Grafika library and using the OpenGL ES class directly?

I believe the Pros and Cons of Grafika library are already discussed here, but because you are asking:

• Pros of using Grafika The implementation indeed needs the some wrapper of OpenGL ES. As mentioned in the proposal, somer wrapper classes, like EglCore, OffscreenSurface, WindowSurface, FullFrameRect, are very straightforward, simplify the coding, and improve the code quality.

• Cons of Grafika As project maintainer mentioned, the README of Grafika implies "it's not stable". As I mentioned many times, however, Grafika is fairly large, and what we need are very specific part of Grafika. As far as I can tell, those wrapper classes do very primitive tasks, and there's no quality concern about them.

• Pros of directly using OpenGL ES Free from external dependency.

• Cons of directly using OpenGL ES That increases code complexity, and impair the code quality. As the nature of the programmer, abstraction is very essential part of being a good programmer, but using OpenGL directly is very against approach.

[theresalech]

The Steering Committee voted to keep this proposal in review, as they were not yet ready to vote on the proposal as it concerned the use of the Grafika library and its impact on current IVI implementations. It was understood that the options for vote in the next meeting would be to either accept the proposal, acknowledging the use of the Grafika library and that it won’t have a negative impact on current IVI implementations, or to return the proposal for revisions to update it to use OpenGL ES.

[Sohei-Suzuki-Nexty]

Grafika is fairly large, and what we need are very specific part of Grafika.

When you say use a special part of Grafika, do you mean to implement only the necessary parts of Grafika library? Or do you mean to implement the Grafika library and use only what you need? If it’s the latter case, I think there is a concern that the size will increase due to the extra implementation.

[shiniwat]

When you say use a special part of Grafika, do you mean to implement only the necessary parts of Grafika library? Or do you mean to implement the Grafika library and use only what you need?

The answer is already mentioned in the "detailed design" section of the proposal. Excerpt from detailed design:

... the following wrapper classes in Grafika already did exactly what we want:

EglCore (com.android.grafika.gles.EglCore) OffscreenSurface (com.android.grafika.gles.OffscreenSurface) WindowSurface (com.android.grafika.gles.WindowSurface) FullFrameRect (com.android.grafika.gles.FullFrameRect)

To be more accurate, what we need are classes under com.android.grafika.gles package only. Again, we don't have to reinvent a wheel; instead, we can reuse those classes, which is allowed by Apache-2 license.

[theresalech]

The Steering Committee acknowledged the risks of using the Grafika library as a dependency and that their current implementations are not negatively impacted by this feature, and voted to accept the proposal.

[theresalech]

Implementation issue entered and referenced above.