<div dir="ltr">Dear all,<div><br></div><div>I've just uploaded the patch. I've also added a few comments to the bug for further discussion. Let me know if anything else is required of me at this point.</div><div><br>
</div><div>Cheers,</div><div>Lorenzo</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2013/7/3 Lorenzo Miniero <span dir="ltr"><<a href="mailto:lminiero@gmail.com" target="_blank">lminiero@gmail.com</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">2013/7/1 Matthew Jordan <span dir="ltr"><<a href="mailto:mjordan@digium.com" target="_blank">mjordan@digium.com</a>></span><br>
<div class="gmail_extra"><div class="gmail_quote"><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote"><div><div>On Mon, Jul 1, 2013 at 8:20 AM, Lorenzo Miniero <span dir="ltr"><<a href="mailto:lminiero@gmail.com" target="_blank">lminiero@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">2013/7/1 Tzafrir Cohen <span dir="ltr"><<a href="mailto:tzafrir.cohen@xorcom.com" target="_blank">tzafrir.cohen@xorcom.com</a>></span><br>
<div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div>On Mon, May 27, 2013 at 09:35:21PM +0200, Lorenzo Miniero wrote:<br>
> 2013/5/27 Tzafrir Cohen <<a href="mailto:tzafrir.cohen@xorcom.com" target="_blank">tzafrir.cohen@xorcom.com</a>><br>
><br>
> > On Mon, May 27, 2013 at 04:09:08PM +0200, Lorenzo Miniero wrote:<br>
> > > Dear all,<br>
> > ><br>
> > > I've just published the patch on github:<br>
> > ><br>
> > > <a href="https://github.com/meetecho/asterisk-opus" target="_blank">https://github.com/meetecho/asterisk-opus</a><br>
> > ><br>
> > > The README should be quite self explainatory, but if you need any<br>
> > > additional info feel free to ask me.<br>
> > > Any feedback will be more than welcome!<br>
> ><br>
> > $ diffstat asterisk-opus/asterisk_opus+vp8.diff<br>
> > build_tools/<a href="http://menuselect-deps.in" target="_blank">menuselect-deps.in</a> | 1<br>
> > channels/chan_sip.c | 91 ++++++-<br>
> > codecs/codec_opus.c | 529<br>
> > +++++++++++++++++++++++++++++++++++++++++<br>
> > codecs/ex_opus.h | 35 ++<br>
> > <a href="http://configure.ac" target="_blank">configure.ac</a> | 3<br>
> > formats/format_vp8.c | 195 +++++++++++++++<br>
> > include/asterisk/format.h | 4<br>
> > main/channel.c | 2<br>
> > main/format.c | 16 +<br>
> > main/frame.c | 38 ++<br>
> > main/rtp_engine.c | 6<br>
> > <a href="http://makeopts.in" target="_blank">makeopts.in</a> | 3<br>
> > res/res_rtp_asterisk.c | 42 +++<br>
> > 13 files changed, 960 insertions(+), 5 deletions(-)<br>
> ><br>
> > Any problem with including the format parts of this patch into Asterisk?<br>
> > Asterisk has limited support for H.264 video. I don't suppose Asterisk<br>
> > comes with a license for a H.264 video playback (let alone encoding).<br>
> ><br>
> > At first glance, the following parts of the patch seem to be related to<br>
> > formats, rather than codecs:<br>
> ><br>
> > channels/chan_sip.c | 91 ++++++-<br>
> > formats/format_vp8.c | 195 +++++++++++++++<br>
> > include/asterisk/format.h | 4<br>
> > main/channel.c | 2<br>
> > main/format.c | 16 +<br>
> > main/rtp_engine.c | 6<br>
> > res/res_rtp_asterisk.c | 42 +++<br>
> ><br>
> > Would a patch / review of those parts by Lorenzo be welcomed?<br>
> ><br>
> ><br>
><br>
> Actually all the files have stuff related to both codecs: so the code<br>
> related to Opus should be stripped from chan_sip, format, rtp_engine, etc.<br>
> first in order to have them refer to VP8 only. The format_vp8.c file itself<br>
> is pretty much a clone of the H.264 one, so if this one's ok I guess that<br>
> one will be too.<br>
<br>
</div></div>In case I was not clear enough: I believe (and I suppose Matt's answer<br>
clarified it) that those parts of the patch can be merged into Asterisk<br>
as there's no reasonable fear of abusing any patents with them.<br>
<br>
If you agree with me, please follow up with the bug I opened:<br>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-21981" target="_blank">https://issues.asterisk.org/jira/browse/ASTERISK-21981</a><br>
and attach the partial patch.<br>
<br>
The nice thing about it is that it includes almost all of the patching<br>
needed to actual Asterisk code (besides the changes to frame.c . I<br>
wonder if those are actually needed). Apart from that you only have<br>
codec modules and changes to the build system required to build them.<br>
<br>
(if this bug is a duplicate: sorry for the noise. I didn't see any<br>
progress here)<br>
<br>
If this is to be merged before the Asterisk 12 freeze, we don't have<br>
much time.<br>
<div><br></div></blockquote><div><br></div><div><br></div></div></div>The code in frame.c is only needed if Asterisk needs to be aware of how many samples an Opus RTP packet actually includes. Considering the patch would be targeted to only add passthrough support for the codec, I guess the changes in frame.c can be safely ignored.<div>
<br></div><div>I think format_vp8.c can be removed as well: in fact, VP8 passthrough support does not strictly require that file, which is only needed in case you also want to provide ability to read/write VP8 files (e.g., for announcements), something that code doesn't currently provide anyway (it's just a copy of format_h264.c acting as a placeholder).</div>
<div><br></div><div>I'm out of the lab today so I won't be able to upload the partial patch until tomorrow, though, is it OK anyway considering the Asterisk 12 freeze deadline you mentioned?</div><span><font color="#888888"><div>
<br></div></font></span></div></div></div></blockquote><div><br></div></div></div><div>I'd put up for review all parts of the code that do not perform transcoding. If additional work in frame.c is needed to know how many samples *any* RTP packet contains, then I'd include that as well (the fact that some codecs support varying sample rates is not strictly unique to any one codec). The same could be true for some portion of Asterisk that reacts to changes in sample rates. Infrastructure support is entirely appropriate for Asterisk 12; the easier it makes it for someone to potentially add future enhancements to Asterisk the better.</div>
<div><br></div><div>I'd consider limited support of pass-through for VP8 to be a candidate as well.<br></div><div><br></div><div>When you have those portions ready, attach them to the issue Tzafrir has made. Feature freeze for Asterisk 12 is July 31st, but all that means is that the patch has to be on the issue and posted to Review Board. We can help get that second step going once it is on the issue.</div>
<div><br></div><div>Thanks,</div><div><br></div><div>Matt</div><div> </div></div></div></div></blockquote><div><br></div></div></div>I prepared the patch but it seems I can't upload it yet. I had to sign the agreement again, even though I thought I already had it done a long time ago (but this was before Jira, so something may have changed in the meanwhile). As soon as I'm accepted I'll post the patch on the issue page.<span class="HOEnZb"><font color="#888888"><div>
<br></div><div>Lorenzo</div><div> </div></font></span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
<div dir="ltr"><div class="gmail_extra">
<div>-- <br><div dir="ltr"><div>Matthew Jordan<br></div><div>Digium, Inc. | Engineering Manager</div><div>445 Jan Davis Drive NW - Huntsville, AL 35806 - USA</div>
<div>Check us out at: <a href="http://digium.com" target="_blank">http://digium.com</a> & <a href="http://asterisk.org" target="_blank">http://asterisk.org</a></div></div>
</div></div></div>
<br></div><div class="im">--<br>
_____________________________________________________________________<br>
-- Bandwidth and Colocation Provided by <a href="http://www.api-digital.com" target="_blank">http://www.api-digital.com</a> --<br>
<br>
asterisk-dev mailing list<br>
To UNSUBSCRIBE or update options visit:<br>
<a href="http://lists.digium.com/mailman/listinfo/asterisk-dev" target="_blank">http://lists.digium.com/mailman/listinfo/asterisk-dev</a><br></div></blockquote></div><br></div></div>
</blockquote></div><br></div>