Video streaming manager implementation #826

askirk · 2018-07-25T21:07:13Z

Partial fix to #782
This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

Unit tests in VideoStreamingManagerTests file, also via manual testing internally.

Summary

Adds described functionality of VideoStreamingManager from Android Manager API proposal.

- Implements methods outlined in evolution proposal - Adds method to get wiProVersion in ISdl, makes method public in SdlProxyBase - Enables implementation of VideoStreamingManager in SdlManager

- Also added some preliminary testing with Mockito

- Added listener in VideoStreamingManager to check that HMILevel is FULL before streaming - Remove listeners added in constructor with dispose() method

- Adding mocked responses to properly start a video stream - Includes setting whether haptic spatial data is supported in GENERAL_VIDEOSTREAMINGCAPABILITY

…om/smartdevicelink/sdl_android into feature/issue_782_video_streaming_manager # Conflicts: # sdl_android/src/main/java/com/smartdevicelink/api/SdlManager.java

- Added new StreamingStateMachine that better represents state of video/audio managers - Allows to support public methods that check state to match iOS - Added test for classic video streaming

This covers adding the methods used in sample apps - createOpenGLInputSurface - startEncoder - drainEncoder - releaseEncoder

- testClassicVideoStreaming test that was failing will now pass, and test getting a service and starting the encoder

…om/smartdevicelink/sdl_android into feature/issue_782_video_streaming_manager # Conflicts: # sdl_android/src/main/java/com/smartdevicelink/api/SdlManager.java

-Add test for convertTouchEvent -Make convertTouchEvent method protected - Remove encoder drain and release calls in attempt to fix travis failures

codecov-io · 2018-08-02T17:37:28Z

Codecov Report

Merging #826 into feature/issue_782_Android_Managers will increase coverage by 2.06%.
The diff coverage is 48.37%.

@@                           Coverage Diff                            @@
##             feature/issue_782_Android_Managers     #826      +/-   ##
========================================================================
+ Coverage                                  41.6%   43.67%   +2.06%     
- Complexity                                 3169     3182      +13     
========================================================================
  Files                                       394      395       +1     
  Lines                                     19556    18531    -1025     
  Branches                                   2112     1892     -220     
========================================================================
- Hits                                       8137     8094      -43     
+ Misses                                    11046    10025    -1021     
- Partials                                    373      412      +39

Impacted Files	Coverage Δ	Complexity Δ
.../main/java/com/smartdevicelink/api/SdlManager.java	`45% <0%> (-2.13%)`	`24 <0> (ø)`
...n/java/com/smartdevicelink/proxy/SdlProxyBase.java	`4.69% <20%> (+1.18%)`	`6 <4> (ø)`	⬇️
...com/smartdevicelink/api/StreamingStateMachine.java	`33.33% <33.33%> (ø)`	`6 <6> (?)`
...com/smartdevicelink/api/VideoStreamingManager.java	`55.29% <55.29%> (ø)`	`14 <14> (?)`
...martdevicelink/transport/SdlBroadcastReceiver.java	`3.43% <0%> (-1.21%)`	`2% <0%> (-1%)`
...martdevicelink/proxy/rpc/enums/StaticIconName.java
...celink/proxy/rpc/RegisterAppInterfaceResponse.java	`80% <0%> (+0.25%)`	`37% <0%> (-1%)`	⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78a3fb2...fe9a8db. Read the comment docs.

joeygrover · 2018-08-13T18:19:25Z

sdl_android/src/main/java/com/smartdevicelink/proxy/interfaces/ISdl.java

+     * Get wiProVersion
+     * @return byte representing wiProVersion
+     */
+    byte getWiProVersion();


We should not continue the misleading naming of the protocol layer. This method should be Version getProtocolVersion

joeygrover · 2018-08-13T18:25:28Z

sdl_android/src/main/java/com/smartdevicelink/api/SdlManager.java

