Details

    • Type: New Feature New Feature
    • Status: Closed
    • Severity: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Target Release Version/s: None
    • Component/s: Codecs/codec_speex
    • Labels:
      None
    • Mantis ID:
      10519
    • Regression:
      No

      Description

      the attached patch adds support for wideband speex to asterisk 1.4.10.1

      it relies on the fact that the speex codec (in asterisk or the client) will transparently handle conversion between wide and narrow band speex, so there are no changes to the codec, we just register a new type. asterisk will either (a) allow wideband packets to pass through client<->client (sounds great!), or when actually talking to asterisk (voicemail, prompts etc), it will send or decode as narrowband speex, so quality while talking to asterisk itself is only narrowband (but of course this means minimal changes since asterisk still deals with 8khz audio). this is all transparent to the caller, apart from the lower quality while talking to asterisk itself.

      the only complex part of the patch is about teaching rtp.c and frame.c about 16khz codecs, so that the RTP timestamps are set correctly.

      openwengo, for some odd reason, insist on sending speex wideband as 'G726-64wb', with a different RTP type. this patch adds support for that. it should be trivial to add support for 'speex/16000', for eg ekiga, but that's another patch... and asterisk will have to 'transcode' between them, because the RTP types don't match

      1. 20071106__format_conservation.diff.txt
        25 kB
        Tilghman Lesher
      2. astwb.pat
        14 kB
        Adam Gundy
      3. astwb2.pat
        14 kB
        Adam Gundy

        Activity

        Hide
        Ronald Chan added a comment -

        openwengo, for some odd reason, insist on sending speex wideband as 'G726-64wb', with a different RTP type. this patch adds support for that. it should be trivial to add support for 'speex/16000', for eg ekiga, but that's another patch... and asterisk will have to 'transcode' between them, because the RTP types don't match

        You forgot to attach the patch

        Show
        Ronald Chan added a comment - openwengo, for some odd reason, insist on sending speex wideband as 'G726-64wb', with a different RTP type. this patch adds support for that. it should be trivial to add support for 'speex/16000', for eg ekiga, but that's another patch... and asterisk will have to 'transcode' between them, because the RTP types don't match You forgot to attach the patch
        Hide
        Adam Gundy added a comment -

        depending on the comments on the initial patch (code style, the concept of handling a wideband codec as narrowband internally, and codec name I guess) I might take a stab at adding 'proper' speex/16000 for ekiga and others.

        first, I'd have to persuade ekiga to actually work properly on my machines..

        as a side note, we're getting pretty close to running out of bits for new codecs.. what's the plan for that?

        Show
        Adam Gundy added a comment - depending on the comments on the initial patch (code style, the concept of handling a wideband codec as narrowband internally, and codec name I guess) I might take a stab at adding 'proper' speex/16000 for ekiga and others. first, I'd have to persuade ekiga to actually work properly on my machines.. as a side note, we're getting pretty close to running out of bits for new codecs.. what's the plan for that?
        Hide
        Adam Gundy added a comment -

        second version of the patch uploaded, I removed references to G726_64_WB everywhere but the MIME type used in the SDP. there's no need to propagate OpenWengo's broken idea of speex naming anywhere but the SIP SDP. the codec name is now 'speexwb-wengo', eg 'allow=speexwb-wengo'

        Show
        Adam Gundy added a comment - second version of the patch uploaded, I removed references to G726_64_WB everywhere but the MIME type used in the SDP. there's no need to propagate OpenWengo's broken idea of speex naming anywhere but the SIP SDP. the codec name is now 'speexwb-wengo', eg 'allow=speexwb-wengo'
        Hide
        Jason Parker added a comment -

        For the most part, this looks okay (except that ast_codec_get_rate returns 8000 for g722 .

        If you could figure out how to do this without taking up a new codec number, that would be pretty awesome.

        Show
        Jason Parker added a comment - For the most part, this looks okay (except that ast_codec_get_rate returns 8000 for g722 . If you could figure out how to do this without taking up a new codec number, that would be pretty awesome.
        Hide
        Adam Gundy added a comment -

        I return 8000 for g722 because that is how asterisk currently behaves (ie without the patch), since everything uses hard coded 8kHz. since I can't test g722, I don't want to break it with this patch... and it obviously works currently because people are using it. now you could be correct and asterisk might be generating garbage timestamps for g722 when playing back recorded audio, someone with a g722 phone could test that I guess.

        unfortunately we need a new codec number, because we have to spot the difference between speex/8000 and wengo's speex at 16kHz, otherwise rtp.c puts incorrect timestamps in the packets, and the audio sounds very odd. we'll also need another codec number for true speex/16000 since we need to know which RTP type to put in the packets (it's not the same number as wengo uses, apparently they use 'cirpack'? servers which won't pass unknown types)

        the fact that we're using the same codec code for all three doesn't help much, since it's the RTP code which needs to know the difference, for the type number, and the rate.

        Show
        Adam Gundy added a comment - I return 8000 for g722 because that is how asterisk currently behaves (ie without the patch), since everything uses hard coded 8kHz. since I can't test g722, I don't want to break it with this patch... and it obviously works currently because people are using it. now you could be correct and asterisk might be generating garbage timestamps for g722 when playing back recorded audio, someone with a g722 phone could test that I guess. unfortunately we need a new codec number, because we have to spot the difference between speex/8000 and wengo's speex at 16kHz, otherwise rtp.c puts incorrect timestamps in the packets, and the audio sounds very odd. we'll also need another codec number for true speex/16000 since we need to know which RTP type to put in the packets (it's not the same number as wengo uses, apparently they use 'cirpack'? servers which won't pass unknown types) the fact that we're using the same codec code for all three doesn't help much, since it's the RTP code which needs to know the difference, for the type number, and the rate.
        Hide
        Tilghman Lesher added a comment -

        Okay, two things:

        1. This patch is against 1.4. That version is frozen for new features, so your patch needs to be against trunk.

        2. We have now added native slin16 support to trunk, so your codec translation should provide a native slin16 translator, so that speex16 to g722 can be natively translated.

        Show
        Tilghman Lesher added a comment - Okay, two things: 1. This patch is against 1.4. That version is frozen for new features, so your patch needs to be against trunk. 2. We have now added native slin16 support to trunk, so your codec translation should provide a native slin16 translator, so that speex16 to g722 can be natively translated.
        Hide
        Dmitry Andrianov added a comment -

        To me it looks like significant part of your patch is not related to new codec at all - I'm talking about introduction of AST_FORMAT_AUDIO_MASK and use of it instead of AST_FORMAT_MAX_AUDIO. While I agree it is good thing to do in general, I'm not sure it is a good idea to mix such a "code cleanup" changes with actually new features like addition of new codec in the same svn commit...

        Show
        Dmitry Andrianov added a comment - To me it looks like significant part of your patch is not related to new codec at all - I'm talking about introduction of AST_FORMAT_AUDIO_MASK and use of it instead of AST_FORMAT_MAX_AUDIO. While I agree it is good thing to do in general, I'm not sure it is a good idea to mix such a "code cleanup" changes with actually new features like addition of new codec in the same svn commit...
        Hide
        Tilghman Lesher added a comment -

        dimas: ignore my patch. It was a cleanup patch, inspired by this one.

        Show
        Tilghman Lesher added a comment - dimas: ignore my patch. It was a cleanup patch, inspired by this one.
        Hide
        Tilghman Lesher added a comment -

        No response from reporter.

        Show
        Tilghman Lesher added a comment - No response from reporter.

          People

          • Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development