Skip to content

RemoteClockOffsetEstimator class - #1882

Open
ibc wants to merge 1 commit into
v3from
remote-clock-offset-estimator
Open

RemoteClockOffsetEstimator class#1882
ibc wants to merge 1 commit into
v3from
remote-clock-offset-estimator

Conversation

@ibc

@ibc ibc commented Aug 11, 2026

Copy link
Copy Markdown
Member

Details

  • Related to issue Do not make RTCP Sender Reports depend on packet arrival time #1881 "Do not make RTCP Sender Reports depend on packet arrival time".
  • Added a new class RemoteClockOffsetEstimator that estimates the offset between the wall clock of a remote sender, as reported in the NTP field of the RTCP Sender Reports it sends, and the mediasoup's monotonic clock, so that a time expressed in the former can be translated into the latter.
  • RTC::Packet: Added captureMs getter/setter (will be needed later).
  • Not yet used.

# Details

- Related to issue #1881 "Do not make RTCP Sender Reports depend on packet arrival time".
- Added a new class ` RemoteClockOffsetEstimator` that estimates the offset between the wall clock of a remote sender, as reported in the NTP field of the RTCP Sender Reports it sends, and the mediasoup's monotonic clock, so that a time expressed in the former can be translated into the latter.
- `RTC::Packet`: Added `captureMs` getter/setter (will be needed later).
- Not yet used.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

feed(estimator, RemoteBaseMs + elapsedMs, LocalBaseMs + elapsedMs, 1, 0, 0);

REQUIRE(estimator.GetOffsetMs().has_value());
REQUIRE(estimator.GetOffsetMs().value() == BaseOffsetMs);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: unchecked access to optional value [bugprone-unchecked-optional-access]

		REQUIRE(estimator.GetOffsetMs().value() == BaseOffsetMs);
          ^
Additional context

worker/subprojects/Catch2-3.14.0/src/catch2/catch_test_macros.hpp:133: expanded from macro 'REQUIRE'

  #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
                                                                                           ^

worker/subprojects/Catch2-3.14.0/src/catch2/internal/catch_test_macro_impl.hpp:49: expanded from macro 'INTERNAL_CATCH_TEST'

            catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); /* NOLINT(bugprone-chained-comparison) */ \
                                                                     ^

{
feed(estimator, RemoteBaseMs, LocalBaseMs, minSampleCount, 40, 0);

REQUIRE(estimator.GetOffsetMs().value() == BaseOffsetMs + 40);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: unchecked access to optional value [bugprone-unchecked-optional-access]

		REQUIRE(estimator.GetOffsetMs().value() == BaseOffsetMs + 40);
          ^
Additional context

worker/subprojects/Catch2-3.14.0/src/catch2/catch_test_macros.hpp:133: expanded from macro 'REQUIRE'

  #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
                                                                                           ^

worker/subprojects/Catch2-3.14.0/src/catch2/internal/catch_test_macro_impl.hpp:49: expanded from macro 'INTERNAL_CATCH_TEST'

            catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); /* NOLINT(bugprone-chained-comparison) */ \
                                                                     ^

{
feed(estimator, RemoteBaseMs, LocalBaseMs, minSampleCount, 40, 80);

REQUIRE(estimator.GetOffsetMs().value() == BaseOffsetMs);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: unchecked access to optional value [bugprone-unchecked-optional-access]

		REQUIRE(estimator.GetOffsetMs().value() == BaseOffsetMs);
          ^
Additional context

worker/subprojects/Catch2-3.14.0/src/catch2/catch_test_macros.hpp:133: expanded from macro 'REQUIRE'

  #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
                                                                                           ^

worker/subprojects/Catch2-3.14.0/src/catch2/internal/catch_test_macro_impl.hpp:49: expanded from macro 'INTERNAL_CATCH_TEST'

            catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); /* NOLINT(bugprone-chained-comparison) */ \
                                                                     ^

{
feed(estimator, RemoteBaseMs, LocalBaseMs, windowSize, 20, 0);

REQUIRE(estimator.GetOffsetMs().value() == BaseOffsetMs + 20);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: unchecked access to optional value [bugprone-unchecked-optional-access]

		REQUIRE(estimator.GetOffsetMs().value() == BaseOffsetMs + 20);
          ^
Additional context

worker/subprojects/Catch2-3.14.0/src/catch2/catch_test_macros.hpp:133: expanded from macro 'REQUIRE'

  #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
                                                                                           ^

worker/subprojects/Catch2-3.14.0/src/catch2/internal/catch_test_macro_impl.hpp:49: expanded from macro 'INTERNAL_CATCH_TEST'

            catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); /* NOLINT(bugprone-chained-comparison) */ \
                                                                     ^


feed(estimator, RemoteBaseMs + elapsedMs, LocalBaseMs + elapsedMs, 1, 500, 0);

