(Almost) Add support for G.729 Annex B #153
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fbriere
changed the title
Closed
fbriere
added
untested
|
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
force-pushed
the
feature/g729b
branch
from
6194969
to
7ea9801
Compare
Once G.729B support is added, the number of audio samples decoded on output will no longer be linearly correlated to the number of bytes fed on input (or vice-versa for encoding). We will therefore need to tally up that number in a variable, which will act as a return value, as well as an index for the destination buffer.
Once G.729B support is added, the size of a frame will no longer be fixed to 10 bytes. Let's get a head start by moving those hard-coded values into a variable. (Note that the assertion can be removed, since we no longer rely on it.)
Once G.729B support is added, these expressions are about to get a bit more complicated, to account for the different size of SID frames. By switching to a 80:10 ratio right away, we can get a head start and reduce the complexity of the last patch. (Not to mention that this makes things a bit more explicit as a bonus.) For the record, there is a reason behind performing the division before the multiplication, which will become apparent in the final patch. Note that the quotient is guaranteed to be integral, due to the assertions.
Once G.729B support is added, the value of the encoder's `silence` variable will only be known after the encoding has been completed. As per the previous commits, we move it ahead of time to simplify the final patch.
Although we don't yet emit or receive SID frames, this will make it possible to deal with them when the time comes. At this point, everything should be ready on the decoder side; SID frames (those within a G.729 stream, not those sent via 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. On the encoder side, as indicated by the 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).
fbriere
force-pushed
the
feature/g729b
branch
from
7ea9801
to
2f1db18
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
conceal()will end up generating the comfort noise (this is all done transparently by bcg729).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". 😄 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 Add support for the new bcg729 API, introduced in version 1.0.2 #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.)