-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Handle live video being disabled during stream #145
Conversation
extension LiveVideoViewController: OTSubscriberDelegate { | ||
public func subscriberDidConnect(toStream subscriber: OTSubscriberKit) {} | ||
|
||
public func subscriber(_ subscriber: OTSubscriberKit, didFailWithError error: OTError) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be implemented for protocol conformance but we've not cared about this error'ing yet. I want to consider its effect and implement it in a separate PR.
@@ -93,6 +93,7 @@ public final class LiveStreamContainerViewController: UIViewController { | |||
_ = self.loaderLabel | |||
|> UILabel.lens.font .~ .ksr_headline(size: 14) | |||
|> UILabel.lens.textColor .~ .white | |||
|> UILabel.lens.textAlignment .~ .center |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This caused almost all the snapshots for this view controller to have to be re-recorded 😐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
@@ -10,6 +10,13 @@ private struct TestOTStreamType: OTStreamType { | |||
fileprivate let streamId: String | |||
} | |||
private class TestOTErrorType: NSError, OTErrorType {} | |||
private struct TestOTSubscriberVideoEventReasonType: OTSubscriberVideoEventReasonType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't think we need to suffix this with Type
since it's a concrete implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah right, lemme tidy that up
self.showErrorAlert | ||
) | ||
self.showErrorAlert, | ||
videoEnabled.filter { !$0 }.mapConst(localizedString( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could filter(isFalse)
if you wanted to point-free.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of course! thanks 👍
During testing of live chat a bug was logged that the video freezed up but that audio and chat continued. I did some testing on a not-so-great 3G connection and was able to reproduce this.
It seems that OpenTok's
OTSubscriberDelegate
(which we hadn't previously implemented) sendssubscriberVideoDisabled
andsubscriberVideoEnabled
with areason
argument which in this case isqualityChanged
.I've changed our
LiveVideoPlaybackState.playing
state to have an associatedBool
value ofvideoEnabled
so that we can handle this.If OpenTok disables the video we hide the view and display text indicating that video will resume when the connection improves. Although I didn't see it resume during my testing, OpenTok does say that the video may resume.