2016-11-11 15:19 GMT+01:00 Joshua Colp <[email protected]>: > 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 >
-- _____________________________________________________________________ -- 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
