From what I can tell, these new "try" methods have the same inputs as the deprecated prior ones, just a new return type (and name).

This is actually a situation where I think maintaining the API and using exceptions is cleaner than deprecation/new API just to get the bool return type.

Thoughts on this? Could we not move the new implementations from room.cpp into the older versions and throw instead of returning false?

isnt that still changing the public API though? maybe im wrong but i would think adding a throw to a function that previously didnt would require a major bump?

Fair point, technically speaking those methods were never marked noexcept and anything within them could throw (STL container exceptions, runtime exceptions from FFI stuff, etc.) so I think it's the least of the evils?

hm yeah im a little torn here. I dont want to just start throwing underneath applications, especially when our mantra has somewhat been team #nothrow 😅

but the point of other things can throw inside the function are valid 🤔

Also torn, I don't love my suggestion, to me keeping the API clean/avoiding deprecations is the least of the evils. Maybe @xianshijing-lk can chime in

I guess I didn't understand why these callback events need to be in the room ?
couldn't they be in the remote participants?

And does this frame callbacks work on both local / remote participants ?

the room has access to the room events which are needed to set callbacks to incoming track messages -- the remote participant doesnt really.

This callback only works for local participants, the intention is to provide a nice ergonomic way for the user to set callbacks for incoming messages.

should you still return true here ?

good question, yes since the user has successfully set the callback. It doesnt necessarily mean that a track has been published or that they will get messages on said track. If we were to only return true to a callback being set after a track has been published, then early joining participants that want incoming messages will effectively have to periodically set callbacks until success, or listen to room events -- both of which defeat the purpose of the ergonomic functionality of these setOn*Callback functions.

I think I forgot some technical details here, is this subscription_thread_dispatcher_ a manager that manages all the subscription thread ?
and does one callback has one corresponding subscription thread ? or one thread that handles all the callbacks ?

I think I forgot some technical details here, is this subscription_thread_dispatcher_ a manager that manages all the subscription thread ?

it manages subscribing to tracks and providing incoming frames to the users callbacks

and does one callback has one corresponding subscription thread ? or one thread that handles all the callbacks ?

Each subscription gets its own thread

@alan-george-lk
In this work, i realized subscription_thread_manager is marked public:

I missed this in the review for the LIVEKIT_API PR. any thoughts on making this private?

I ran a query on this and it doesnt look like:

1. this file is actually included in any of the other public includes
2. the SubscriptionThreadDispatcher doesnt really work without the room wrapper
3. its only used by the room

discussed in the standup need to confirm that:

• cpp examples build with this private
• it is in fact not included/usable outside the room.cpp/hpp

consider moving to src/ so that this isn't reachable

🟡 Removing a data callback from inside itself never stops delivery

When the removal runs from inside the data callback it targets, the self-join guard (extractDataReaderThreadLocked) returns without cancelling the reader or closing its stream, so the reader keeps reading and keeps invoking the removed callback until the track is unpublished or the room is torn down. The unpublish path handles the same case by closing the stream.

In extractDataReaderThreadLocked (src/subscription_thread_dispatcher.cpp), the self-thread branch returns early to avoid a self-join, but it does not stop the reader: it neither sets cancelled nor closes the reader's stream. As a result a removeOnDataFrameCallback call made from inside the target data callback leaves the reader looping on stream->read() and invoking its own captured copy of the callback indefinitely. The sibling case in handleDataTrackUnpublished handles this correctly by setting reader->cancelled = true and closing reader->stream under sub_mutex before leaving the slot in place. Consider mirroring that here: mark the reader cancelled and close its stream (guarded by sub_mutex; lock_ is already held so the lock_->sub_mutex order is preserved) before returning the empty thread, so the reader exits on its next read and stops delivering to the removed callback. Note the comment claiming 'the reader exits on its own once its stream closes' is currently inaccurate because nothing closes the stream in this branch.

Was this helpful? React with 👍 or 👎 to provide feedback.

at first i thought this was a bit verbose, but i actually think this is appropriate