Skip to content
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

[flatbuffers] Add pause() and resume() in DataProducer and DataConsumer #1104

Conversation

ibc
Copy link
Member

@ibc ibc commented Jun 21, 2023

NOTE: This PR targets flatbuffers branch.

Details

  • pause()and resume() in DataProducer and DataConsumer.
  • paused getter in DataProducer and DataConsumer.
  • dataProducerPaused getter in DataConsumer.
  • dataproducerpause and dataproducerresume events in DataConsumer.
  • pause and resume events in DataProducer.observer and DataConsumer.observer.
  • paused?: boolean in transport.consumeData() and transport.produceData().

Bonus Tracks

  • FBS: Rename WEBRTC_TRANSPORT to WEBRTCTRANSPORT, DATA_PRODUCER to DATAPRODUCER, etc etc everywhere.
  • C++: Add some missing brackets here and there.

@jmillan jmillan self-requested a review June 21, 2023 11:22
@nazar-pc
Copy link
Collaborator

Why though? Seems less readable to me.

@ibc
Copy link
Member Author

ibc commented Jun 21, 2023

Why though? Seems less readable to me.

Consistency. Majority won. It's easier if we don't use separators within entity/class names in FBS dictionaries.

@ibc
Copy link
Member Author

ibc commented Jun 21, 2023

@nazar-pc is this a bug? In producer.rs:

/// Callback is called when the producer is paused.
pub fn on_pause<F: Fn() + Send + Sync + 'static>(&self, callback: F) -> HandlerId {
    self.inner().handlers.pause.add(Arc::new(callback))
}

But nobody calls on_pause() in Producer instances:

$ ack on_pause
src/router.rs
1070:       pipe_consumer
1071:            .on_pause({

src/router/producer.rs
675:    pub fn on_pause<F: Fn() + Send + Sync + 'static>(&self, callback: F) -> HandlerId {

src/router/active_speaker_observer.rs
244:    fn on_pause(&self, callback: Box<dyn Fn() + Send + Sync + 'static>) -> HandlerId {

src/router/audio_level_observer.rs
255:    fn on_pause(&self, callback: Box<dyn Fn() + Send + Sync + 'static>) -> HandlerId {

src/router/rtp_observer.rs
72:    fn on_pause(&self, callback: Box<dyn Fn() + Send + Sync + 'static>) -> HandlerId;

src/router/consumer.rs
853:    pub fn on_pause<F: Fn() + Send + Sync + 'static>(&self, callback: F) -> HandlerId {

@ibc ibc added this to the v3 updates milestone Jun 21, 2023
@ibc ibc added the feature label Jun 21, 2023
@ibc ibc requested a review from nazar-pc June 21, 2023 17:16
@ibc ibc marked this pull request as ready for review June 21, 2023 17:16
@ibc
Copy link
Member Author

ibc commented Jun 21, 2023

@nazar-pc @jmillan this is ready for review. Everything is done, including Rust. However, since Rust tests don't work yet in the flatbuffers branch so... but they should be ok.

@nazar-pc
Copy link
Collaborator

on_pause is for users, so they can attach a callback that will be triggered when producer is paused, I do not believe it is a bug.

@ibc
Copy link
Member Author

ibc commented Jun 21, 2023

I see. Thanks. Approve please 🙏

Copy link
Member

@jmillan jmillan left a comment

Choose a reason for hiding this comment

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

👍

@ibc ibc merged commit 4b104f6 into flatbuffers Jun 22, 2023
30 checks passed
@ibc ibc deleted the flatbuffers-add-pause-resume-in-data-producer-and-data-consumer branch June 22, 2023 09:15
@ibc ibc mentioned this pull request Jun 22, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants