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

Fri Nov 11 17:36:38 CST 2016

2016-11-11 15:19 GMT+01:00 Joshua Colp <jcolp at digium.com>:

> 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!
>
>

Hi Joshua,

thanks for the quick feedback!

> >
> > 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.
>
>

Ack, I'll update the patch to do that.

> >
> > 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!
>
>

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?

Lorenzo

> --
> 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
>
> --
> _____________________________________________________________________
> -- Bandwidth and Colocation Provided by http://www.api-digital.com --
>
> asterisk-dev mailing list
> To UNSUBSCRIBE or update options visit:
>    http://lists.digium.com/mailman/listinfo/asterisk-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20161112/f635c9c7/attachment.html>