FeedbackRtpTransport.cpp may have an overflow mistake about mediasoup

[Lynnworld]

but how about sequence reset ？

[Lynnworld]

right，my mistake

[ibc]

Nope, that's why just above that block we check received sequence manager with SeqManager utility:

// If the wide sequence number of the new packet is lower than the latest
// seen, ignore it.
// NOTE: Not very spec compliant but libwebrtc does it.
// Also ignore if the sequence number matches the latest seen.
if (!RTC::SeqManager<uint16_t>::IsSeqHigherThan(sequenceNumber, this->latestSequenceNumber))
{
	return AddPacketResult::SUCCESS;
}

// Check if there are too many missing packets.
{
	auto missingPackets = sequenceNumber - (this->latestSequenceNumber + 1);

	if (missingPackets > FeedbackRtpTransportPacket::maxMissingPackets)
	{
		MS_WARN_DEV("RTP missing packet number exceeded");

		return AddPacketResult::FATAL;
	}
}

[ibc]

but how about sequence reset ？

What is sequence reset? When seq number reaches max number 65535 and next packet has seq number 0?
Well, that's exactly what RTC::SeqManager<uint16_t>::IsSeqHigherThan() cares about.

[Lynnworld]

Yes, IsSeqHigherThan get true ,but missingPackets get wrong result.

[ibc]

Actually this is indeed wrong, but not sure if you are pointing to the real problem which is the usage of auto:

#include <cinttypes>  // PRIu64, etc
#include <cstddef>    // size_t
#include <cstdint>    // uint8_t, etc
#include <cstdio>

int main() {
    
    uint16_t sequenceNumber = 0;
    uint16_t latestSequenceNumber = 65535;
    
    auto missingPackets = sequenceNumber - (latestSequenceNumber + 1);
    
    printf("missingPackets: %" PRIu16 "\n", missingPackets);

    return 0;
}

missingPackets: 4294901760

#include <cinttypes>  // PRIu64, etc
#include <cstddef>    // size_t
#include <cstdint>    // uint8_t, etc
#include <cstdio>

int main() {
    
    uint16_t sequenceNumber = 0;
    uint16_t latestSequenceNumber = 65535;
    
    uint16_t missingPackets = sequenceNumber - (latestSequenceNumber + 1);
    
    printf("missingPackets: %" PRIu16 "\n", missingPackets);

    return 0;
}

missingPackets: 0

[Lynnworld]

Not familiar with c++，But yes, auto is the real problem 👍

[Lynnworld]

uint16 minus will overflow too

[ibc]

uint16 minus will overflow too

It wont. If I am wrong please be more explicit.

BTW as shown above:

#include <cinttypes>  // PRIu64, etc
#include <cstddef>    // size_t
#include <cstdint>    // uint8_t, etc
#include <cstdio>

int main() {
    
    uint16_t sequenceNumber = 0;
    uint16_t latestSequenceNumber = 65535;
    
    uint16_t missingPackets = sequenceNumber - (latestSequenceNumber + 1);
    
    printf("missingPackets: %" PRIu16 "\n", missingPackets);

    return 0;
}

missingPackets: 0