Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1c73cb1
Change RtpPacket class to parse header extension if exists
Kekelic Mar 26, 2024
9d75a51
Format changes
Kekelic Mar 26, 2024
d245a9c
Add reading length of extension payload from header extension in RtpP…
Kekelic Apr 29, 2024
2c3c1a9
Remove saving unnecessary header extension data
Kekelic Jul 8, 2024
8e10976
Fix documentation and improve readability of unit test with rtpDataWi…
Kekelic Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 64 additions & 1 deletion libraries/exoplayer_rtsp/src/main/java/androidx/media3/exoplayer/rtsp/RtpPacket.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,15 @@ public final class RtpPacket {
/** Builder class for an {@link RtpPacket} */
public static final class Builder {
private boolean padding;
private boolean extension;
private boolean marker;
private byte payloadType;
private int sequenceNumber;
private long timestamp;
private int ssrc;
private byte[] csrc = EMPTY;
private byte[] headerExtension = EMPTY;
private byte[] extensionPayload = EMPTY;
private byte[] payloadData = EMPTY;

/** Sets the {@link RtpPacket#padding}. The default is false. */
Expand All @@ -78,6 +81,13 @@ public Builder setPadding(boolean padding) {
return this;
}

/** Sets the {@link RtpPacket#extension}. The default is false. */
@CanIgnoreReturnValue
public Builder setExtension(boolean extension) {
this.extension = extension;
return this;
}

/** Sets {@link RtpPacket#marker}. The default is false. */
@CanIgnoreReturnValue
public Builder setMarker(boolean marker) {
Expand Down Expand Up @@ -122,6 +132,26 @@ public Builder setCsrc(byte[] csrc) {
return this;
}

/**
* Sets {@link RtpPacket#headerExtension}. The default is an empty byte array.
*/
@CanIgnoreReturnValue
public Builder setHeaderExtension(byte[] headerExtension) {
checkNotNull(headerExtension);
this.headerExtension = headerExtension;
return this;
}

/**
* Sets {@link RtpPacket#extensionPayload}. The default is an empty byte array.
*/
@CanIgnoreReturnValue
public Builder setExtensionPayload(byte[] extensionPayload) {
checkNotNull(extensionPayload);
this.extensionPayload = extensionPayload;
return this;
}

/** Sets {@link RtpPacket#payloadData}. The default is an empty byte array. */
@CanIgnoreReturnValue
public Builder setPayloadData(byte[] payloadData) {
Expand All @@ -143,6 +173,7 @@ public RtpPacket build() {
public static final int MIN_SEQUENCE_NUMBER = 0;
public static final int MAX_SEQUENCE_NUMBER = 0xFFFF;
public static final int CSRC_SIZE = 4;
public static final int HEADER_EXTENSION_SIZE = 4;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/** Returns the next sequence number of the {@code sequenceNumber}. */
public static int getNextSequenceNumber(int sequenceNumber) {
Expand Down Expand Up @@ -186,6 +217,12 @@ public static int getPreviousSequenceNumber(int sequenceNumber) {
/** The RTP CSRC fields (Optional, up to 15 items). */
public final byte[] csrc;

/** The RTP header extension fields (Optional, 32 bits). */
public final byte[] headerExtension;

/** The RTP extension payload fields (Optional). */
public final byte[] extensionPayload;

public final byte[] payloadData;

/**
Expand All @@ -205,6 +242,7 @@ public static RtpPacket parse(ParsableByteArray packetBuffer) {
byte version = (byte) (firstByte >> 6);
boolean padding = ((firstByte >> 5) & 0x1) == 1;
byte csrcCount = (byte) (firstByte & 0xF);
boolean hasExtension = ((firstByte & 0x10) >> 4) == 1;

if (version != RTP_VERSION) {
return null;
Expand Down Expand Up @@ -233,6 +271,24 @@ public static RtpPacket parse(ParsableByteArray packetBuffer) {
csrc = EMPTY;
}

//Extension.
byte[] headerExtension;
byte[] extensionPayload;
if (hasExtension) {
headerExtension = new byte[HEADER_EXTENSION_SIZE];
packetBuffer.readBytes(headerExtension, 0, HEADER_EXTENSION_SIZE);
int extensionPayloadLength = (headerExtension[2] & 0xFF) << 8 | (headerExtension[3] & 0xFF);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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])

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

if (extensionPayloadLength != 0) {
extensionPayload = new byte[extensionPayloadLength * 4];
packetBuffer.readBytes(extensionPayload, 0, extensionPayloadLength * 4);
}else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

extensionPayload = EMPTY;
}
}else {
headerExtension = EMPTY;
extensionPayload = EMPTY;
}

// Everything else will be RTP payload.
byte[] payloadData = new byte[packetBuffer.bytesLeft()];
packetBuffer.readBytes(payloadData, 0, packetBuffer.bytesLeft());
Expand All @@ -246,6 +302,9 @@ public static RtpPacket parse(ParsableByteArray packetBuffer) {
.setTimestamp(timestamp)
.setSsrc(ssrc)
.setCsrc(csrc)
.setExtension(hasExtension)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ?

Copy link
Contributor

@microkatz microkatz Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

.setHeaderExtension(headerExtension)
.setExtensionPayload(extensionPayload)
.setPayloadData(payloadData)
.build();
}
Expand All @@ -264,14 +323,16 @@ public static RtpPacket parse(byte[] buffer, int length) {

private RtpPacket(Builder builder) {
this.padding = builder.padding;
this.extension = false;
this.extension = builder.extension;
this.marker = builder.marker;
this.payloadType = builder.payloadType;
this.sequenceNumber = builder.sequenceNumber;
this.timestamp = builder.timestamp;
this.ssrc = builder.ssrc;
this.csrc = builder.csrc;
this.csrcCount = (byte) (this.csrc.length / CSRC_SIZE);
this.headerExtension = builder.headerExtension;
this.extensionPayload = builder.extensionPayload;
this.payloadData = builder.payloadData;
}

Expand Down Expand Up @@ -310,6 +371,8 @@ public int writeToBuffer(byte[] target, int offset, int length) {
.putInt((int) timestamp)
.putInt(ssrc)
.put(csrc)
.put(headerExtension)
.put(extensionPayload)
.put(payloadData);
return packetLength;
}
Expand Down
63 changes: 63 additions & 0 deletions libraries/exoplayer_rtsp/src/test/java/androidx/media3/exoplayer/rtsp/RtpPacketTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,29 @@ public final class RtpPacketTest {
Arrays.copyOfRange(
rtpDataWithLargeTimestamp, RtpPacket.MIN_HEADER_SIZE, rtpDataWithLargeTimestamp.length);

/*
10.. .... = Version: RFC 1889 Version (2)
..0. .... = Padding: False
...0 .... = Extension: True
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the extension is true then this bit should be a 1.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, it was an error in copying the code. Also, I changed 1 to 0 at Marker bit.

.... 0000 = Contributing source identifiers count: 0
1... .... = Marker: False
Payload type: DynamicRTP-Type-96 (96)
Sequence number: 61514
Timestamp: 2000000000
Synchronization Source identifier: 0x35ff2773 (905914227)
extension: 00a20003a94f000062150100cbca0100
Payload: 7c85b841bc439048000834f1a6943c00040bf038ee4de07acb6d…
*/
private final byte[] rtpDataWithHeaderExtension =
getBytesFromHexString("9060f04a7735940035ff277300a20003a94f000062150100cbca01007c85b841bc439048000834f1a6943c00040bf038ee4de07acb6dc67cb44716d9b61800600f1041214b121de2af09a8063ff2d88fedf7f565eafb9c44412a8e5a247d0ac76a6a8566a6ff593f9711114b6c625ca1363950ae8524a37c75c509a806833fd4bbeb6dda6db697aef12d709a80910e522bb3e2e793eb3c37995c4429448f2ba8b16bcb825ca11c3dffb3ff50ba8c5a3e5ffaff978b7e1350037d7dce4ddc906dbfff50ba8069ace7a9df442fffde2afc26a004b076dd611ebedb8bf15fd9596fc47e03e1008a32013d454401f8590ea42b6a67a3cf4da90aa006aca053283cf09c0e42c000444519045f3a0002c4c0e5e0f81e8316c5b16cbf3737c462bd5f87cc66b3e508a10128ac18d5656c78a6e293f10e0252c1819c2040fc5b16fe222296c4da247284ae892a16de65db7236a9bab718da108d05f09bf85d22bebb3ff11ff178b8da7c52c52fc4428b07ae1f1a9e0e3cc963136d542b2d698b6e84d572fbfbffcf1eafaf5af5e3d4092e33443b7b7ff09a8008ae5d661597bdbcfeb54dbd944c711014a2161cd76dc63b5087d087befe7ffb97e5484d5025f4e5fda36ffdcb500975c917ac1357fffea6484d401565b4a77bba2ff34135b711004583df39c7a16669f2840edca371fec8f575fff5f8f5049f3b1fff7d709a8338aaffeff1ad4981350cb5bffbfef09a87f043fff5f426a1132f7f5ffd09a821786b3fff7d426a057b80dfff6fe84d430c37fe9a7baed09a800769f54dd97df7dbbd3dc9e23e23158888250743e4da9d997974501d0879f2fe581b62b3f7711825831ca84b9421d3e4fa4d0874bc9ef895b771190dac47e55003a076252c032ccac0d4583c67078e3f101a946e260152754c800402a134002756425e7bb77f97e2305b1094e1c2c067a62284172a506a260011df686ed758e28c461702e001778034a407a1b16cd4ff11105bb0003784c552d810f6e5c5ce79e03e84381fd1008d9e471b8bb0cb2e20c70a34a8ab3fff1100a945d03f257e3cc499e2be9ae9072000f94896b5ca506e1c577fb28fc442b2503e0bce41bc1abacb5e05d6320455c3f3505681ec1c1a888a3240aab928b10001004f2c10782ac03e640ba8691007cc559a827a3cbfe221e600288540a914651e4be0e87c007b983f8cc550080038df00d6c8001818aa92448153c01ef80fc3b362084bf97f111059618ed4580e84505d02d6242f5d8c8e03f9e0705036e1400025385840c64c3a16da22241291e7e6fc9f1111ef1e738583a11ef2b553f1e62380ce79c845ac2901008910430ea3c00040078592b101bb030b2e33d97f111ac5000811454c01a83a00f138d0c0022d06cc09476727596154030742a4141bc3415c9264021789002af000f9e1b8bfcbf88852be0e4e2381400a8d266a1e00e1a9f10f02c1610c6780f29f05440792d0210684000a79f0625601b88c16e958f5eb6583012a944252f8ca00ba920f9fc7ad1500025061e018ca88000b8d7bfc132625ae297f8885641a000100c052c006be1d8000801faf80014b19614e521f0e87c9f21b000170d4c410c0372c007a4320d40600315906e156fda684435100f47855e2c05c73a5985d4009135a0e927d09e1dead7fffb752c63af06890b07fb9531297e0d0af0e03b1fb7c9d3f20a02fb1d7bca101c2c05064c0b78b2cb0ff0ba800dec69a27563d71bbb570bb95510da8fad5695a1814378ab8b7e135001be5e47055c6ba97975bbd75be5f4c826a2ee52bffdbdb6e96750516c6ab5077a0f5035b219ffbfa8f50997537f7aecbe223426e66b5426b057565b5f25bc7a98c6f047ffd7c26a037df97eaffedc4629dccd6a8448d0afa8b742146e4fbf09a80eeeecb2aed7ff7e221ab99a3aaf312d83fcaa857e25ece7f9950ba847b6574eafb7fffbf16f09a879268b7fff951d04d4007eabcf048f7cdedd046c754d447f11fd47a8011ba4b0ed3fdfe69");

private final byte[] rtpDataExtension =
getBytesFromHexString("00a20003a94f000062150100cbca0100");

private final byte[] rtpWithHeaderExtensionPayloadData =
Arrays.copyOfRange(
rtpDataWithHeaderExtension, RtpPacket.MIN_HEADER_SIZE + rtpDataExtension.length, rtpDataWithHeaderExtension.length);

@Test
public void parseRtpPacket() {
RtpPacket packet = checkNotNull(RtpPacket.parse(rtpData, rtpData.length));
Expand Down Expand Up @@ -103,6 +126,24 @@ public void parseRtpPacketWithLargeTimestamp() {
assertThat(packet.payloadData).isEqualTo(rtpWithLargeTimestampPayloadData);
}

@Test
public void parseRtpPacketWithHeaderExtension() {
RtpPacket packet =
checkNotNull(RtpPacket.parse(rtpDataWithHeaderExtension, rtpDataWithHeaderExtension.length));

assertThat(packet.version).isEqualTo(RtpPacket.RTP_VERSION);
assertThat(packet.padding).isFalse();
assertThat(packet.extension).isTrue();
assertThat(packet.csrcCount).isEqualTo(0);
assertThat(packet.csrc).hasLength(0);
assertThat(packet.marker).isFalse();
assertThat(packet.payloadType).isEqualTo(96);
assertThat(packet.sequenceNumber).isEqualTo(61514);
assertThat(packet.timestamp).isEqualTo(2000000000);
assertThat(packet.ssrc).isEqualTo(0x35ff2773);
assertThat(packet.payloadData).isEqualTo(rtpWithHeaderExtensionPayloadData);
}

@Test
public void writetoBuffer_withProperlySizedBuffer_writesPacket() {
int packetByteLength = rtpData.length;
Expand Down Expand Up @@ -187,6 +228,28 @@ public void buildRtpPacketWithLargeTimestamp_matchesPacketData() {
assertThat(builtPacketBytes).isEqualTo(rtpDataWithLargeTimestamp);
}

@Test
public void buildRtpPacketWithHeaderExtension_matchesPacketData() {
RtpPacket builtPacket =
new RtpPacket.Builder()
.setPadding(false)
.setExtension(true)
.setMarker(false)
.setPayloadType((byte) 96)
.setSequenceNumber(61514)
.setTimestamp(2000000000)
.setSsrc(0x35ff2773)
.setHeaderExtension(Arrays.copyOfRange(rtpDataExtension, 0, RtpPacket.HEADER_EXTENSION_SIZE))
.setExtensionPayload(Arrays.copyOfRange(rtpDataExtension, RtpPacket.HEADER_EXTENSION_SIZE, rtpDataExtension.length))
.setPayloadData(rtpWithHeaderExtensionPayloadData)
.build();

int packetSize = RtpPacket.MIN_HEADER_SIZE + rtpDataExtension.length + builtPacket.payloadData.length;
byte[] builtPacketBytes = new byte[packetSize];
builtPacket.writeToBuffer(builtPacketBytes, /* offset= */ 0, packetSize);
assertThat(builtPacketBytes).isEqualTo(rtpDataWithHeaderExtension);
}

@Test
public void getNextSequenceNumber_invokingAtWrapOver() {
assertThat(getNextSequenceNumber(65534)).isEqualTo(65535);
Expand Down