@@ -141,28 +141,29 @@ protected void initialize(){
 		this.permissionManager = new PermissionManager(_internalInterface);
 		this.permissionManager.start(subManagerListener);

+		this.videoStreamingManager = new VideoStreamingManager(_internalInterface);
+		this.videoStreamingManager.start(subManagerListener);


I'm wondering if it makes sense to create the video streaming manager for every app. It seems as if the most responsible way to handle this would be to only create and start the manager when requested by the developer.

Or we could do it based on AppType. If its nav or proj, then we could start it. same w/ the audiostreammanager

Yea that's a good idea. Though I'm not sure if audio streaming is limited to those two app types, something need to check into

joeygrover · 2018-08-13T19:48:11Z

sdl_android/src/main/java/com/smartdevicelink/api/VideoStreamingManager.java

+	private static String TAG = "VideoStreamingManager";
+
+	private WeakReference<Context> context;
+	private volatile VirtualDisplayEncoder encoder;


Since there are two encoders of different types in this class, the names should be more specific to their types.

joeygrover · 2018-08-13T19:54:28Z

sdl_android/src/main/java/com/smartdevicelink/api/VideoStreamingManager.java

+	 * Starts the MediaCodec encoder utilized in conjunction with the Surface returned via the createOpenGLInputSurface method
+	 * @see #createOpenGLInputSurface(int, int, int, int, int, boolean)
+	 */
+	public void startEncoder(){


It seems that these are exposed to the developer and they are responsible for handling them. I would assume the manager would take care of this instead. I'm wondering if allowing this is even useful for developers since they could create an SdlRemoteDisplay that contains a surface view on which they could draw.

Should I just call this internally in createOpenGLInputSurface after creating the surface then?

As discussed offline, if we remove the createOpenGLInputSurface functionality for this release these methods can also be removed. We will instruct developers to create a remote display with a surface view for now.

joeygrover · 2018-08-13T19:59:54Z

sdl_android/src/main/java/com/smartdevicelink/api/VideoStreamingManager.java

+		}
+
+		@Override
+		public void onServiceError(SdlSession session, SessionType type, String reason) {}


Should transition to ERROR state

joeygrover · 2018-08-13T20:00:34Z

sdl_android/src/main/java/com/smartdevicelink/api/VideoStreamingManager.java

+	private final OnRPCNotificationListener hmiListener = new OnRPCNotificationListener() {
+		@Override
+		public void onNotified(RPCNotification notification) {
+			hmiLevel = ((OnHMIStatus)notification).getHmiLevel();


Potential NPE

joeygrover · 2018-08-13T20:01:40Z

sdl_android/src/main/java/com/smartdevicelink/api/VideoStreamingManager.java

+	 * @return IVideoStreamListener interface if service is opened successfully and streaming is
+	 *         started, null otherwise
+	 */
+	public IVideoStreamListener startVideoStream(VideoStreamingParameters parameters, boolean encrypted){


Is there a reason this is public? I believe this is better protected at least.

The naming between startVideoStream and startVideoStreaming is very confusing. We should provide clearer naming conventions here.

Will change to protected, make the other method startRemoteDisplayStreaming

I believe startVideoStream is more related to starting the video service so I think maybe it's better named according to that. startVideoService. Correct me if I'm wrong though.

joeygrover · 2018-08-13T20:03:26Z

sdl_android/src/main/java/com/smartdevicelink/api/VideoStreamingManager.java

+	/**
+	 * Stops streaming service and remote display encoder if applicable.
+	 */
+	public void stopStreaming(){


Is there a reason this is public?

To give a way to stop streaming, I suppose dispose does the same thing, so this could be private

Ok, no I think that is smart. If stopped is called, the manager has to be able to restart properly without being recreated and as long as that is possible a stopStreaming method would be useful for developers. I would add javadoc to this method that explains what steps (at a high level) would need to be done to restart the stream.

- Change getWiProVersion to getProtocolVersion with Version obj - Only create and start video manager if AppHMIType is Projection or Navigation

joeygrover · 2018-08-13T20:07:02Z

sdl_android/src/main/java/com/smartdevicelink/api/VideoStreamingManager.java

+	 * Check if a video service is currently active
+	 * @return boolean (true = active, false = inactive)
+	 */
+	public boolean isVideoConnected(){


I'm confused by the return here since it will return true even if the state is STOPPED. I think the naming could be better here as well, since this is already the video streaming manager we don't need to preface everything with video, could just be isStreaming or isActive but it will depend on what info it's intending to return.

Maybe I should change the name to isActive(). I intended this for this to give developer an idea of whether the video service was started / is running successfully

So there are a few different states and I think we just need to define what is important for the developer to know:

Video manager is ready to be used

Video service has been started

Streaming has started and is active

So what does active mean? Maybe we have different methods to define those states. I'm open for suggestions here.

joeygrover · 2018-08-13T20:07:42Z

sdl_android/src/main/java/com/smartdevicelink/api/VideoStreamingManager.java

+	 * Check if video streaming has been paused due to app moving to background
+	 * @return boolean (true = not paused, false = paused)
+	 */
+	public boolean isVideoStreamingPaused(){


See previous comment on naming; isPaused, isStreamingPaused, etc

Will change to isPaused

joeygrover · 2018-08-15T14:50:39Z

sdl_android/src/main/java/com/smartdevicelink/proxy/SdlProxyBase.java

@@ -1713,7 +1718,7 @@ private void dispatchIncomingMessage(ProtocolMessage message) {
 		}
 	}

-	private byte getWiProVersion() {
+	public byte getWiProVersion() {


This should be changed to protocol version as well

joeygrover · 2018-08-15T14:52:05Z

sdl_android/src/main/java/com/smartdevicelink/proxy/SdlProxyBase.java

@@ -354,6 +354,11 @@ public SdlMsgVersion getSdlMsgVersion(){
 			return null;
 		}

+		@Override
+		public com.smartdevicelink.util.Version getProtocolVersion() {
+			return new com.smartdevicelink.util.Version(Byte.toString(SdlProxyBase.this.getWiProVersion()) + ".0.0");


I believe we can access this information directly from wiproprotocol and should not just use the major byte as previously used. https://github.com/smartdevicelink/sdl_android/blob/master/sdl_android/src/main/java/com/smartdevicelink/protocol/WiProProtocol.java#L96

- eliminate manual surface method of video streaming and corresponding unit tests - change getWiProVersion() and setWiProVersion) methods in SdlProxyBase to use protocolVersion (Version vs. byte) - changed ambiguous naming in VideoStreamingManager- isPaused(), isStreaming(), isServiceActive() - added a way to stop and restart a video stream for dev

…om/smartdevicelink/sdl_android into feature/issue_782_video_streaming_manager # Conflicts: # sdl_android/src/main/java/com/smartdevicelink/api/SdlManager.java # sdl_android/src/main/java/com/smartdevicelink/proxy/SdlProxyBase.java

askirk added 13 commits July 23, 2018 17:25

Implement VideoStreamingManager APIs

6e63e5b

- Implements methods outlined in evolution proposal - Adds method to get wiProVersion in ISdl, makes method public in SdlProxyBase - Enables implementation of VideoStreamingManager in SdlManager

Fix setting of context, remoteDisplayClass

c93c0c8

- Also added some preliminary testing with Mockito

Add test for video streaming, monitor hmiLevel

442e121

- Added listener in VideoStreamingManager to check that HMILevel is FULL before streaming - Remove listeners added in constructor with dispose() method

Alter tests to successfully “start” a video stream

1b7d6bf

- Adding mocked responses to properly start a video stream - Includes setting whether haptic spatial data is supported in GENERAL_VIDEOSTREAMINGCAPABILITY

Merge branch 'feature/issue_782_Android_Managers' of https://github.c…

2d73036

…om/smartdevicelink/sdl_android into feature/issue_782_video_streaming_manager # Conflicts: # sdl_android/src/main/java/com/smartdevicelink/api/SdlManager.java

Add state machine & public methods to check state

b66a6d7

- Added new StreamingStateMachine that better represents state of video/audio managers - Allows to support public methods that check state to match iOS - Added test for classic video streaming

Add support for classic video streaming

a9c05b3

This covers adding the methods used in sample apps - createOpenGLInputSurface - startEncoder - drainEncoder - releaseEncoder

Fixing state machine transitions and surface test

66d66e1

- testClassicVideoStreaming test that was failing will now pass, and test getting a service and starting the encoder

Merge branch 'feature/issue_782_Android_Managers' of https://github.c…

787d478

…om/smartdevicelink/sdl_android into feature/issue_782_video_streaming_manager # Conflicts: # sdl_android/src/main/java/com/smartdevicelink/api/SdlManager.java

Add call to drain Encoder in tests

a21fc7f

Remove encoder related calls (travis) and add test

ead56eb

-Add test for convertTouchEvent -Make convertTouchEvent method protected - Remove encoder drain and release calls in attempt to fix travis failures

Remove static keyword from view in test

9799a3b

remove mock of View class

d0097ac

askirk changed the title ~~[WIP] Video streaming manager implementation~~ Video streaming manager implementation Aug 2, 2018

askirk requested review from BrettyWhite, bilal-alsharifi and joeygrover August 2, 2018 17:40

joeygrover reviewed Aug 13, 2018

View reviewed changes

Addressing review comments

f27133c

- Change getWiProVersion to getProtocolVersion with Version obj - Only create and start video manager if AppHMIType is Projection or Navigation

joeygrover reviewed Aug 13, 2018

View reviewed changes

joeygrover reviewed Aug 15, 2018

View reviewed changes

askirk added 2 commits August 15, 2018 15:49

joeygrover approved these changes Aug 23, 2018

View reviewed changes

joeygrover merged commit d5dcd03 into feature/issue_782_Android_Managers Aug 23, 2018

joeygrover deleted the feature/issue_782_video_streaming_manager branch August 23, 2018 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Video streaming manager implementation #826

Video streaming manager implementation #826

askirk commented Jul 25, 2018 •

edited

codecov-io commented Aug 2, 2018 •

edited

joeygrover Aug 13, 2018

joeygrover Aug 13, 2018

BrettyWhite Aug 13, 2018

joeygrover Aug 13, 2018

joeygrover Aug 13, 2018

joeygrover Aug 13, 2018

askirk Aug 14, 2018

joeygrover Aug 15, 2018

joeygrover Aug 13, 2018

joeygrover Aug 13, 2018

joeygrover Aug 13, 2018

joeygrover Aug 13, 2018

askirk Aug 14, 2018

joeygrover Aug 15, 2018

joeygrover Aug 13, 2018

askirk Aug 14, 2018

joeygrover Aug 15, 2018

joeygrover Aug 13, 2018

askirk Aug 13, 2018

joeygrover Aug 15, 2018

joeygrover Aug 13, 2018

askirk Aug 14, 2018

joeygrover Aug 15, 2018

joeygrover Aug 15, 2018

Video streaming manager implementation #826

Video streaming manager implementation #826

Conversation

askirk commented Jul 25, 2018 • edited

Risk

Testing Plan

Summary

codecov-io commented Aug 2, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askirk commented Jul 25, 2018 •

edited

codecov-io commented Aug 2, 2018 •

edited