androidx
Add support for parsing RTP packets with header extension used in RTSP
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.
@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
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.
@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.
@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?
@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.
@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 Hi, sorry for the big delay.
I made a changes we discussed. Now, data about header extension is not saved in RtpPacket and RtpPacket now only reads headers extension. RtpPacketTest has one new unit test parseRtpPacketWithHeaderExtension that tests use case when RTP packet has header extension.
What I think now can be confusing that RtpPacket has hardcoded extension value to false (as I removed saving data, I set it to be as before) and all unit tests in RtpPacketTest check extension to be false including this new unit test which I added. I just want to check that this is fine?
@Kekelic
What I think now can be confusing that RtpPacket has hardcoded extension value to false (as I removed saving data, I set it to be as before) and all unit tests in RtpPacketTest check extension to be false including this new unit test which I added. I just want to check that this is fine?
I understand what you are asking. I think that you could put a comment above the check in your unit test to note that the created RtpPacket object will parse but not save the extension data. You can also change the title of your unit test to note the specific behavior you are intending to test. For example, you can change the name to "parseRtpPacketWithHeaderExtension_createsRtpPacketWithoutHeaderExtension".
@microkatz It seems fine to me now. If there is something else I need to add or change, please let me know. Also do you know when this PR will be merged/included in release?
I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!