<p>Patch set 5:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/12814">View Change</a></p><p>4 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/12814/5/include/asterisk/rtp_engine.h">File include/asterisk/rtp_engine.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/12814/5/include/asterisk/rtp_engine.h@592">Patch Set #5, Line 592:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">#ifdef TEST_FRAMEWORK<br>/*! \brief Structure that represents the test functionality for res_rtp_asterisk unit tests */<br>struct ast_rtp_engine_test {<br> /*! Set to 1 whenever SDES is received */<br> int sdes_received;<br>};<br>#endif<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Make this an opaque structure (unless you move the test callbacks into this - see below).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/12814/5/include/asterisk/rtp_engine.h@682">Patch Set #5, Line 682:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /*! Get the number of packets in the receive buffer for a RTP instance */<br> size_t (*recv_buffer_count)(struct ast_rtp_instance *instance);<br> /*! Get the maximum number of packets the receive buffer can hold for a RTP instance */<br> size_t (*recv_buffer_max)(struct ast_rtp_instance *instance);<br> /*! Get the number of packets in the send buffer for a RTP instance */<br> size_t (*send_buffer_count)(struct ast_rtp_instance *instance);<br> /*! Set the schedid for RTCP */<br> void (*set_schedid)(struct ast_rtp_instance *instance, int id);<br> /*! Set the number of packets to drop on RTP read */<br> void (*drop_packets)(int num);<br> /*! Send a SR/RR on next RTP send */<br> void (*queue_report)(void);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/12814/5/main/rtp_engine.c">File main/rtp_engine.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/12814/5/main/rtp_engine.c@2903">Patch Set #5, Line 2903:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (instance->engine->test) {<br> return instance->engine->test;<br> }<br><br> return NULL;<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">If you want you can drop the "if" and just return instance->engine->test</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/12814/5/tests/test_res_rtp.c">File tests/test_res_rtp.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/12814/5/tests/test_res_rtp.c@81">Patch Set #5, Line 81:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">*instance1 = ast_rtp_instance_new("asterisk", test_sched, &addr, NULL);<br> *instance2 = ast_rtp_instance_new("asterisk", test_sched, &addr, NULL);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Check for NULL on these before continuing.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/12814">change 12814</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/12814"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 17 </div>
<div style="display:none"> Gerrit-Change-Id: I56107c7411003a247589bbb6086d25c54719901b </div>
<div style="display:none"> Gerrit-Change-Number: 12814 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 06 Sep 2019 15:25:02 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>