Skip to content

RTSP H265 Aggregation packet support #2413

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.

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

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

Conversation

ArtsemKurantsou
Copy link

@ArtsemKurantsou ArtsemKurantsou commented May 12, 2025

Aggregation packet support for H265 RTSP added according to the specification.
The implementation is based on the assumption that DONL won't be present in the packet since sprop-max-don-diff != 0 is not supported

Fixes issue: #1008

@ArtsemKurantsou ArtsemKurantsou marked this pull request as ready for review May 12, 2025 00:09
@ArtsemKurantsou
Copy link
Author

Hi @tonihei, could you please help with review and moving forward with the change?

@microkatz microkatz self-requested a review May 16, 2025 12:23
@microkatz microkatz self-assigned this May 16, 2025
int endOfData = data.bytesLeft();
int currentPosition = 2; // skipping payload header (2 bytes)
do {
int nalUnitSize = ((data.getData()[currentPosition] & 0xFF) << 8)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this equivalent to data.readUnsignedShort()?

@ArtsemKurantsou ArtsemKurantsou requested a review from microkatz May 20, 2025 16:21
@ArtsemKurantsou
Copy link
Author

Hi @microkatz thank you for review and comment, could you please review again?

ArtsemKurantsou and others added 5 commits May 22, 2025 10:37
ParsableByteArray operations utilized instead of manual bytes manipulation
Got read of endOfData variable in favor of check based on bytesLeft
@microkatz microkatz force-pushed the rtap-h265-aggregation-packet-support branch from 68d2c66 to af67e05 Compare May 22, 2025 10:46
@microkatz
Copy link
Contributor

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!

@microkatz
Copy link
Contributor

microkatz commented May 22, 2025

@ArtsemKurantsou

Would you be able to add a test for this? I believe that you would create a file and test similar to the one in RtpH263ReaderTest.

Thank you!

data.setPosition(2); // skipping payload header (2 bytes)
do {
short nalUnitSize = data.readShort(); // 2 bytes of NAL unit size
int nalHeaderType = (data.getData()[data.getPosition()] >> 1) & 0x3F;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the data.getData()[data.getPosition()] would be data.peekUnsignedByte().

Or the whole line as NalUnitUtil.getH265NalUnitType(data.getData(), -3).

trackOutput.sampleData(data, nalUnitSize);
fragmentedSampleSizeBytes += nalUnitSize;
bufferFlags = getBufferFlagsFromNalType(nalHeaderType);
} while (data.bytesLeft() > 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also make this a while loop such that while(data.bytesLeft() >= 3)

Copy link
Author

Choose a reason for hiding this comment

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

That is correct, however in this case we may miss "invalid" packet with 1-2 bytes "extra"

@ArtsemKurantsou
Copy link
Author

@ArtsemKurantsou

Would you be able to add a test for this? I believe that you would create a file and test similar to the one in RtpH263ReaderTest.

Thank you!

Yes, sure, I can add tests for that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants