[asterisk-dev] SIP/SDP: ptime in translation module?

[Matthew Jordan]

On Sun, Oct 11, 2015 at 3:09 PM, Alexander Traud
<pabstraud at compuserve.com> wrote:
>> 2) ... codec modules are fed [with] frames already ...
>
> Uh. That is a bit too abstract, yet: When do I tag the frames exactly?
>
> ast_rtp_codecs_get_framing(ast_rtp_instance_get_codecs(instance))
> provides the expected packetization value in res/res_rtp_asterisk.c, yes.
> However, the transcoding module needs that value while creating the frame,
> somewhere in the call chain: ast_write -> ast_translate -> *frameout.

You can only get the packetization in one of two ways:
* The channel drivers inform the codec instance during negotiation
and/or when the channel is created. I'm not sure of any way to do this
without modifying the codec core/channel core in some fashion. I will
say that a format is still not the appropriate place for it.
  (1) Formats are immutable, more on that below.
  (2) Packetization is a session level attribute, not a media format
attribute. It is semantically wrong to store it in that location.
* The RTP instance knows of the packetization. It can request the
packetization via ast_rtp_codecs_get_framing; it can add that to the
frames it creates in ast_rtp_read. A read frame will be passed up to
the codec translator with the correct packetization; that should set
the packetization in the codec "on the fly". Realistically, this will
result in one generated frame with the "default" packetization.
Practically, I would expect that to not matter.

> res/res_rtp_asterisk.c:ast_rtp_write is called after the above chain.
> Therefore, the frame would be tagged with the current packetization value
> too late. By the way, I might have created a misunderstanding: While reading
> (ast_rtp_read, ast_read), the AMR transcoding module does not care about
> that value because the remote device could send/use a different value
> anytime - actually the module does *not* need to know the value while
> reading AMR, because the module able to extract the amount of frames from
> the payload data itself. It is just about writing.

Hm. I would expect packetization to be negotiated the same on both
sides. If not, then yes, the frame approach won't work.

> My second issue with solution #2: Because ptime is needed in *frameout, the
> place where the frames are created, I do not know, which frames to tag.
> There is one frame in the private structure of the translation itself
> (ast_trans_pvt). Again yet, I was not able to find a place in code where
> that meets the RTP packetization value. Mhm.

I don't understand this point; you'll need to point more closely to
where in particular you are talking about.

>> formats in 13+ are immutable by convention
>
> Since Asterisk 13, there is a format cache, which is used heavily, yes. I
> guess, this is meant by immutable. Or? However, this is not used everywhere,

Immutable means you MUST NOT change a format instance once it is
created. You *will* cause something horrible to happen.

Formats have no lock; they are accessed by multiple threads
concurrently. If you modify one, you *may* modify it for multiple
channels. You may crash. You may have monkeys fly out of your Asterisk
system.

Please don't do it. :-)

There are safe ways to duplicate a format, modifying it safely in the
process of duplication. This, however, is generally not a great
solution, as it requires a reasonable amount of overhead.

> see <http://github.com/traud/asterisk-amr> capability_cached_format.patch
> (Is that a patch not for a new feature but a bug?). At the end, I saw a lot
> of different ast_formats pointers in my res/res_format_attr_*.c, which had
> to be reduced because:
>  I) The AMR decoder and the encoder require access to the fmtp line(s).
>     Actually, the negotiated result of both lines is required.
> II) The decoder has to talk to the encoder (only that way is a must) because
>     the remote device is able to request a AMR-mode change for the encoder.

I haven't looked at the patch, but if you're changing the contract of
what occurs in the format modules and the formats, I would suspect
that you've introduced a major bug in the format core.

You can't modify formats that are shared between channels.

> Therefore, ast_format_interface->format_get_joint creates a new ast_format.
> Consequently, I meet a unique entity when
> ast_format_cap_get_format_framing() is called. That is the place where I tag
> "my" ast_format/ast_format_interface with the current packetization value
> (possibility negotiated; sip.conf:autoframing=yes). That is very much your
> solution #1. Any comments?
>

I think any modification to ast_format is the wrong approach.
(1) It is semantically incorrect, storing a media session level
attribute on a specific format within that session, while the
packetization applies to all formats within that session.
(2) It goes against the contract ast_format tries hard to enforce.
You'll either kill performance by requiring a large amount of new
ast_format structures when you don't want them, or you'll cause major
threading issues. Either channels will be running with a packetization
they didn't want, or you may just crash. Or both.

If setting it on the ast_frame doesn't work - and that's fair - and
packetization is a media session attribute - why not just store it on
the ast_translator on the channel? Frankly, it is the closest to
something keeping track of the state of the media session that I can
think of, and that's really where packetization should probably go.

-- 
Matthew Jordan
Digium, Inc. | Director of Technology
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: http://digium.com & http://asterisk.org