Skip to content

Conversation

@gongxinheng
Copy link

Add local video track support and publishing hue video track example

@CLAassistant
Copy link

CLAassistant commented Aug 4, 2023

CLA assistant check
All committers have signed the CLA.

room->Connect(URL, TOKEN);

// TODO Non blocking ?
while (!room->GetLocalParticipant()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should implement a correct way to handle events.
Should we expose all events in a virtual class (observer)?

listenerId_ = FfiClient::getInstance().AddListener(
std::bind(&LocalParticipant::OnEvent, this, std::placeholders::_1));

// cv_.wait(lock, [this] { return publishCallback_ != nullptr; });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should block here, but we should have a LocalTrackPublished event

request.mutable_publish_track();
publishTrackRequest->set_track_handle(
track->ffiHandle_.GetHandleId());
*publishTrackRequest->mutable_options() = options;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like your forgot to put the participant_handle inside the request

const proto::TrackPublishOptions& options);

private:
std::condition_variable cv_; // Should we block?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should have a blocking API.
Maybe it is OK to add callback pointers inside the arguments?

uint64_t publishAsyncId_;
FfiClient::ListenerId listenerId_{0};
std::unique_ptr<proto::PublishTrackCallback> publishCallback_;
std::weak_ptr<Room> room_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the new FFI version, it is no longer required to keep a reference to the room here (request don't need it)

FfiClient();
~FfiClient() = default;
FfiHandle(FfiHandle const&) = delete;
FfiHandle& operator=(FfiHandle const&) = delete;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add a unit tests that use the operator= (You said you saw issues related to dropping twice an handle)

@theomonnom
Copy link
Member

theomonnom commented Aug 6, 2023

Open to feedbacks wrt exposing events to the users (Should we do a virtual class? Event polling?)
Cc @cloudwebrtc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants