[asterisk-dev] [Code Review] RTP monitoring branch: initial review

Russell Bryant russell at digium.com
Tue Dec 15 03:10:03 CST 2009 

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/394/#review1299
-----------------------------------------------------------


I certainly understand the concept of wanting to handle call recording over RTP.  I think that part is a great idea.  However, I'm not thrilled with this approach to the implementation.

I think we need to evaluate the design of how this feature fits into Asterisk before we dive into any specific code details.  My first reaction for how a feature like this should be implemented is to write a completely new module that implements the audiohook interface.  That is the preferred method for new components to hook into the audio stream.

What are you missing with the audiohooks API that you are able to accomplish with the modification of res_monitor and file format API?

- Russell


On 2009-10-07 16:12:05, Tzafrir Cohen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/394/
> -----------------------------------------------------------
> 
> (Updated 2009-10-07 16:12:05)
> 
> 
> Review request for Asterisk Developers and Tzafrir Cohen.
> 
> 
> Summary
> -------
> 
> I'm working on the RTP monitoring branch:
> 
>   http://svn.digium.com/svn/asterisk/team/tzafrir/monitor-rtp/
>   http://svn.digium.com/svn/asterisk/team/tzafrir/monitor-rtp/doc/monitor-rtp.txt
> 
> Basically this branch intends to allow sending all the recorded
> (Monitor) files to a remote recording server instead of local files.
> This could be recorded by some remote recording server or even by
> a sniffer such as Wireshark.
> 
> At the moment the code basically works and now I want to figure out how
> to best fit it in Asterisk.
> 
> I'm aware that the code could use some improvements, and I have some
> items on my TODO list (at the end of monitor-rtp.txt). But the first
> thing to do is to figure out where things are headed. Ref. the specific
> questions in the description below. 
> 
> The changes involved:
> 
> 1. Generation of dummy SIP calls from res_monitor.c
> 2. Allowing ast_writefile to write to a socket
> 
> 
> Generating Dummy SIP Calls
> --------------------------
> The Recorded calls are sent over the network as faked SIP calls. That
> is: dummy SIP INVITE header, an RTP stream, and a dummy BYE header.
> The INVITE includes SDP that describes the port numbers of the RTP
> streams. The BYE can include more information about the call (such as
> the call duration from the CDR. Though see the TODO list). The recording
> server will not answer us: it listens to network traffic
> (using libpcap). So we basically have a faked, single-sided SIP call.
> 
> Note that there is such a call (and a socket) for each file that we
> would have recorded using res_monitor. Thus we have two such "calls" for
> each recorded channel.
> 
> My main target is the Oreka/Orecx recording server
> (http://oreka.sourceforge.net/ ). However I also test the network
> traffic with wireshark. Both of them identify what I send as "SIP calls"
> (or rather: RTP stream, for wireshark, see the doc), which makes me more
> cdonfident I'm not making up my own crazy format.
> 
> However such a one-sided forked SIP call doesn't fit well inside
> Asterisk.
> 
> As I explain later on, we change main/file.c to allowed passing sockets to
> ast_writefile. But we still have to open the socket somehow.
> At the moment we treat it as a file format:
> 
>   http://svn.digium.com/svn/asterisk/team/tzafrir/monitor-rtp/formats/format_rtp.c
> 
> We can't simply pass the file format "rtp" to the monitoring
> code: The format code only knows how to write, and doesn't handle
> opening files, and we need to change "opening the file" (creating a
> socket).
> 
> Thus extra code is added in res_monitor.c to create sockets for the RTP
> stream and hand it over as the "filename".
> 
> The result, though, is ipv4 networking code inside res_monitor.c . The
> nature of this one-sided faked SIP call is quite different from anything
> else I can think of within Asterisk.
> 
> This is done by the bulk of the changes in res_monitor.c:
> 
>   http://svn.digium.com/svn/asterisk/team/tzafrir/monitor-rtp/res/res_monitor.c
>   https://reviewboard.asterisk.org/r/394/diff/#6
> 
> Any better place for this networking code? Any interface I missed?
> 
> 
> Writing to a socket
> -------------------
> We hook into a mechnism that writes to files and want to convert it to
> sending packets to a remote server. Writing to a file and writing to a
> socket is basically the same (write(2)). Opening is slightly different.
> 
> res_monitor writes to files (calls ast_writefile) and not to file
> descriptors directly.  Our current workaround is to overload the
> interface of ast_writefile:
> 
>   struct ast_filestream *ast_writefile(const char *filename, const char
>         *type, const char *comment, int flags, int check, mode_t mode)
> 
> The parameter 'check' is currently unused: It seems to always get a '0'.
> Hence we decided to use it to mark: if it is not 0, it is
> considered to be a file descriptor. The filename pointer is the
> considered to be a pointer to some state information.
> (And yes, I know file descriptor 0 is a valid number).
> 
> An extra function is then needed to return the "filename" of a
> filestream. I named it ast_filestream_xxx_get_filename() for now.
> 
> Any better way to allow ast_writefile to write to a socket? Any better
> way to allow writing to a socket?
> 
>   http://svn.digium.com/svn/asterisk/team/tzafrir/monitor-rtp/main/file.c
>   https://reviewboard.asterisk.org/r/394/diff/#5
> 
> (Not many changes. ast_writefile's declaration has not changed, but its
> semantics has, as clearly seen in the small change following it)
> 
> 
> Diffs
> -----
> 
>   /trunk/configs/monitor.conf.sample PRE-CREATION 
>   /trunk/doc/monitor-rtp.txt PRE-CREATION 
>   /trunk/formats/format_rtp.c PRE-CREATION 
>   /trunk/include/asterisk/file.h 222649 
>   /trunk/include/asterisk/monitor.h 222649 
>   /trunk/main/file.c 222649 
>   /trunk/res/res_monitor.c 222649 
> 
> Diff: https://reviewboard.asterisk.org/r/394/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Tzafrir
> 
>

More information about the asterisk-dev mailing list