search

ultravideo / uvgRTP

An open-source library for RTP/SRTP media delivery
BSD 2-Clause "Simplified" License
296 stars 84 forks source link

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

Closed Banner2404 closed 2 weeks ago

Banner2404 commented 1 month ago

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 commented 4 weeks ago

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 commented 2 weeks ago

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

jrsnen commented 2 weeks ago

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