search

jitsi / libjitsi

Advanced Java media library for secure real-time audio/video communication.
Apache License 2.0
628 stars 281 forks source link

AMR: Encoder added a Null byte to Frame content. #496

Closed traud closed 4 years ago

traud commented 4 years ago

Many AMR-WB implementations check the length of the frame, and reject frames with wrong lengths, even if only one frame is in the RTP packet. The solution is to use the right length which is dstLen - 1 = srcLen = the length has not changed.

fixes traud/asterisk-amr#17

jitsi-jenkins commented 4 years ago

Hi, thanks for your contribution! If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

bgrozev commented 4 years ago

If the correct length is srcLen why do we allocate an extra byte in dst in the first place?

traud commented 4 years ago

Because of that dstI++.

In the destination, Codec Mode Request (CMR), the Another Frame Indicator (F), the Frame Type Index (FT), and the Frame Quality Indicator (Q) get pre-pended. Furthermore, the first byte in the source gets removed. Consequently, everything else is moved 6 bits. However, frame type 8 (of AMR-WB) has six Stuffing Bits. Consequently, the last operation dst[srcLen] = (byte) (0x03 & s) << 6); does nothing.

There are two alternatives: (A) Simply cut off that last byte but picky-pack it everywhere (which is OK because we are in Java with its memory management) or (B) change the bit-shifting loop to not do the very last operation on the very last byte. In the latter case, int dstLen = srcLen; would be correct. I went for alternative A. Another alternative C would be to rewrite the whole packetize(.) and shift in place.

Actually, the current packetize(.) is not perfect because it is not agnostic about the frame type. The whole current implementation is hard coded just for frame type 8 although the variable bitRate suggests differently. This allows me to leverage a coincidence: dstLen stays srcLen. Actually, the semantically correct value for the resulting length would be a hard coded value of 61, because some frame types get bigger (in AMR-WB, just frame type 2), others keep the length. Actually, actually, such an agnostic code would even work for AMR (Narrow-Band), see this C code…

bgrozev commented 4 years ago

Sorry for the slow response on this.

Your fix makes sense, I'm just worried that it makes the code even more confusing than it already is. Can you please add a one line comment to the effect of "we're intentionally keeping the original length and discarding dstLen"?

traud commented 4 years ago

it makes the code even more confusing than it already is

Instead for a comment, I made it correct. Now, packetize(.) handles all frame types created by the encoder; it is agnostic about the frame type index. With this, a user is able to change the frame type via bitRate as expected. Furthermore, this change allows SID frames created for DTX because of VAD. With these changes, VAD can be enabled via one line of code. I am going to post that in another Pull Request. Successfully tested here with my Sangoma Asterisk module, Google Android (native SIP client), and two Nokia Mobile Phone series.

traud commented 4 years ago

Is anything blocking this?