Conversation
| const std::string &method, const std::string &payload, | ||
| std::optional<double> response_timeout = std::nullopt); | ||
| std::string | ||
| performRpc(const std::string &destination_identity, const std::string &method, |
There was a problem hiding this comment.
did you run clang format ?
There was a problem hiding this comment.
yes, not sure how the original unformatted got in to begin with. Maybe before linting was in the pipeline?
| auto *opts = connect->mutable_options(); | ||
| opts->set_auto_subscribe(options.auto_subscribe); | ||
| opts->set_dynacast(options.dynacast); | ||
| opts->set_single_peer_connection(options.single_peer_connection); |
There was a problem hiding this comment.
may I ask why we stop calling this set_single_peer_connection() ?
There was a problem hiding this comment.
restored -- this was from a local branch that removed this due to SFUs not supporting single peer connection by default
bridge/src/rpc_manager.h
Outdated
| /// @param destination_identity Identity of the remote participant. | ||
| /// @param track_name Name of the track to mute. | ||
| /// @throws if the LocalParticipant requestTrackMute fails. | ||
| void requestTrackMute(const std::string &destination_identity, |
There was a problem hiding this comment.
why this TrackMute / TrackUnmute APIs belong to rpc_manager ? rather than the tracks ?
There was a problem hiding this comment.
this is to mute other tracks. Unclear naming, so i updated the function name to requestRemoteTrackMute/Unmute ? This was a feature asked for by Polymath.
| const std::string &method, const std::string &payload, | ||
| const std::optional<double> &response_timeout) { | ||
| assert(lp_ != nullptr); | ||
| return lp_->performRpc(destination_identity, method, payload, |
There was a problem hiding this comment.
I don't understand why we need such RpcManager ? it seems a wrapper to call local_participant APIs ?
There was a problem hiding this comment.
great question. The motivation behind the RPCManager was to be a single object that holds all RPC "work". I also think that people will be asking for "out of the box" RPC calls. For example Polymath asked for the ability to mute remote tracks. This RPC manager is a clean single place to add these "built in" RPC calls.
There was a problem hiding this comment.
I see. thanks for the context, I think now things make sense.
Tho I do wonder if RpcManager is the right object for the long term. RpcManager sounds like a manager that manages all Rpc calls, is this its intention ? or you want to implement a remote controller ? that this remote_controller is using RPC under the hood to pass msgs to the remote participant, and take actions ?
bridge/src/rpc_manager.h
Outdated
| /// Callback the bridge provides to execute a track action | ||
| /// (mute/unmute/release). Throws livekit::RpcError if the track is not found | ||
| /// or the action is invalid. | ||
| using TrackActionFn = std::function<void(const std::string &action, |
There was a problem hiding this comment.
why it takes std::string as action ? rather than enum (assuming it only suuport mute / unmute/release) ?
btw, not sure why we need this TrackActionFn either, it is a callback ?
There was a problem hiding this comment.
nice great point ill make it an enum (can always add more enums in the future) :)
and yeah its just a type for the callback function to make the callback setting a bit cleaner to read
There was a problem hiding this comment.
Nice, I think enum is easier to use and less error prone for the developers.
stephen-derosa
force-pushed
the
sderosa/bridge-rpc
branch
from
927f6ed to
f508fec
Compare
RPCManager: bridge object for managing default and custom RPC calls bridge_rpc examples RPCManager tests
…rackActionFn action input an Enum
stephen-derosa
force-pushed
the
sderosa/bridge-rpc
branch
from
e38dd58 to
3237ad6
Compare
|
I think we are on the right track. some more questions |
| * @param handler Callback invoked on each incoming invocation. | ||
| * @return true if the RPC method was registered successfully. | ||
| */ | ||
| bool registerRpcMethod(const std::string &method_name, |
There was a problem hiding this comment.
RpcMethod is pretty low level, I wonder if we should make a remote controller, and we will setup a remoteControlInternal method as the communication channel for the remote control
2 participants
Overview
RPC Manager for handling built in RPC calls and user registered RPC calls.
Built in RPC calls
Examples
examples/bridge_mute_unmute/
examples/bridge_rpc/
Testing