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

Add helper APIs for adding & removing port destroy handler #4244

Merged
merged 3 commits into from
Jan 10, 2025

Conversation

nanangizz
Copy link
Member

As suggested by @LeonidGoltsblat (here & here):

I think we need an API to add/remove the grp_lock port conf handle - it's simple and gives the application the ability to safely close all the resources it needs.

Real world applications may use pjsip as one of many other libraries and may not be written only in C/C++. The application manages resources outside the pjsip logic and may not be able to use the pool API to allocate memory, but in some cases the closing of application resources must be synchronized with the destruction of conference bridge ports.

For example, an application may implement a special media port proxy to play audio data stored in a BLOB field of a database. With an asynchronous switch, the application cannot close the database connection not only when performing a disconnect, but also when calling remove port, because the conference bridge may try to receive the next frame of data after that. The application must know the exact moment when to release resources, close the database connection, etc. This moment is the "grp_lock handle call".

@@ -563,6 +563,37 @@ PJ_DECL(pj_status_t) pjmedia_port_add_ref( pjmedia_port *port );
PJ_DECL(pj_status_t) pjmedia_port_dec_ref( pjmedia_port *port );


/**
* This is a helper function to add port destructor handler.
Copy link
Member

Choose a reason for hiding this comment

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

It's not obvious when users can, or should, call this function? Should we call pjmedia_port_init_grp_lock() first? Or after adding the port to conf?

Copy link
Member Author

@nanangizz nanangizz Jan 7, 2025

Choose a reason for hiding this comment

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

After adding the port to conf (where port is guaranted to have a group lock, i.e: added by conf if not yet). Updated #3928 desc issue 1 point ii. Should it be added to the function docs (also about avoiding premature pool release)?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please.

Adding to the doc will be of great help in the future so we can easily recall how we're supposed to do this.

@LeonidGoltsblat
Copy link
Contributor

Hi, everyone,

This is not exactly what I meant.

Currently, an application can call pj_grp_lock_add_handler(port->grp_lock, NULL, member, handler) on a media_port pointer without requiring additional API. Technically, grp_lock is part of the media_port. However, applications typically interact with the conference bridge instead.

From a logical standpoint, I believe this functionality should be provided directly by the conference bridge. For example:

PJ_DEF(pj_status_t) pjmedia_conf_add_destroy_handler(
    pjmedia_conf *conf,
    unsigned slot,
    void* member,
    pj_grp_lock_handler handler);

or even:

PJ_DEF(pj_status_t) pjsua_conf_add_destroy_handler(
    unsigned slot,
    void* member,
    pj_grp_lock_handler handler);

This should ideally include paired *_del_* methods for completeness.

@nanangizz
Copy link
Member Author

nanangizz commented Jan 8, 2025

Right, the async behavior is introduced by conference bridge, so it is conference bridge's responsibility to provide the functionality. As currently I assume it is for advanced use (using built-in pjmedia ports does not need such handler, right?), the APIs are to be added for PJMEDIA level only for now.

On the other hand, the group lock being part of the port declares that the nature of port itself is asynchronous (e.g: even conf is not used, some app/module may want to add ref to the port for monitoring purpose), so IMO it is still reasonable to put the functionality to the port itself.

@LeonidGoltsblat
Copy link
Contributor

using built-in pjmedia ports does not need such handler, right?

I am not sure. For example, mem_player may require external memory deallocation (buffer to play). Let it be on both levels media_port + con_bridge. And ideally at the pjsua level too.

@nanangizz
Copy link
Member Author

Ah you're right, I forgot that mem_player/capture is using app buffer. Assuming applications using pjmedia API level directly such as mem_player should be able to access pjmedia port/conf API too, so I guess higher level API is not urgent for now.

@nanangizz nanangizz merged commit 6853491 into master Jan 10, 2025
41 checks passed
@nanangizz nanangizz deleted the port-destroy-handler branch January 10, 2025 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants