[Asterisk-code-review] res_rtp: Add unit tests for RTCP stats. (...asterisk[16])

[Joshua Colp]

Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/12813 )

Change subject: res_rtp: Add unit tests for RTCP stats.
......................................................................


Patch Set 4: Code-Review-1

(5 comments)

https://gerrit.asterisk.org/#/c/12813/4/include/asterisk/rtp_engine.h 
File include/asterisk/rtp_engine.h:

https://gerrit.asterisk.org/#/c/12813/4/include/asterisk/rtp_engine.h@593 
PS4, Line 593: /*! \brief Structure that represents the test functionality for RTP/RTCP unit tests */
To be pedantic - this is actually in regards to res_rtp_asterisk functionality.


https://gerrit.asterisk.org/#/c/12813/4/include/asterisk/rtp_engine.h@595 
PS4, Line 595: /*! Set the number of RTP packets to drop */
             : 	int packets_to_drop;
             : 	/*! Sends a RTCP SR/RR report the next time RTP is sent instead of the RTP packet and sets this back to 0 */
             : 	int send_report;
Any reason these weren't callbacks into the RTP engine, that could then act on them however it wished?


https://gerrit.asterisk.org/#/c/12813/4/include/asterisk/rtp_engine.h@601 
PS4, Line 601: /*! Used to compare integer values */
             : 	int int_value;
             : 	/*! Compare function for int_value; sets condition_met to 1 */
             : 	void (*check_int_value)(int value);
What does this actually... mean and why didn't check_int_value just return an int stating that the condition was met if the function is synchronous? As well since int_value is public, you could just check that and not have a callback


https://gerrit.asterisk.org/#/c/12813/4/res/res_rtp_asterisk.c 
File res/res_rtp_asterisk.c:

https://gerrit.asterisk.org/#/c/12813/4/res/res_rtp_asterisk.c@2408 
PS4, Line 2408: static void set_rtp_rtcp_schedid(struct ast_rtp_instance *instance, int id)
              : {
              : 	struct ast_rtp *rtp = ast_rtp_instance_get_data(instance);
              : 
              : 	if (rtp && rtp->rtcp) {
              : 		rtp->rtcp->schedid = id;
              : 	}
              : }
Why this instead of just passing in a valid scheduler context on instance creation?


https://gerrit.asterisk.org/#/c/12813/4/res/res_rtp_asterisk.c@6242 
PS4, Line 6242: 				test_engine->condition_met = 1;
Is this used anywhere else? I think it'd be better named as sdes_received or something.



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/12813
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: I56107c7411003a247589bbb6086d25c54719901b
Gerrit-Change-Number: 12813
Gerrit-PatchSet: 4
Gerrit-Owner: Benjamin Keith Ford <bford at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Comment-Date: Tue, 03 Sep 2019 14:35:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190903/e7c4b1b8/attachment.html>