[jitsi-dev] Some thoughts on RawPacket implementation :-)


#1

Hi all,

probably just a minor bug that didn't hurt and probably will never hurt
at all.

In RawPacket please have a look at the following methods and the lines marked
with ***** WD ***** in the code snippet below.

In both case IMHO this should be shifitng by 8, not by 4. It's a conversion
network-to-host order and host-to-network order. Bytes are 8 bits, not 4, aren't they :wink: ?

If it's ok I'll fix it during the next days when I do some more stuff to integrate
the SRTCP enhancements and I need to slightly modify RawPacket (adding just two
new methods) to handle RTCP packets.

Best regards,
Werner

    public int getExtensionLength()
    {
        if (!getExtensionBit())
            return 0;

        //the extension length comes after the RTP header, the CSRC list, and
        //after two bytes in the extension header called "defined by profile"
        int extLenIndex = offset
                        + FIXED_HEADER_SIZE
                        + getCsrcCount()*4 + 2;

        return ((buffer[extLenIndex] << 4) | buffer[extLenIndex + 1]) * 4; ***** WD *****
    }

    public void addExtension(byte[] extBuff, int newExtensionLen)
    {
        int newBuffLen = length + offset + newExtensionLen;
        int bufferOffset = offset;
        int newBufferOffset = offset;
        int lengthToCopy = FIXED_HEADER_SIZE + getCsrcCount()*4;
        boolean extensionBit = getExtensionBit();
        //if there was no extension previously, we also need to consider adding
        //the extension header.
        if (extensionBit)
        {
            // without copying the extension length value, will set it later
            lengthToCopy += EXT_HEADER_SIZE - 2;
        }
        else
            newBuffLen += EXT_HEADER_SIZE;

        byte[] newBuffer = new byte[ newBuffLen ];

....

          }
        // length field counts the number of 32-bit words in the extension
        int lengthInWords = (totalExtensionLen + 3)/4;
        newBuffer[newBufferOffset++] = (byte)(lengthInWords >>4); ***** WD *****
        newBuffer[newBufferOffset++] = (byte)lengthInWords;

....


#2

        return ((buffer[extLenIndex] << 4) | buffer[extLenIndex + 1]) * 4;

Agreed.

        newBuffer[newBufferOffset++] = (byte)(lengthInWords >>4);

Agreed.

Ingo


#3

OK, just had a look. Seems like an oversight indeed and it appears that
the only reason things work currently is because we've never had
extensions that go beyond 60 bytes.

Nice catch Werner!

Emil

–Ě–į 13.10.11 19:09, Werner Dittmann –Ĺ–į–Ņ–ł—Ā–į:

···

Hi all,

probably just a minor bug that didn't hurt and probably will never hurt
at all.

In RawPacket please have a look at the following methods and the lines marked
with ***** WD ***** in the code snippet below.

In both case IMHO this should be shifitng by 8, not by 4. It's a conversion
network-to-host order and host-to-network order. Bytes are 8 bits, not 4, aren't they :wink: ?

If it's ok I'll fix it during the next days when I do some more stuff to integrate
the SRTCP enhancements and I need to slightly modify RawPacket (adding just two
new methods) to handle RTCP packets.

Best regards,
Werner

    public int getExtensionLength()
    {
        if (!getExtensionBit())
            return 0;

        //the extension length comes after the RTP header, the CSRC list, and
        //after two bytes in the extension header called "defined by profile"
        int extLenIndex = offset
                        + FIXED_HEADER_SIZE
                        + getCsrcCount()*4 + 2;

        return ((buffer[extLenIndex] << 4) | buffer[extLenIndex + 1]) * 4; ***** WD *****
    }

    public void addExtension(byte[] extBuff, int newExtensionLen)
    {
        int newBuffLen = length + offset + newExtensionLen;
        int bufferOffset = offset;
        int newBufferOffset = offset;
        int lengthToCopy = FIXED_HEADER_SIZE + getCsrcCount()*4;
        boolean extensionBit = getExtensionBit();
        //if there was no extension previously, we also need to consider adding
        //the extension header.
        if (extensionBit)
        {
            // without copying the extension length value, will set it later
            lengthToCopy += EXT_HEADER_SIZE - 2;
        }
        else
            newBuffLen += EXT_HEADER_SIZE;

        byte[] newBuffer = new byte[ newBuffLen ];

....

          }
        // length field counts the number of 32-bit words in the extension
        int lengthInWords = (totalExtensionLen + 3)/4;
        newBuffer[newBufferOffset++] = (byte)(lengthInWords >>4); ***** WD *****
        newBuffer[newBufferOffset++] = (byte)lengthInWords;

....

--
Emil Ivov, Ph.D. 67000 Strasbourg,
Project Lead France
Jitsi
emcho@jitsi.org PHONE: +33.1.77.62.43.30
http://jitsi.org FAX: +33.1.77.62.47.31