[Asterisk-code-review] bridge softmix / app confbridge: Add support for REMB combin... (asterisk[15])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/8782 )

Change subject: bridge_softmix / app_confbridge: Add support for REMB combining.
......................................................................


Patch Set 1: Code-Review-1

(4 comments)

https://gerrit.asterisk.org/#/c/8782/1/bridges/bridge_softmix.c
File bridges/bridge_softmix.c:

https://gerrit.asterisk.org/#/c/8782/1/bridges/bridge_softmix.c@1156
PS1, Line 1156: 		if (!bridge->softmix.video_mode.mode_data.sfu_data.video_update_discard ||
              : 			ast_tvdiff_ms(ast_tvnow(), softmix_data->last_video_update) > bridge->softmix.video_mode.mode_data.sfu_data.video_update_discard) {
Moving video_update_discard into this new location requires a check that we are in SFU mode before accessing.  This new location is held within a union and not a struct.


https://gerrit.asterisk.org/#/c/8782/1/bridges/bridge_softmix.c@1191
PS1, Line 1191: 	if (frame->subclass.integer != AST_RTP_RTCP_PSFB || feedback->fmt != AST_RTP_RTCP_FMT_REMB ||
              : 		!bridge->softmix.video_mode.mode_data.sfu_data.remb_send_interval) {
Need to check that we are in SFU mode before accessing remb_send_interval.


https://gerrit.asterisk.org/#/c/8782/1/bridges/bridge_softmix.c@1629
PS1, Line 1629: 		if (bridge->softmix.video_mode.mode_data.sfu_data.remb_send_interval &&
              : 			ast_tvdiff_ms(ast_tvnow(), softmix_data->last_remb_update) > bridge->softmix.video_mode.mode_data.sfu_data.remb_send_interval) {
Need check that we are in SFU mode before accessing remb_send_interval.


https://gerrit.asterisk.org/#/c/8782/1/include/asterisk/bridge.h
File include/asterisk/bridge.h:

https://gerrit.asterisk.org/#/c/8782/1/include/asterisk/bridge.h@141
PS1, Line 141: 	/*! The minimum interval between video updates */
             : 	unsigned int video_update_discard;
             : 	/*! The interval at which a REMB report is generated and sent */
             : 	unsigned int remb_send_interval;
Moving video_update_discard will affect ABI by shifting struct ast_bridge member offsets in released v15 versions.

Moving remb_send_interval into here actually fixes ABI shifging caused by adding remb_send_interval since it has not made it to a release yet.

How concerned are we about this ABI breakage?

Actually moving video_update_discard is not good because it could be accessed when not in SFU mode by an AST_CONTROL_VIDUPDATE frame.



-- 
To view, visit https://gerrit.asterisk.org/8782
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: I9eafe4e7c1f72d67074a8d6acb26bfcf19322b66
Gerrit-Change-Number: 8782
Gerrit-PatchSet: 1
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Mon, 16 Apr 2018 20:12:32 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180416/c15abc78/attachment.html>