referenceTime calculation error in FeedbackRtpTransport.cpp

[bianxg]

this->referenceTime = static_cast<int32_t>((timestamp & 0x1FFFFFC0) / 64); should be (referenceTime is 3 byte long) this->referenceTime = static_cast<int32_t>((timestamp & 0x3FFFFFC0) / 64);

for example timestamp = 763974970 BIN 00101101100010010101010100111010 0x1FFFFFC0 is 00011111111111111111111111000000

from webrtc source constexpr int kBaseScaleFactor = 64; constexpr int64_t kTimeWrapPeriod = (1ll << 24) * kBaseScaleFactor; this->referenceTime = static_cast<int32_t>((timestamp % kTimeWrapPeriod)/64);

[ibc]

CC @jmillan

[jmillan]

It's definitely odd.

I wonder why we set that mask. Being a 3 byte field it should be 0x00FFFFFF indeed.

What's the reason behind 0x3FFFFFC0, @bianxg ?

[jmillan]

Indeed we need it to be a 23bit field, so the mask should be: 0x7FFFFF00

[jmillan]

There is indeed discussion/explanation about this here https://github.com/meetecho/janus-gateway/issues/1733#issuecomment-526553410 @ibc , but I don't see why we set that mask.

[jmillan]

I'm checking it.

[ibc]

Honestly I don't think I can give better rationale than the one given in that comment when it was fresh in my mind, but AFAIS I provide there even with code to demonstrate things.

[jmillan]

Got it:

0x1FFFFFC0 => 536870848 => 11111111111111111111111000000

Dividing such value by 64:

0x7FFFFF => 8388607 => 00000000011111111111111111111111

So by adding 0x1FFFFFC0 mask we make sure that after dividing the value by 64, the resulting number will fit in 23 bits.

[jmillan]

Where do you then see the problem @bianxg ?

[bianxg]

from webrtc source code, referenceTime is calculated as below: constexpr int kBaseScaleFactor = 64; constexpr int64_t kTimeWrapPeriod = (1ll << 24) * kBaseScaleFactor; this->referenceTime = static_cast<int32_t>((timestamp % kTimeWrapPeriod)/64);

which is equals to this->referenceTime = static_cast<int32_t>((timestamp & 0x3FFFFFC0) / 64); by adding 0x3FFFFFC0 mask after dividing the value by 64, the resulting number will fit in 24 bits.

[kingpeter2015]

I wrote one test:

auto calc_mediasoup = [](uint64_t ts){
    return static_cast<int32_t>((ts & 0x1FFFFFC0) / 64);
};

auto calc_mediasoup_modified = [](uint64_t ts){
    return static_cast<int32_t>((ts & 0x3FFFFFC0) / 64);
};

auto calc_webrtc = [](uint64_t ts){
    constexpr int kBaseScaleFactor = 64;
		constexpr int64_t kTimeWrapPeriod = (1ll << 24) * kBaseScaleFactor;
    return static_cast<int32_t>((ts % kTimeWrapPeriod) / 64);
};

for(uint64_t t = 0; t < UINT64_MAX; t++) {
    int32_t ref_mediasoup = calc_mediasoup(t);
    int32_t ref_mediasoup_modified = calc_mediasoup_modified (t);
    int32_t ref_webrtc = calc_webrtc(t);
    if(ref_mediasoup != ref_webrtc ) {
      printf("mediasoup vs webrtc difference: timestamp=%d, mediasoup reference time=%d, webrtc reference time=%d\n", t, ref_mediasoup , ref_webrtc );
    }

    if(ref_mediasoup_modified != ref_webrtc ) {
      printf("mediasoup_modified vs webrtc difference: timestamp=%d, mediasoup reference time=%d, webrtc reference time=%d\n", t, ref_mediasoup_modified , ref_webrtc );
    }
  }  

I found definitely ref_mediasoup != ref_webrtc, whereas ref_mediasoup_modified == ref_webrtc

[jmillan]

The difference is that mediasoup does NOT generate negative timestamp values. That's why we use 23 bits to represent it, so the most significant bit (sign) is always 0 and thus represent a positive number.

[jmillan]

Anyway, have you seen this is an issue for libwebrtc processing this RTCP packets?

[kingpeter2015]

@jmillan There are int, int16_t, int32_t, int64_t etc types in C/C++, however, there is no 24-bit signed integer. And using the lowest 24 bits of a uint32_t type to assign and store can ensure reference_time is not negative.

[jmillan]

This was handled here https://github.com/versatica/mediasoup/pull/899