livekit_bridge: An ergonomic interface into the Livekit C++ SDK#58
livekit_bridge: An ergonomic interface into the Livekit C++ SDK#58stephen-derosa wants to merge 1 commit intolivekit:mainfrom
Conversation
|
|
xianshijing-lk
left a comment
There was a problem hiding this comment.
Good work, that is a large PR, I will need more time to review it.
some initial questions to get a clearer picture on the design.
| @@ -0,0 +1,272 @@ | |||
| /* | |||
There was a problem hiding this comment.
please add livekit copyright, and add it to all the new files.
| #include <cstring> | ||
| #include <iostream> | ||
| #include <mutex> | ||
| #include <vector> |
There was a problem hiding this comment.
run clang format, you can hook it up to git commit:
Clang format
CPP SDK is using clang C++ format
brew install clang-format
|
|
||
| // Check for a new video frame | ||
| { | ||
| std::lock_guard<std::mutex> lock(g_latest_video.mutex); |
There was a problem hiding this comment.
can we try avoiding locking while doing the heavy duty things ?
I think this can be achieved by having a local_pixel, and just swap the buffer, like
std::vectorstd::uint8_t local_pixels;
int fw = 0, fh = 0;
bool have_frame = false;
{
std::lock_guardstd::mutex lock(g_latest_video.mutex);
if (g_latest_video.dirty && g_latest_video.width > 0 && g_latest_video.height > 0) {
fw = g_latest_video.width;
fh = g_latest_video.height;
local_pixels.swap(g_latest_video.data); // avoid copy
g_latest_video.dirty = false;
have_frame = true;
}
}
and use the local_pixel and other local variables for rendering.
| @@ -0,0 +1,133 @@ | |||
| /* | |||
| * Human example -- receives audio and video frames from a robot in a | |||
| options.auto_subscribe = true; | ||
| options.dynacast = false; | ||
|
|
||
| bool result = room_->Connect(url, token, options); |
There was a problem hiding this comment.
In general, I think it is not recommended to call Connect() while holding the std::lock_guardstd::mutex lock(mutex_);
maybe it is better to create a local room, like
auto room = std::make_uniquelivekit::Room();
set things up, then reassign it to room_
| * @param token Access token for authentication. | ||
| * @return true if connection succeeded. | ||
| */ | ||
| bool connect(const std::string &url, const std::string &token); |
There was a problem hiding this comment.
curiously, don't you need to expose the room option via the connect() function ?
| * @param source Track source (e.g. SOURCE_MICROPHONE). | ||
| * @param callback Function to invoke per audio frame. | ||
| */ | ||
| void registerOnAudioFrame(const std::string &participant_identity, |
There was a problem hiding this comment.
is it register** the right word ?
I wonder what will happen if users register the same callback or multiple callbacks here ?
do we support those use cases ?
if not, can we make it a bit less ambiguous ?
| * If a reader thread is active for this (identity, source), it is | ||
| * stopped and joined. | ||
| */ | ||
| void unregisterOnAudioFrame(const std::string &participant_identity, |
There was a problem hiding this comment.
it looks like it should "clear" rather than unregister ?
| void startAudioReader(const CallbackKey &key, | ||
| const std::shared_ptr<livekit::Track> &track, | ||
| AudioFrameCallback cb); | ||
| void startVideoReader(const CallbackKey &key, |
There was a problem hiding this comment.
hmm. this is a bit worrying, are we saying that we will have multiple render threads if we want to render multiple tracks ?
| * mic->mute(); | ||
| * mic.reset(); // unpublishes | ||
| */ | ||
| class BridgeAudioTrack { |
There was a problem hiding this comment.
Are these BridgeAudioTrack, BridgeVideoTrack public interface to our developers ?
Note, from the code, these track implementations are not thread safe on its own, that says, if users want to get access to these bridge track, they are not protected
| released_ = true; | ||
|
|
||
| // Unpublish the track from the room | ||
| if (participant_ && publication_) { |
There was a problem hiding this comment.
From what I read, the BridgeVideoTrack can outlive the LivekitBridge::disconnect(), like that can be destruct after calling livekitBridge::disconnect().
In that case, how does our code guarantee that the it will not call into the participant_ ?
| } | ||
|
|
||
| // Create room and delegate | ||
| room_ = std::make_unique<livekit::Room>(); |
There was a problem hiding this comment.
nit, should you add an assert to make sure room_ is nullptr here ?
| { | ||
| std::lock_guard<std::mutex> lock(mutex_); | ||
|
|
||
| if (!connected_) { |
There was a problem hiding this comment.
I think it is wrong to return here.
For instance, if the connect() go thru the steps and fail at bool result = room_->Connect(url, token, options);
the connected_ is false, but you never call livekit::shutdown(); to clear up things
Overview
An ergonomic library providing simple usage of the C++ SDK.
Building
This library is attached to the build system of the core C++ SDK library. Use
build.shas is.Testing
The
bridge/examples/directory has simulatedhumanandrobottests. There are 4 files:These have been tested manually.
Unit tests
CallbackKeyhashing/equality,BridgeAudioTrack/BridgeVideoTrackstate management, andLiveKitBridgepre-connection behaviour (callback registration, error handling).Limitations
The bridge is designed for simplicity and currently only supports limited audio and video features. It does not expose:
RoomOptionsorTrackPublishOptionsFor advanced use cases, use the full
client-sdk-cppAPI directly, or expand the bridge to support your use case.TODOs before merge