Properly handle broadcast capture state #551
Conversation
When using the broadcast capturer on iOS, the broadcast extension now drives whether or not screen sharing is enabled, only publishing a screen sharing track when the extension actually begins broadcasting. Fixes livekit#444
Adds the ability to get a publisher for receiving notifications.
There was a problem hiding this comment.
I think this looks pretty good and seems to be the right approach.
One design question besides what's in the review comments is how does a developer "stop" a broadcast if they don't want it? Right now, their control is only over whether it is published or not. there needs to be a way to stop it, too...
Another question is is this a breaking change? it looks like maybe it's actually not a breaking change and it will "just work"?
|
|
||
| Task { [weak self] in | ||
| guard let self else { return } | ||
| let shouldPublish = await self.broadcastStarted?() ?? true |
There was a problem hiding this comment.
i think that method could have a clearer name, such as shouldPublishBroadcast, to make it more obvious how it fits in
although, thinking more about it, perhaps it would be clearest if the method were simply called onBroadcastStart. if the developer implements it, then they're given complete control over what to do next. otherwise the default implementation is to call try await self.setScreenShare(enabled: true) if they're connected and to cancel the broadcast if they're not.
that way we don't need to let this task hang indefinitely if the developer has more complex needs (such as maybe the user is running broadcast in a "preview" mode not meant to be published right away)
| return | ||
| } | ||
| do { | ||
| try await self.setScreenShare(enabled: true) |
There was a problem hiding this comment.
what happens if you're not already actively connected to a room?
There was a problem hiding this comment.
setScreenShare(enabled:) calls down to set(source:enabled:captureOptions:publishOptions:) which in turn calls requireRoom(), performing a check to ensure the room isn't nil. I will need to perform some more examination to see if additional checks are necessary.
| /// | ||
| public var broadcastStarted: (() async -> Bool)? | ||
|
|
||
| private var isBroadcasting = false { |
There was a problem hiding this comment.
i think this or something like it needs to be publicly gettable, because developers may have UI that hinges on this state, in the case that they aren't starting publishing right away
There was a problem hiding this comment.
Is it okay if we expose a Combine AnyPublisher<Bool, Never> in the public API? If so, I could see making the isBroadcasting property a CurrentValueSubject which is type erased into AnyPublisher. This would allow both getting the current value and listening for changes. We could then replace the broadcastStarted closure with a Boolean publishOnBroadcast property which allows the developer to disable the default behavior to manually control track publication as per your suggestion. The public interface would then look something like this:
public var publishOnBroadcast: Bool
public var isBroadcasting: AnyPublisher<Bool, Never>There was a problem hiding this comment.
it's an interesting idea and i'd defer to @hiroshihorie but my instinct is that we should consider introducing new patterns carefully and deliberately. i'm not sure it's worth using something novel here that is unlike other things in our SDK, especially because I'm not sure there's a really strong reason we'd need to besides that it might be a little nicer.
we use a delegate pattern everywhere else. maybe good to make a new BroadcastDelegate that has this and the onStarted method in it
There was a problem hiding this comment.
I think @bcherry is right, I'm open to adding a AnyPublisher in addition to current design of reading value synchronously and notifying with delegate perhaps.
We eventually need to modernize the SDK and I think we can start here incrementally.
There was a problem hiding this comment.
@hiroshihorie, just to make sure I’m following you, are you thinking the public interface would look something like this then?
public var isBroadcasting: Bool
public var isBroadcastingPublisher: AnyPublisher<Bool, Never>This would be in addition to the delegate methods.
Currently, there is no method to stop a broadcast other than by unpublishing the screen share track. To address your other comments, I am adding a method to stop the broadcast even if a track has not been published. Regarding whether this constitutes a breaking change, there are a few points to consider. At present, when calling Please let me know your thoughts on this. |
yeah it makes sense to add something new since broadcast is no longer explicitly tied to publication.
that does sound like a minor breaking change so we'll be sure to update the sdk minor version and include a clear note about it in the release notes. since the default "detailed changelog" is just the git history, can you ensure the description on this PR includes notes about how this would affect existing apps and what they need to change to adapt? that will then go into the sqaushed commit as a description and be linked in the history as well |
|
looks like this will also close #472 |
When using the broadcast capturer on iOS, the broadcast extension now drives whether or not screen sharing is enabled, publishing a screen sharing track when the extension begins broadcasting upon user approval. Fixes #444.
Minor breaking change
Calling
setScreenShare(enabled: true)onLocalParticipantcurrently returns aLocalTrackPublicationrepresenting the newly published screen share track. After this change, when using the broadcast capturer on iOS, this method will returnnil, as the track is published asynchronously pending user approval. Developers should treat enabling screen share as a request that might not be fulfilled and should not interpret anilreturn value from this method as an error.