Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suspend and resume instead of restart video stream #980

Conversation

ghost
Copy link

@ghost ghost commented May 24, 2018

Fixes #979

This PR is ready for review.

Risk

This PR makes minor API changes:

  • New video stream state Suspended added. It'll occur if the stream was ready and the app entered inactive state
  • Property shouldRestartVideoStream is deprecated as not used anymore and can be deleted with the next major version.

Testing Plan

There was a unit test if the app enters inactive state. The test was changed to match the new state.

Enhancements
  • Resuming a suspended video stream is faster (<1s) than restarting it (>1s)

CLA

…deo stream is ready and the app enters inactive state.
@joeljfischer joeljfischer added the bug A defect in the library label May 24, 2018
@joeljfischer joeljfischer added this to the 6.0.0 milestone May 24, 2018
@NicoleYarroch NicoleYarroch assigned ghost May 30, 2018
@@ -434,6 +442,10 @@ - (void)didEnterStateVideoStreamReady {
}
}

- (void)didEnterStateVideoStreamSuspended {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is unused

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I wasn‘t sure if the state methods must be implemented. I can remove it if it‘s not necessary.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NicoleYarroch I added the requested suspended notification to the method.

@NicoleYarroch
Copy link
Contributor

Every time the video comes to the foreground, a SDLVideoStreamDidStartNotification is broadcast. However, no corresponding SDLVideoStreamDidStopNotification is sent. Perhaps there should be another notification called SDLVideoStreamSuspendedNotification that is broadcast when the video stream is suspended.

Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left one comment on the PR about fixing notifications.

@joeljfischer joeljfischer merged commit 8716ebb into smartdevicelink:develop Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A defect in the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suspend and resume instead of restart video stream
2 participants