REQUIRE(estimator.GetOffsetMs().value() == BaseOffsetMs + 20);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: unchecked access to optional value [bugprone-unchecked-optional-access]

		REQUIRE(estimator.GetOffsetMs().value() == BaseOffsetMs + 20);
          ^
Additional context

worker/subprojects/Catch2-3.14.0/src/catch2/catch_test_macros.hpp:133: expanded from macro 'REQUIRE'

  #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
                                                                                           ^

worker/subprojects/Catch2-3.14.0/src/catch2/internal/catch_test_macro_impl.hpp:49: expanded from macro 'INTERNAL_CATCH_TEST'

            catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); /* NOLINT(bugprone-chained-comparison) */ \
                                                                     ^

estimator.AddSenderReport(RemoteBaseMs + 1001, LocalBaseMs + 1000, 0);
estimator.AddSenderReport(RemoteBaseMs + 2000, LocalBaseMs + 2000, 0);

REQUIRE(estimator.GetOffsetMs().value() == BaseOffsetMs);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: unchecked access to optional value [bugprone-unchecked-optional-access]

		REQUIRE(estimator.GetOffsetMs().value() == BaseOffsetMs);
          ^
Additional context

worker/subprojects/Catch2-3.14.0/src/catch2/catch_test_macros.hpp:133: expanded from macro 'REQUIRE'

  #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
                                                                                           ^

worker/subprojects/Catch2-3.14.0/src/catch2/internal/catch_test_macro_impl.hpp:49: expanded from macro 'INTERNAL_CATCH_TEST'

            catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); /* NOLINT(bugprone-chained-comparison) */ \
                                                                     ^


SECTION("remote times are translated into our clock")
{
RTC::RemoteClockOffsetEstimator untrainedEstimator;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: variable 'untrainedEstimator' of type 'RTC::RemoteClockOffsetEstimator' can be declared 'const' [misc-const-correctness]

Suggested change
RTC::RemoteClockOffsetEstimator untrainedEstimator;
RTC::RemoteClockOffsetEstimator const untrainedEstimator;


feed(estimator, RemoteBaseMs, LocalBaseMs, minSampleCount, 0, 0);

REQUIRE(estimator.RemoteMsToLocalMs(RemoteBaseMs).value() == LocalBaseMs);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: unchecked access to optional value [bugprone-unchecked-optional-access]

		REQUIRE(estimator.RemoteMsToLocalMs(RemoteBaseMs).value() == LocalBaseMs);
          ^
Additional context

worker/subprojects/Catch2-3.14.0/src/catch2/catch_test_macros.hpp:133: expanded from macro 'REQUIRE'

  #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
                                                                                           ^

worker/subprojects/Catch2-3.14.0/src/catch2/internal/catch_test_macro_impl.hpp:49: expanded from macro 'INTERNAL_CATCH_TEST'

            catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); /* NOLINT(bugprone-chained-comparison) */ \
                                                                     ^

feed(estimator, RemoteBaseMs, LocalBaseMs, minSampleCount, 0, 0);

REQUIRE(estimator.RemoteMsToLocalMs(RemoteBaseMs).value() == LocalBaseMs);
REQUIRE(estimator.RemoteMsToLocalMs(RemoteBaseMs + 500).value() == LocalBaseMs + 500);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: unchecked access to optional value [bugprone-unchecked-optional-access]

		REQUIRE(estimator.RemoteMsToLocalMs(RemoteBaseMs + 500).value() == LocalBaseMs + 500);
          ^
Additional context

worker/subprojects/Catch2-3.14.0/src/catch2/catch_test_macros.hpp:133: expanded from macro 'REQUIRE'

  #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
                                                                                           ^

worker/subprojects/Catch2-3.14.0/src/catch2/internal/catch_test_macro_impl.hpp:49: expanded from macro 'INTERNAL_CATCH_TEST'

            catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); /* NOLINT(bugprone-chained-comparison) */ \
                                                                     ^

// the very same times as before the reset are not taken as duplicates.
feed(estimator, RemoteBaseMs, LocalBaseMs, minSampleCount, 0, 0);

REQUIRE(estimator.GetOffsetMs().value() == BaseOffsetMs);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: unchecked access to optional value [bugprone-unchecked-optional-access]

		REQUIRE(estimator.GetOffsetMs().value() == BaseOffsetMs);
          ^
Additional context

worker/subprojects/Catch2-3.14.0/src/catch2/catch_test_macros.hpp:133: expanded from macro 'REQUIRE'

  #define REQUIRE( ... ) INTERNAL_CATCH_TEST( "REQUIRE", Catch::ResultDisposition::Normal, __VA_ARGS__  )
                                                                                           ^

worker/subprojects/Catch2-3.14.0/src/catch2/internal/catch_test_macro_impl.hpp:49: expanded from macro 'INTERNAL_CATCH_TEST'

            catchAssertionHandler.handleExpr( Catch::Decomposer() <= __VA_ARGS__ ); /* NOLINT(bugprone-chained-comparison) */ \
                                                                     ^

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant