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

[BUG] resendPacketOnNack() potentially putting RTP Packet back into wrong location in Rolling Buffer when RTP sequence overflows #1453

Open
suggestedfixes opened this issue Apr 15, 2022 · 5 comments

Comments

@suggestedfixes
Copy link
Contributor

suggestedfixes commented Apr 15, 2022

Describe the bug
Observation, when nack count from chrome://webrtc-internals exceeds around 3.5k, chrome is no longer able to decode video.
When turning on logging lvl 1, the observation is that when resendPacketOnNack is called, RTP sequence number starts to roll back from 65535 (UINT16) to 0, and rolling buffer will complain about STATUS_ROLLING_BUFFER_NOT_IN_RANGE,
At first glance, it might seem to be the rolling buffer not handling the edge case where the index rolls back. However, after a closer
look. It might seem that the Rtp packet putting back into the rollingbuffer isn't the same as the one extracted.

Source:
https://github.com/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/blob/master/src/source/PeerConnection/Retransmitter.c

Item retrieved:
retStatus = rollingBufferExtractData(pSenderTranceiver->sender.packetBuffer->pRollingBuffer, pRetransmitter->validIndexList[index], &item);

Item put back:
retStatus =
rollingBufferInsertData(pSenderTranceiver->sender.packetBuffer->pRollingBuffer, pRetransmitter->sequenceNumberList[index], item);

Expected to be put back
retStatus =
rollingBufferInsertData(pSenderTranceiver->sender.packetBuffer->pRollingBuffer, pRetransmitter->validIndexList[index], item);

To Reproduce
Steps to reproduce the behavior:

  1. Increase the bitrate, or decrease the network bandwidth such that we observe a linear increase of Nacks on the browser side.
  2. On the master side, turn on all log levels.
  3. Observe "Resent packet ssrc %lu seq %lu succeeded", where seq reaches beyond 65535 and rolls back to 0.
  4. Also observe that chrome will stop decoding video correctly

Expected behavior
reendPacketOnNack() added back the correct RtpPacket back into the rolling buffer.

Screenshots
image

Additional context
Add any other context about the problem here.

@suggestedfixes suggestedfixes added the bug Something isn't working label Apr 15, 2022
@suggestedfixes suggestedfixes changed the title [BUG] resendPacketOnNack() potentially putting the wrong RtpPacket back into Rolling Buffer [BUG] resendPacketOnNack() potentially putting the wrong RtpPacket index back into Rolling Buffer Apr 15, 2022
@suggestedfixes suggestedfixes changed the title [BUG] resendPacketOnNack() potentially putting the wrong RtpPacket index back into Rolling Buffer [BUG] resendPacketOnNack() potentially putting RTP Packet back into wrong location in Rolling Buffer when RTP sequence overflows Apr 16, 2022
@disa6302 disa6302 added needs-triage and removed bug Something isn't working labels Apr 19, 2022
@disa6302 disa6302 added the bug Something isn't working label May 23, 2022
@github-actions
Copy link

This is a very old issue. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to open a new one.

@github-actions
Copy link

This is a very old issue. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to open a new one.

Copy link

This is a very old issue. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to open a new one.

Copy link

This is a very old issue. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to open a new one.

@wei-zhang-simplisafe
Copy link

The ticket was not fixed. Please see new ticket #1991

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

No branches or pull requests

5 participants