<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">2016-11-11 15:19 GMT+01:00 Joshua Colp <span dir="ltr"><<a href="mailto:jcolp@digium.com" target="_blank">jcolp@digium.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, Nov 11, 2016, at 10:09 AM, Lorenzo Miniero wrote:<br>
<br>
<snip><br>
<span class=""><br>
><br>
> Hi all,<br>
><br>
> I just completed a tentative patch that implements what we discussed. You<br>
> can find it attached (assuming mailman doesn't strip it), and I used the<br>
> Asterisk 14.1.2 as a basis if you want to try it. Can you give a look to<br>
> see if this is in line with what we discussed, and if as an approach it<br>
> is<br>
> in line with the architecture philosophy of Asterisk itself?<br>
><br>
> To summarize, this is what I did:<br>
><br>
><br>
</span>>    1. I added a new ast_frame frametype called AST_FRAME_RTCP (I guess we<br>
<span class="">>    could re-use AST_FRAME_CONTROL with something like an AST_CONTROL_RTCP<br>
>    subclass, but that's semantics);<br>
</span>>    2. I also added a new callback that translators can implement, called<br>
<span class="">>    "feedback", that takes a pvt and a frame as parameters just as<br>
>    "framein";<br>
>    as a proof of concept, I implemented a placeholder method for<br>
>    codec_speex<br>
>    that prints the incoming feedback;<br>
</span>>    3. when the RTP stack receives a SR/RR, ast_rtcp_read returns a frame<br>
<span class="">>    containing a copy of the ast_rtp_rtcp_report object instead of a null<br>
>    frame;<br>
</span>>    4. when __ast_read gets an AST_FRAME_RTCP frame, it checks if a<br>
<span class="">>    "write"<br>
>    translator exists (as we want to notify the encoder, not the decoder)<br>
>    via<br>
>    ast_channel_writetrans(chan) and calls ast_translate over it to pass<br>
>    over<br>
>    the frame;<br>
</span>>    5. when ast_translate gets an AST_FRAME_RTCP frame, it checks if the<br>
<span class="">>    "feedback" callback exists for the specified translator, and if it<br>
>    does it<br>
>    sends the frame there;<br>
</span>>    6. the feedback callback in the translator can be used by the codec to<br>
<span class="">>    parse the ast_rtp_rtcp_report object and handle it accordingly.<br>
><br>
><br>
> Not sure if this is exactly what you meant or envisaged but, while<br>
> probably<br>
> ugly, it seems to work fine, and does not affect codec modules not<br>
> interested in or aware of the feature.<br>
<br>
</span>This is what I envisioned would need to be done and is how I would have<br>
approached it so you're good in my book!<br>
<span class=""><br></span></blockquote><div><br></div><div><br></div><div>Hi Joshua,</div><div><br></div><div>thanks for the quick feedback!</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
><br>
> That said, there are probably a few things to think about. For one, the<br>
> feedback is "routed" by __ast_read automatically, without involving<br>
> channel<br>
> modules themselves. Not sure if this is good or not, but I thought it<br>
> made<br>
> sense to have something that worked automatically no matter the channel,<br>
> and without requiring any update on their code either. Nevertheless,<br>
> there<br>
> may be reasons I'm not aware of for having this go through channel<br>
> modules<br>
> anyway, so let me know if that's indeed an issue.<br>
<br>
</span>I don't think it needs to go via them for the purpose of informing the<br>
translator. Eventually it would be useful to explore if we can (and how<br>
to) pass feedback end to end for cases where we aren't transcoding.<br>
<span class=""><br>
><br>
> Another potential issue in the patch in its current form is that I'm not<br>
> sure if codec translator chaining can happen in calls and if we should<br>
> care. If, for instance, there's a slin->slin16->codecX because codecX<br>
> only<br>
> has "native" slin16->codecX support but we're bridging to slin, then this<br>
> approach may need be extended, namely to make sure the feedback gets to<br>
> all<br>
> the pieces, or at least to the one that really matters (the last one?) In<br>
> fact, I guess ast_channel_writetrans(chan) would pass the feedback to<br>
> slin->slin16, and not to slin16->codecX which is where the real deal<br>
> should<br>
> happen instead. Not exactly sure on how we could handle this: at the<br>
> moment, the feedback callback doesn't expect the module to return<br>
> anything,<br>
> something we might change to see if we can have this feedback crawl up.<br>
> Alternatively this could be done automatically within translate.c itself,<br>
> who would be aware of such a chain. Again, not even sure this is an issue<br>
> but I thought I'd mention it.<br>
<br>
</span>In the case of some codecs the path will indeed involve a resample<br>
before getting to the real codec. I think going through the path (which<br>
is just a linked list) and giving the feedback to each would be best and<br>
shouldn't harm things.<br>
<span class=""><br></span></blockquote><div><br></div><div><br></div><div>Ack, I'll update the patch to do that.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
><br>
> To conclude, the patch doesn't currently do anything with the feedback. I<br>
> guess that at this stage that's beyond the point, as the main result I<br>
> wanted to achieve was putting up some sort of feedback mechanism in the<br>
> first place. Should you believe I'm on the right way I'll work a bit on<br>
> the<br>
> codec stuff itself (and speex here is a good candidate as it does have<br>
> many<br>
> knobs one can play with).<br>
><br>
> As a side note, I anticipated the code is ugly in its current form, and I<br>
> haven't checked if there can be leaks as it is. I've been away from<br>
> Asterisk coding for a while, so fresh eyes can probably help spot those,<br>
> if<br>
> any! :-)<br>
<br>
</span>I think there's actually no leaks in it from first glance, so even for<br>
testing you should be fine. Looks like you're on the right track!<br>
<div class="HOEnZb"><div class="h5"><br></div></div></blockquote><div><br></div><div><br></div><div>Thanks! I'll play a bit with codecs then as soon as I have some time. By the way, would you rather me open a jira or something like this for this effort, or is keeping on reporting here fine?</div><div><br></div><div>Lorenzo</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
--<br>
Joshua Colp<br>
Digium, Inc. | Senior Software Developer<br>
445 Jan Davis Drive NW - Huntsville, AL 35806 - US<br>
Check us out at: <a href="http://www.digium.com" rel="noreferrer" target="_blank">www.digium.com</a> & <a href="http://www.asterisk.org" rel="noreferrer" target="_blank">www.asterisk.org</a><br>
<br>
--<br>
______________________________<wbr>______________________________<wbr>_________<br>
-- Bandwidth and Colocation Provided by <a href="http://www.api-digital.com" rel="noreferrer" 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" rel="noreferrer" target="_blank">http://lists.digium.com/<wbr>mailman/listinfo/asterisk-dev</a><br>
</div></div></blockquote></div><br></div></div>