Snom patch: rtp_echo action #259

pbertera · 2016-10-01T15:40:52Z

rtp_echo: This patch introduces a new action <rtp_echo> allowing to stop and start the RTP echo from the scenario file.

Example:

<nop>
        <action>
                <rtp_echo value="0"/> <!-- stop the RTP echo -->
        </action>
</nop>
<pause milliseconds="15000"/>
<nop>
        <action>
                <rtp_echo value="1"/> <!-- start the RTP echo -->
        </action>
</nop>

All the attribution for this patch should go to Snom Technology AG

wdoekes

This lacks documentation. A simple test case that uses the new rtp_echo and doesn't die because of unimplemented commands would be better than nothing.

wdoekes · 2016-10-02T09:18:53Z

src/call.cpp

+#ifdef RTP_STREAM
+        } else if (currentAction->getActionType() == CAction::E_AT_RTP_ECHO) {
+            rtp_echo_state = (currentAction->getDoubleValue() != 0 ? true : false);
+#endif


There is an RTP_STREAM ifdef below on line 3693, why not put it in there.

And rtp_echo_state = (currentAction->getDoubleValue() != 0) is explicit enough for a boolean assignment.

wdoekes · 2016-10-02T09:19:45Z

src/scenario.cpp

+        } else if(!strcmp(actionElem, "rtp_echo")) {
+            tmpAction->setActionType(CAction::E_AT_RTP_ECHO);
+            handle_rhs(tmpAction, "rtp_echo");
+#endif


There is an #ifdef already at 1634.. please move it closer.

wdoekes · 2016-10-02T09:22:05Z

src/sipp.cpp

            return;
        }
+#ifdef RTP_STREAM
+        if (rtp_echo_state == false) {


!rtp_echo_state

pbertera · 2016-10-03T19:37:56Z

Thanks @wdoekes for the review and feedback. This should be better now.

wdoekes

Thanks for the fixes. Looks almost okay.

Two issues below.
Please add a small description to the changelog.
Please squash all your changes into a single commit (git rebase -i ...; git push -f).

wdoekes · 2016-10-04T08:25:07Z

src/scenario.cpp

+            tmpAction->setActionType(CAction::E_AT_RTP_ECHO);
+            handle_rhs(tmpAction, "rtp_echo");
+#else
+                ERROR("Scenario specifies a rtp_echo action, but this version of SIPp does not have RTP stream support");


Indentation nitpick.

wdoekes · 2016-10-04T08:27:06Z

regress/github-#0259/run

+    fail "got RTP from wrong source port $port"
+else
+    fail "got no RTP at all"
+fi


Lines 21..27 are NO-OPS since $port and $uac_media_port are blank.

Replace with a single ok.

pbertera

Done, Thanks for your patient review.

wdoekes · 2016-10-05T10:22:55Z

Looks good.

I wonder why this just happened in one travis check:

checking for unistd.h... yes
checking openssl/md5.h usability... no
checking openssl/md5.h presence... no
checking for openssl/md5.h... no
configure: error: <openssl/md5.h> header missing
The command "./configure $CONFOPTS" failed and exited with 1 during .

(see build error in #259)

wdoekes · 2016-10-05T10:40:35Z

Something has changed with the OSX travis build. Investigating... I hope it's fixed with: https://travis-ci.org/SIPp/sipp/builds/165197224

(see build error in #259)

wdoekes · 2016-10-05T11:42:15Z

Ok. Should be fixed by 7412b02. Can you rebase against new master?

pbertera · 2016-10-05T11:59:00Z

Thanks, now travis build looks good

- UAC side of RTPSTREAM must specify mandatory payload_name parameter

wdoekes requested changes Oct 2, 2016

View reviewed changes

wdoekes added question request-feedback and removed question labels Oct 2, 2016

wdoekes requested changes Oct 4, 2016

View reviewed changes

pbertera force-pushed the snom-rtp_echo branch 2 times, most recently from 6bb84c2 to f4c79b2 Compare October 5, 2016 10:13

pbertera commented Oct 5, 2016

View reviewed changes

wdoekes added a commit that referenced this pull request Oct 5, 2016

build: We don't use openssl/md5.h. Don't check for it either.

61247ad

(see build error in #259)

wdoekes added a commit that referenced this pull request Oct 5, 2016

build: We don't use openssl/md5.h. Don't check for it either.

b3e2d5a

(see build error in #259)

Added the rtp_echo scenario action

b540cec

pbertera force-pushed the snom-rtp_echo branch from f4c79b2 to b540cec Compare October 5, 2016 11:51

wdoekes approved these changes Oct 6, 2016

View reviewed changes

wdoekes merged commit 8b0ee78 into SIPp:master Oct 6, 2016

jeannotlanglois mentioned this pull request Oct 2, 2019

Gateway RTP/SRTP bit pattern testing support #417

Closed

jeannotlanglois added a commit to jeannotlanglois/sipp that referenced this pull request Aug 23, 2020

[JLTAG] Fix GITHUB regression TC SIPp#259:

9c4bcd6

- UAC side of RTPSTREAM must specify mandatory payload_name parameter

rajeshsingh381 mentioned this pull request Apr 10, 2023

Resolve full path of pcap which starts with '~' #607

Merged

Snom patch: rtp_echo action #259

Snom patch: rtp_echo action #259

Uh oh!

Conversation

pbertera commented Oct 1, 2016

Uh oh!

wdoekes left a comment

Choose a reason for hiding this comment

Uh oh!

wdoekes Oct 2, 2016

Choose a reason for hiding this comment

Uh oh!

wdoekes Oct 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wdoekes Oct 2, 2016

Choose a reason for hiding this comment

Uh oh!

pbertera commented Oct 3, 2016

Uh oh!

wdoekes left a comment

Choose a reason for hiding this comment

Uh oh!

wdoekes Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

wdoekes Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

pbertera left a comment

Choose a reason for hiding this comment

Uh oh!

wdoekes commented Oct 5, 2016

Uh oh!

wdoekes commented Oct 5, 2016

Uh oh!

wdoekes commented Oct 5, 2016

Uh oh!

pbertera commented Oct 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wdoekes Oct 2, 2016 •

edited

Loading