twinkle icon indicating copy to clipboard operation
twinkle copied to clipboard

(Almost) Add support for G.729 Annex B

Open fbriere opened this issue 6 years ago • 1 comments

As bcg729 now supports G.729 Annex B (since 1.0.2), it should be possible (once #104 has been merged) for Twinkle to do the same, at least in theory.

This PR lays most of the groundwork necessary to handle SID frames and DTX properly. (VAD is obviously none of our concern, and CNG is done automatically through the same process as frame erasure concealment.)

To quote the state of affairs from the last commit:

  • Everything should be ready on the decoder side: SID frames (notwithstanding RFC 3389) will be decoded as expected, and untransmitted frames will be treated as erased frames, where conceal() will end up generating the comfort noise (this is all done transparently by bcg729).
  • On the encoder side, as indicated by the code comments, two issues remain: one regarding SID frames (if they do not happen to be at the end of the payload), and one with an empty payload of only untransmitted frames (if it ends up being transmitted nonetheless as a NAT keepalive).

I think I could tackle the second issue by myself, but the first one is beyond my abilities. (Can we afford to mercilessly drop the other samples after a SID frame?)

Some additional points:

  • Despite what it may look like, I actually have no experience or expertise in any of this stuff. (I'm merely a fast learner, that's all.) Therefore, it's quite possible that I got a few (or many) things wrong. Please let me know of any mistake I may have made.

  • ~~It goes without saying that none of this has ever been tested, aside from confirming that "yup, it does compile just fine". :smile: Although the Annex B aspect isn't functional yet, it would be nice to confirm that I did not wreck the common G.729 code by mistake. If someone could check and make sure, this would be most welcome!~~

  • The first two commits are merely a copy of #152, on which this PR depends.

  • The other commits prior to the last one may appear needlessly trivial; their role is merely to lessen the size and complexity of the last commit. I tried to make the job of whoever is going to review this PR as easy and painless as I could manage.


(Filing this as a draft PR, until #104 is merged.)

fbriere avatar Jul 07 '19 22:07 fbriere

As I stated in #104, I've now had a chance to actually test G.729, and this code seems to work fine, in that it does not have any impact when using plain G.729A. (Any confirmation would always be welcome, of course.)

fbriere avatar Sep 08 '19 14:09 fbriere