[asterisk-dev] Strategies for handling RTCP feedback in codec modules

[Joshua Colp]

On Fri, Nov 11, 2016, at 10:09 AM, Lorenzo Miniero wrote:

<snip>

> 
> Hi all,
> 
> I just completed a tentative patch that implements what we discussed. You
> can find it attached (assuming mailman doesn't strip it), and I used the
> Asterisk 14.1.2 as a basis if you want to try it. Can you give a look to
> see if this is in line with what we discussed, and if as an approach it
> is
> in line with the architecture philosophy of Asterisk itself?
> 
> To summarize, this is what I did:
> 
> 
>    1. I added a new ast_frame frametype called AST_FRAME_RTCP (I guess we
>    could re-use AST_FRAME_CONTROL with something like an AST_CONTROL_RTCP
>    subclass, but that's semantics);
>    2. I also added a new callback that translators can implement, called
>    "feedback", that takes a pvt and a frame as parameters just as
>    "framein";
>    as a proof of concept, I implemented a placeholder method for
>    codec_speex
>    that prints the incoming feedback;
>    3. when the RTP stack receives a SR/RR, ast_rtcp_read returns a frame
>    containing a copy of the ast_rtp_rtcp_report object instead of a null
>    frame;
>    4. when __ast_read gets an AST_FRAME_RTCP frame, it checks if a
>    "write"
>    translator exists (as we want to notify the encoder, not the decoder)
>    via
>    ast_channel_writetrans(chan) and calls ast_translate over it to pass
>    over
>    the frame;
>    5. when ast_translate gets an AST_FRAME_RTCP frame, it checks if the
>    "feedback" callback exists for the specified translator, and if it
>    does it
>    sends the frame there;
>    6. the feedback callback in the translator can be used by the codec to
>    parse the ast_rtp_rtcp_report object and handle it accordingly.
> 
> 
> Not sure if this is exactly what you meant or envisaged but, while
> probably
> ugly, it seems to work fine, and does not affect codec modules not
> interested in or aware of the feature.

This is what I envisioned would need to be done and is how I would have
approached it so you're good in my book!

> 
> That said, there are probably a few things to think about. For one, the
> feedback is "routed" by __ast_read automatically, without involving
> channel
> modules themselves. Not sure if this is good or not, but I thought it
> made
> sense to have something that worked automatically no matter the channel,
> and without requiring any update on their code either. Nevertheless,
> there
> may be reasons I'm not aware of for having this go through channel
> modules
> anyway, so let me know if that's indeed an issue.

I don't think it needs to go via them for the purpose of informing the
translator. Eventually it would be useful to explore if we can (and how
to) pass feedback end to end for cases where we aren't transcoding.

> 
> Another potential issue in the patch in its current form is that I'm not
> sure if codec translator chaining can happen in calls and if we should
> care. If, for instance, there's a slin->slin16->codecX because codecX
> only
> has "native" slin16->codecX support but we're bridging to slin, then this
> approach may need be extended, namely to make sure the feedback gets to
> all
> the pieces, or at least to the one that really matters (the last one?) In
> fact, I guess ast_channel_writetrans(chan) would pass the feedback to
> slin->slin16, and not to slin16->codecX which is where the real deal
> should
> happen instead. Not exactly sure on how we could handle this: at the
> moment, the feedback callback doesn't expect the module to return
> anything,
> something we might change to see if we can have this feedback crawl up.
> Alternatively this could be done automatically within translate.c itself,
> who would be aware of such a chain. Again, not even sure this is an issue
> but I thought I'd mention it.

In the case of some codecs the path will indeed involve a resample
before getting to the real codec. I think going through the path (which
is just a linked list) and giving the feedback to each would be best and
shouldn't harm things.

> 
> To conclude, the patch doesn't currently do anything with the feedback. I
> guess that at this stage that's beyond the point, as the main result I
> wanted to achieve was putting up some sort of feedback mechanism in the
> first place. Should you believe I'm on the right way I'll work a bit on
> the
> codec stuff itself (and speex here is a good candidate as it does have
> many
> knobs one can play with).
> 
> As a side note, I anticipated the code is ugly in its current form, and I
> haven't checked if there can be leaks as it is. I've been away from
> Asterisk coding for a while, so fresh eyes can probably help spot those,
> if
> any! :-)

I think there's actually no leaks in it from first glance, so even for
testing you should be fine. Looks like you're on the right track!

-- 
Joshua Colp
Digium, Inc. | Senior Software Developer
445 Jan Davis Drive NW - Huntsville, AL 35806 - US
Check us out at: www.digium.com & www.asterisk.org