Add support for parsing RTP packets with header extension used in RTSP

[Kekelic]

RTP packets that contain header extension should be properly handled. Something about that is available in RFC3550, Section 5.3.1.

This fix only reads the header extension from the RTP packet if exists. It is done before parsing the remaining payload data, so that payload data can be handled properly.

Without this fix, Player that runs RTSP video will fail under the error: RTP H264 packetization mode [0] not supported. This is thrown in RtpH264Reader which consumes payload and chooses rtpH264PacketMode depending on the first byte of payload. So if payload contains the header extension, reader will fail.

[microkatz]

@Kekelic

Apologies for the delay. Thank you for creating a pull request to the project!

May I ask if you create a unit test in RtpPacketTest for your change? Also I think your code reads part of the extension header but does not actually read the extension. Your code I think would have issue with an header extension with a non-zero sized payload.

[Kekelic]

@Kekelic

Apologies for the delay. Thank you for creating a pull request to the project!

May I ask if you create a unit test in RtpPacketTest for your change? Also I think your code reads part of the extension header but does not actually read the extension. Your code I think would have issue with an header extension with a non-zero sized payload.

@microkatz
I'm glad you answered.

Probably I could create it if needed.
Yes, that's what i was trying to emphasize. Code only read that header from RTP packet and don't do anything with it anymore. But if it doesn't read, video will fail.
Not sure why u think that. I have a system with video stream which produces RTP packets with extension and only this fix make it to work. Also i tested this fix with a video stream that produces RTP packets without extension and it worked same as before this fix.

[microkatz]

@Kekelic

Yes, it would be great if you added a unit test that showcases your code fixing the present issue. It may require you to create an RtpH264ReaderTest file. There is currently an RtpH263ReaderTest file, https://github.com/androidx/media/blob/release/libraries/exoplayer_rtsp/src/test/java/androidx/media3/exoplayer/rtsp/reader/RtpH263ReaderTest.java. In this test you can showcase how an H264 Rtp packet containing a header extension fails in parsing.

Is there a reason that your video system is using the header extensions? What data is it providing?

Apologies, it is not that you are expecting an empty extension, it is that you not taking into account that the extension's length is variable. In RFC3550, Section 5.3.1. It describes the extension as the following: "The header extension contains a 16-bit length field that counts the number of 32-bit words in the extension, excluding the four-octet extension header (therefore zero is a valid length)." Below is a diagram from the RFC spec of the extension header:

 0                   1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |      defined by profile       |           length              |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                        header extension                       |
   |                             ....                              |

Your extension parsing code is as follows:

//Extension.
 if (hasExtension){
   byte[] extension = new byte[EXTENSION_SIZE];
   packetBuffer.readBytes(extension, 0, EXTENSION_SIZE);
 }

Your code is just reading 16 bytes of header extension data always. Your code should parse the extension's length and then read however many bytes of the header extension payload.

The reason why your system works with the change is that you may specifically have a 16 byte header extension(including 12 byte header extension payload). It works with video streams without the header extension because then hasExtension is false and are not reading 16 bytes.

[Kekelic]

@microkatz

I will try to make it when i get time. If I struggle with it, I will contact you.

There is no reason, system is made to use server which produces that kind of stream and it is unlikely that it can be changed. I think it uses live555 - RTSP server application or something like that.
I am sure that there is a lot of people faced same error at least when I researched about it. Some of them are probably because of same (header extension error) and the rest are for something else. So this fix might be useful to them.

I understand what you want to say. But I would have to research more about that header extension to find out how to read length of that header from packet. At least it works when length is 16 bytes, if the length is less the 16 it will probably fail same as without this fix. Do you know how to determinate length of header?

[microkatz]

@Kekelic

Definitely reach out if you are struggling!

You can look at the earlier code in that parse(ParsableByteArray packetBuffer) and the diagram in RFC 3550 Section 5.1 - RTP Fixed Header Fields for examples on how to parse the data field.

The extension looks like the following:

 0                   1                   2                   3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |      defined by profile       |           length              |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |                        header extension                       |
   |                             ....                              |

Basically you either want to read the first Int(4 bytes) and mask against 0xFF to get the length. Or just read the first short(two bytes) then read another short(2 bytes) as the length. The second way would be as follows(also this is rough, with comments for you, extraneously descriptive parameter names, etc.):

//Extension.
 if (hasExtension){
   int headerExtensionProfileData = packetBuffer.readShort();
   int headerExtensionPayloadLength = packetBuffer.readShort(); // Number of 32 bit words in extension payload
   if (headerExtensionPayloadLength != 0) {
       byte[] extensionPayload = new byte[headerExtensionPayloadLength * 4];
       packetBuffer.readBytes(extensionPayload, 0, headerExtensionPayloadLength*4);
   }
 }

What would be very helpful for your unit test is if you took the example RTP from the H264 stream that fails at the moment and use that. Then you could debug in the parsing code and make sure the length and payload data matches as you expect.

[Kekelic]

@microkatz

Great explained and I like your descriptive parameter names.

Both solutions worked, but I made it work with a first solution with small changes. Take a look.

Also, I made 2 unit tests with RTP data that failed without this fix as you told me, but it didn't require me to make RtpH264ReaderTest file. So I'm not sure if that's only what you are looking for. But I covered checking for payload data matches as expected and to build packet matches packet data which required me to edit a RtpPacket little bit more.

[microkatz]

Is the & with 0xFF necessary? I think that would just return headerExtension[N]?

[microkatz]

Ah. I see you may have used the 0xFF mapping due to java sign handling. You could use some guava method? Like https://guava.dev/releases/snapshot-jre/api/docs/com/google/common/primitives/Ints.html#fromBytes(byte,byte,byte,byte).
Ints.fromBytes(0, 0, headerExtension[2], headerExtension[3])

[Kekelic]

Actually that I was planning to do firstly, but didn't know could I include Ints in file.

[Kekelic]

But if I will remove saving data about header extension? I can go with your solution:

int headerExtensionProfileData = packetBuffer.readShort();
int headerExtensionPayloadLength = packetBuffer.readShort();

[microkatz]

I think the name of this constant field is misleading? Its not the size of the header extension but the size of the first bit of it that contains the "profile related value" and the extension length. At least that is what you are using it for.

[microkatz]

nit: Add space between '}' and 'else'

[microkatz]

I don't think its necessary to save the header extension data. None of the RTSP readers are utilizing this data at the moment and its just adding cruft to the class. I think its enough to just parse the extension if it exists as to not have the readers choke on the payload.

[Kekelic]

@microkatz

True, good suggestions.
So I can also remove saving "headerExtension" (thats's also misleading) and extensionPayload data? Which means I can also delete second Unit test I added buildRtpPacketWithHeaderExtension_matchesPacketData. And If I don't save hasExtension I will not check it in first Unit test I added parseRtpPacketWithHeaderExtension ?

[microkatz]

The reason why I mentioned the RtpH264ReaderTest was to have a unit test parsing an RTP packet with both a header extension and a payload. In this case, without your changes, the Rtp parsing logic would incorrectly group the headerextension into the general payload. If you can create a unit test in RtpPacketTest that exemplifies this then that is fine as well.

[Kekelic]

@microkatz

Yes that's what i did with parseRtpPacketWithHeaderExtension unit test added in RtpPacketTest. I pulled out failed packet without this fix that contains both header extension and payload.
So do I need to remove saving data about header extension in RtpPacket which I added?