-
Notifications
You must be signed in to change notification settings - Fork 119
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
DEVX-5711 Implement selective stream feature for Archive and Broadcast #235
Conversation
lib/opentok/archives.rb
Outdated
# @raise [ArgumentError] | ||
# The has_audio and has_video properties of the options parameter are both set to "false" | ||
# | ||
def add_stream(archive_id, stream_mode, options) |
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.
Please remove the stream_mode parameter. You do not set the stream mode when calling this method (you set it when you start the archive).
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.
I think stream_jd
should be its own parameter, not a property of the options parameter.
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.
Please add stream_mode
to the options
of the opentok.archives.create
method. The Archive class should include a stream_mode attribute and a streams attribute. (The same comments apply to broadcast.)
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.
Please remove the stream_mode parameter. You do not set the stream mode when calling this method (you set it when you start the archive).
Archives#add_stream
isn't intended to be called directly, only via the Archive#add_stream
wrapper method; this method passes in whatever the stream mode is for the currently running archive. The only reason I was passing this into Archives#add_stream
was to perform a check on it there, but I can move the check to the Archive#add_stream
wrapper method instead and not pass the argument (I only did it this way to try and keep the wrapper methods as simple as possible in terms of logic).
The Archive class should include a stream_mode attribute and a streams attribute. (The same comments apply to broadcast.)
I'm not entirely sure what you mean here. If you mean store the stream_mode
data somewhere on the Archive
object itself, it's already stored in the Hash assigned to the @json
instance variable. TBH, I don't think this is an optimal approach, but I also don't particularly want to go refactoring parts of the current implementation that aren't directly related to adding this new feature.
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.
Re adding stream_mode attribute and a streams attributes to the Archive and Broadcast classes, I suggested it because the Archive and Broadcast objects returned by the REST methods now include streamMode
and streams
propperties. (See https://tokbox.com/developer/rest/#retrieve_archive_info and https://tokbox.com/developer/rest/#get_info_broadcast). Even if these properties are exposed via the @JSON instance variable, shouldn't we add @attr
references to these in the docs comments (@attr [Array] streams
and @attr [string] streamMode
)?
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.
Hi @jeffswartz. I'm not opposed to adding properties on the objects (FWIW, I don't particularly like the current implementation of using a Hash assigned to the @json
ivar). However, I'd suggest making such changes as part of a separate PR (or even waiting until we roll the video functionality into the core SDK next year -- there's also a bunch of other stuff I'd like to change about the way these classes are implemented), since a number of other methods not related to this particular feature would also need to be refactored.
Regarding the docs comments, the data in the hash assigned to the @json
instance variable in the initialize
method represents the response from starting an Archive/ Broadcast.
- For
broadcast.rb
the comments seem to be in line with the documentation, including thestreamMode
comment;streams
isn't listed in the comments, but this isn't in the docs either. - For
archive.rb
, again thestreamMode
comment is there, though notstreams
(though again this isn't listed in the documentation for the response data for starting an archive). There are a couple of discrepancies with the documentation (which I will fix):partner_id
should beprojectId
, andresolution
is missing.
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.
Updated the Archive
comments in this commit
lib/opentok/archives.rb
Outdated
# @raise [ArgumentError] | ||
# The has_audio and has_video properties of the options parameter are both set to "false" | ||
# | ||
def add_stream(archive_id, stream_mode, options) |
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.
Please add stream_mode
to the options
of the opentok.archives.create
method. The Archive class should include a stream_mode attribute and a streams attribute. (The same comments apply to broadcast.)
c4a711e
to
c3f08ad
Compare
Description
Implements the play Archive/ Broadcast selective stream feature. This feature allows for streams to be added to or removed from archives or broadcasts started in manual mode.
Motivation and Context
It implements a feature in the SDK which has already been added to the API.
How Has This Been Tested?
Example Output or Screenshots (if appropriate):
Types of changes
Checklist: