Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Capture rtp host and port from sdp #428

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Marc-i2x
Copy link

@Marc-i2x Marc-i2x commented Oct 25, 2019

This allows the creation of RTP streams with ffmpeg instead of pcap replay. Snippet from UAC scenario:

  <!-- Play RTP stream through ffmpeg   -->
  <nop>
    <action>
      <exec command="ffmpeg -nostats -loglevel 0 -re -i ~/arbitrary.wav -ar 8000 -acodec pcm_mulaw -f rtp rtp://[last_media_ip]:[last_media_port] >>/dev/null" />
    </action>
  </nop>

@wdoekes
Copy link
Member

wdoekes commented Oct 30, 2019

Hi!

Could you rebase this against master and fix so all tests succeed?

Thanks!

@Marc-i2x Marc-i2x force-pushed the rtp_destination branch 3 times, most recently from d8fe5d2 to 8f454e5 Compare October 30, 2019 10:05
@Marc-i2x Marc-i2x marked this pull request as ready for review October 30, 2019 10:22
@Marc-i2x
Copy link
Author

Could you rebase this against master and fix so all tests succeed?

done. I also made find_in_sdp availability independent of compile flags as the PR will allow sending RTP without pcap support.

@wdoekes
Copy link
Member

wdoekes commented Nov 8, 2019

Interesting idea. I'm a bit reluctant to allow overloading the last_HEADER for this purpose though, as media_ip is not a header (obviously).

It's already a jungle of small little hacks here and there.

How about you make it [lastsdp_media_ip] instead. Add the appropriate docs, so people will actually know the feature exists. And indent with 4 spaces (*). Thanks! :)

(*) See: 7e62db1 and
astyle --indent=spaces=4 --unpad-paren --pad-header --pad-comma --pad-oper ...

@rkday
Copy link
Contributor

rkday commented Nov 10, 2019

Or perhaps peer_media_ip, peer_media_port, by analogy with peer_tag_param?

@@ -957,6 +955,19 @@ char * call::get_last_header(const char * name)
ERROR("call::get_last_header: Header to parse bigger than %d (%zu)", MAX_HEADER_LEN, strlen(name));
}

if(strcmp(name, "media_ip")==0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it might be slightly better to always call extract_rtp_remote_addr (at line 2820 or so) instead of guarding it with #ifdef RTP_STREAM, and store the host/port off as member variables on the call object, then just use those (in the same way we use the pre-stored peer_tag when handling E_Message_Peer_Tag_Param). That would guarantee that the host/port would be retrieved consistently whether you used built-in RTP streaming or this method.

@Marc-i2x
Copy link
Author

Marc-i2x commented Jan 20, 2020

Interesting idea. I'm a bit reluctant to allow overloading the last_HEADER for this purpose though, as media_ip is not a header (obviously).

It's already a jungle of small little hacks here and there.

How about you make it [lastsdp_media_ip] instead.

Could you guide me how to achieve this? Where would I have to add the respective code?

Add the appropriate docs, so people will actually know the feature exists.

Will do once I find some free time

And indent with 4 spaces (*). Thanks! :)

Will do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants