[asterisk-bugs] [JIRA] (ASTERISK-22107) Bridge API Enhancements - refactor and redesign ast_bridge_featuresremove interval hooks from ast_bridge_features

[Matt Jordan (JIRA)]

     [ https://issues.asterisk.org/jira/browse/ASTERISK-22107?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Matt Jordan updated ASTERISK-22107:
-----------------------------------

    Description: 
This is sub-optimal:

{noformat}
/*!
 * \brief Structure that contains features information
 */
struct ast_bridge_features {
	/*! Attached DTMF feature hooks */
	struct ao2_container *dtmf_hooks;
	/*! Attached hangup interception hooks container */
	struct ao2_container *hangup_hooks;
	/*! Attached bridge channel join interception hooks container */
	struct ao2_container *join_hooks;
	/*! Attached bridge channel leave interception hooks container */
	struct ao2_container *leave_hooks;
	/*! Attached interval hooks */
	struct ast_heap *interval_hooks;
	/*! Used to determine when interval based features should be checked */
	struct ast_timer *interval_timer;
	/*! Limits feature data */
	struct ast_bridge_features_limits *limits;
	/*! Callback to indicate when a bridge channel has started and stopped talking */
	ast_bridge_talking_indicate_callback talker_cb;
	/*! Callback to destroy any pvt data stored for the talker. */
	ast_bridge_talking_indicate_destructor talker_destructor_cb;
	/*! Talker callback pvt data */
	void *talker_pvt_data;
	/*! Feature flags that are enabled */
	struct ast_flags feature_flags;
	/*! Used to assign the sequence number to the next interval hook added. */
	unsigned int interval_sequence;
	/*! TRUE if feature_flags is setup */
	unsigned int usable:1;
	/*! TRUE if the channel/bridge is muted. */
	unsigned int mute:1;
	/*! TRUE if DTMF should be passed into the bridge tech.  */
	unsigned int dtmf_passthrough:1;
};
{noformat}

While {{ast_bridge_features}} should be the structure that holds a channel's features while it is in a bridge, there are better ways of structuring this data.
# It should structure specific information. Talk detection and other optional function callbacks should at least be put in a sub-structure.
# It should not have a different container for each type of feature. We will extend the types of features that are possible in a bridge - we should not break the ABI every time we do so. While this may mean having a few different containers, {{ao2_containers}} support selection of specific types of objects through {{ao2_callback}}. Given that these containers will not support many hooks, this should not be a major performance hit.
# {{ast_bridge_feature_limits}} is currently leaked in certain situations. If the lifetime of the limits object is not explicitly tied to the lifetime of the features object, it should be an ao2 object.

In general, the goal should be to have a features API that is flexible enough to allow for new feature hooks to be developed without being changed.

While the design of ConfBridge does currently limit this in certain ways, we should attempt to make this as robust as possible.



  was:
This is sub-optimal:

{noformat}
/*!
 * \brief Structure that contains features information
 */
struct ast_bridge_features {
	/*! Attached DTMF feature hooks */
	struct ao2_container *dtmf_hooks;
	/*! Attached hangup interception hooks container */
	struct ao2_container *hangup_hooks;
	/*! Attached bridge channel join interception hooks container */
	struct ao2_container *join_hooks;
	/*! Attached bridge channel leave interception hooks container */
	struct ao2_container *leave_hooks;
	/*! Attached interval hooks */
	struct ast_heap *interval_hooks;
	/*! Used to determine when interval based features should be checked */
	struct ast_timer *interval_timer;
	/*! Limits feature data */
	struct ast_bridge_features_limits *limits;
	/*! Callback to indicate when a bridge channel has started and stopped talking */
	ast_bridge_talking_indicate_callback talker_cb;
	/*! Callback to destroy any pvt data stored for the talker. */
	ast_bridge_talking_indicate_destructor talker_destructor_cb;
	/*! Talker callback pvt data */
	void *talker_pvt_data;
	/*! Feature flags that are enabled */
	struct ast_flags feature_flags;
	/*! Used to assign the sequence number to the next interval hook added. */
	unsigned int interval_sequence;
	/*! TRUE if feature_flags is setup */
	unsigned int usable:1;
	/*! TRUE if the channel/bridge is muted. */
	unsigned int mute:1;
	/*! TRUE if DTMF should be passed into the bridge tech.  */
	unsigned int dtmf_passthrough:1;
};
{noformat}

While {{ast_bridge_features}} should be the structure that holds a channel's features while it is in a bridge, there are better ways of structuring this data.
# It should structure specific information. Talk detection and other optional function callbacks should at least be put in a sub-structure.
# It should not have a different container for each type of feature. We will extend the types of features that are possible in a bridge - we should not break the ABI every time we do so. While this may mean having a few different containers, {{ao2_containers}} support selection of specific types of objects through {{ao2_callback}}. Given that these containers will not support many hooks, we should take
# Interval hooks should be reworked. An instance of an interval hook - which has its own virtual table - should be created and stored with a channel's features in the same ao2 container as the rest of the features. This should be a ref counted object. The current mechanism has bugs related to their destruction and their lifetime.




    
> Bridge API Enhancements - refactor and redesign ast_bridge_featuresremove interval hooks from ast_bridge_features
> -----------------------------------------------------------------------------------------------------------------
>
>                 Key: ASTERISK-22107
>                 URL: https://issues.asterisk.org/jira/browse/ASTERISK-22107
>             Project: Asterisk
>          Issue Type: Bug
>      Security Level: None
>            Reporter: Matt Jordan
>
> This is sub-optimal:
> {noformat}
> /*!
>  * \brief Structure that contains features information
>  */
> struct ast_bridge_features {
> 	/*! Attached DTMF feature hooks */
> 	struct ao2_container *dtmf_hooks;
> 	/*! Attached hangup interception hooks container */
> 	struct ao2_container *hangup_hooks;
> 	/*! Attached bridge channel join interception hooks container */
> 	struct ao2_container *join_hooks;
> 	/*! Attached bridge channel leave interception hooks container */
> 	struct ao2_container *leave_hooks;
> 	/*! Attached interval hooks */
> 	struct ast_heap *interval_hooks;
> 	/*! Used to determine when interval based features should be checked */
> 	struct ast_timer *interval_timer;
> 	/*! Limits feature data */
> 	struct ast_bridge_features_limits *limits;
> 	/*! Callback to indicate when a bridge channel has started and stopped talking */
> 	ast_bridge_talking_indicate_callback talker_cb;
> 	/*! Callback to destroy any pvt data stored for the talker. */
> 	ast_bridge_talking_indicate_destructor talker_destructor_cb;
> 	/*! Talker callback pvt data */
> 	void *talker_pvt_data;
> 	/*! Feature flags that are enabled */
> 	struct ast_flags feature_flags;
> 	/*! Used to assign the sequence number to the next interval hook added. */
> 	unsigned int interval_sequence;
> 	/*! TRUE if feature_flags is setup */
> 	unsigned int usable:1;
> 	/*! TRUE if the channel/bridge is muted. */
> 	unsigned int mute:1;
> 	/*! TRUE if DTMF should be passed into the bridge tech.  */
> 	unsigned int dtmf_passthrough:1;
> };
> {noformat}
> While {{ast_bridge_features}} should be the structure that holds a channel's features while it is in a bridge, there are better ways of structuring this data.
> # It should structure specific information. Talk detection and other optional function callbacks should at least be put in a sub-structure.
> # It should not have a different container for each type of feature. We will extend the types of features that are possible in a bridge - we should not break the ABI every time we do so. While this may mean having a few different containers, {{ao2_containers}} support selection of specific types of objects through {{ao2_callback}}. Given that these containers will not support many hooks, this should not be a major performance hit.
> # {{ast_bridge_feature_limits}} is currently leaked in certain situations. If the lifetime of the limits object is not explicitly tied to the lifetime of the features object, it should be an ao2 object.
> In general, the goal should be to have a features API that is flexible enough to allow for new feature hooks to be developed without being changed.
> While the design of ConfBridge does currently limit this in certain ways, we should attempt to make this as robust as possible.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.asterisk.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira