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

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/12814 )

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


Patch Set 5: Code-Review-1

(4 comments)

https://gerrit.asterisk.org/#/c/12814/5/include/asterisk/rtp_engine.h 
File include/asterisk/rtp_engine.h:

https://gerrit.asterisk.org/#/c/12814/5/include/asterisk/rtp_engine.h@592 
PS5, Line 592: #ifdef TEST_FRAMEWORK
             : /*! \brief Structure that represents the test functionality for res_rtp_asterisk unit tests */
             : struct ast_rtp_engine_test {
             : 	/*! Set to 1 whenever SDES is received */
             : 	int sdes_received;
             : };
             : #endif
Make this an opaque structure (unless you move the test callbacks into this - see below).


https://gerrit.asterisk.org/#/c/12814/5/include/asterisk/rtp_engine.h@682 
PS5, Line 682: 	/*! Get the number of packets in the receive buffer for a RTP instance */
             : 	size_t (*recv_buffer_count)(struct ast_rtp_instance *instance);
             : 	/*! Get the maximum number of packets the receive buffer can hold for a RTP instance */
             : 	size_t (*recv_buffer_max)(struct ast_rtp_instance *instance);
             : 	/*! Get the number of packets in the send buffer for a RTP instance */
             : 	size_t (*send_buffer_count)(struct ast_rtp_instance *instance);
             : 	/*! Set the schedid for RTCP */
             : 	void (*set_schedid)(struct ast_rtp_instance *instance, int id);
             : 	/*! Set the number of packets to drop on RTP read */
             : 	void (*drop_packets)(int num);
             : 	/*! Send a SR/RR on next RTP send */
             : 	void (*queue_report)(void);
I think I'd prefer these in their own structure for grouping of all test facilities. Can these be moved into the ast_rtp_engine_test struct?


https://gerrit.asterisk.org/#/c/12814/5/main/rtp_engine.c 
File main/rtp_engine.c:

https://gerrit.asterisk.org/#/c/12814/5/main/rtp_engine.c@2903 
PS5, Line 2903: 	if (instance->engine->test) {
              : 		return instance->engine->test;
              : 	}
              : 
              : 	return NULL;
If you want you can drop the "if" and just return instance->engine->test


https://gerrit.asterisk.org/#/c/12814/5/tests/test_res_rtp.c 
File tests/test_res_rtp.c:

https://gerrit.asterisk.org/#/c/12814/5/tests/test_res_rtp.c@81 
PS5, Line 81: *instance1 = ast_rtp_instance_new("asterisk", test_sched, &addr, NULL);
            : 	*instance2 = ast_rtp_instance_new("asterisk", test_sched, &addr, NULL);
Check for NULL on these before continuing.



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

Gerrit-Project: asterisk
Gerrit-Branch: 17
Gerrit-Change-Id: I56107c7411003a247589bbb6086d25c54719901b
Gerrit-Change-Number: 12814
Gerrit-PatchSet: 5
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-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Fri, 06 Sep 2019 15:25:02 +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/20190906/bbd5497e/attachment.html>