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.

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

Fixed the issue when reconstructing h264 frames that are not continuous #221

Merged
merged 2 commits into from
Jun 17, 2024

Conversation

Banner2404
Copy link
Contributor

Hello, I noticed that there is an issue with RTP frame handling (h264), when receiving a stream with a noticeable packet loss. The crash happens in the following case. Consider the client receiving these RTP frames:

Packet seq 100, type = FT_START
Packet seq 101, type = FT_MIDDLE
Packet seq 102, type = FT_END // This packet lost
Packet seq 103, type = FT_START // Reconstructed (deallocated)
Packet seq 104, type = FT_END  // Reconstructed (deallocated)
Packet seq 105, type = FT_START
Packet seq 106, type = FT_END

In that case the packet seq 102 was lost. When the library receives seq 104, then it can successfully reconstruct the NAL unit from seq 103, 104 and deallocate them. Then the library receives seq 106 and incorrectly determines the frames 100-106 as continuous and attempts to reconstruct the NAL unit from them -> resulting in a crash when accessing deallocated 103 frame.
Looks like the issue is that continuous flag was never reset to false in case the frames are not continuous.
Screenshot 2024-06-03 at 10 24 15

Example of the crash:
Screenshot 2024-06-03 at 10 22 39

@Banner2404
Copy link
Contributor Author

Also added a commit to fix the following case:

Packet seq 100, type = FT_START
Packet seq 101, type = FT_MIDDLE
Packet seq 102, type = FT_END // This packet lost
Packet seq 103, type = FT_START // This packet lost
Packet seq 104, type = FT_MIDDLE
Packet seq 105, type = FT_END

Previously, the library detected the sequence 100...105 as continuous and could crash. Updated the check to fix this case.

@tampsa
Copy link
Collaborator

tampsa commented Jun 14, 2024

Hi, thanks for the pull request! This is a welcome fix.

@jrsnen jrsnen merged commit 4ac1cb6 into ultravideo:master Jun 17, 2024
1 check passed
@jrsnen
Copy link
Member

jrsnen commented Jun 17, 2024

I merged this, thanks.

On a related note, last week I rewrote the whole section of code last week as there was a bug related to sequence number roll-over. I think I also fixed most problems related to checking continuity of frames. If there are any issues, feel frame to open an issue or make another PR.

BR, Joni

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

Successfully merging this pull request may close these issues.

None yet

3 participants