fix: disable pthread_condattr_setclock on lower versions of Android #3138

lilinxiong · 2025-03-12T08:19:09Z

On Android OS versions equal to or less than 21, pthread_condattr_setclock is not available, so we need to disable it to prevent compilation failures.

maxsharabayko · 2025-03-12T10:06:12Z

I have a feeling this check should better be done by CMake, somewhere here
https://github.com/Haivision/srt/blob/v1.5.4/CMakeLists.txt#L141
and SRT_SYNC_CLOCK_GETTIME_MONOTONIC stays undefined

Or here
https://github.com/Haivision/srt/blob/v1.5.4/srtcore/sync.h#L29

lilinxiong · 2025-03-12T10:10:01Z

I have a feeling this check should better be done by CMake, somewhere here https://github.com/Haivision/srt/blob/v1.5.4/CMakeLists.txt#L141

change test_requires_clock_gettime ?

maxsharabayko · 2025-03-12T10:19:16Z

Hm, sorry, I got confused. You define USE_CLOCK_GETTIME, but it controls usage of the pthread_condattr_setclock, but clock_gettime(CLOCK_MONOTONIC, &timeout) is still used if SRT_SYNC_CLOCK == SRT_SYNC_CLOCK_GETTIME_MONOTONIC. 🤔

maxsharabayko

Maybe this way?

srtcore/sync_posix.cpp

maxsharabayko

So it seems like
pthread_condattr_setclock(&CondAttribs, CLOCK_MONOTONIC) won't be called to configure a CV to use MONOTONIC clock.
But there will still be calls to clock_gettime(CLOCK_MONOTONIC, &timeout) to get the target timeout, while the CV won't probably use monotonic clock to wait...

So timeout will be with monotonic clock base, but CV will use system clock internally to wait...

timeout = us_to_timespec(now_us + count_microseconds(rel_time));
return pthread_cond_timedwait(&m_cv, &lock.mutex()->ref(), &timeout) != ETIMEDOUT;

lilinxiong · 2025-03-12T11:03:41Z

It seems that we forgot to call pthread_condattr_destroy to release the attr after initializing it with pthread_condattr_init. It remains in existence, which is a bug.

cl-ment · 2025-03-12T16:03:08Z

May I suggest to modify CMakelists.txt:335 as below

if (ENABLE_MONOTONIC_CLOCK)
    list(PREPEND CMAKE_REQUIRED_LIBRARIES "${CMAKE_THREAD_LIBS_INIT}")
    check_symbol_exists(pthread_condattr_setclock "pthread.h" HAVE_PTHREAD_CONDATTR_SETCLOCK)
    if (NOT (ENABLE_MONOTONIC_CLOCK_DEFAULT AND (HAVE_PTHREAD_CONDATTR_SETCLOCK OR ENABLE_STDCXX_SYNC)))
		message(FATAL_ERROR "Your platform does not support CLOCK_MONOTONIC. Build with -DENABLE_MONOTONIC_CLOCK=OFF.")
	endif()
	set (WITH_EXTRALIBS "${WITH_EXTRALIBS} ${MONOTONIC_CLOCK_LINKLIB}")
	add_definitions(-DENABLE_MONOTONIC_CLOCK=1)
endif()

I'm not sure about the changes made to CMAKE_REQUIRED_LIBRARIES with CMAKE_THREAD_LIBS_INIT. Maybe it would be better to use PTHREAD_LIBRARY and to move this block after its definition (near line 940).

lilinxiong · 2025-03-13T03:22:28Z

May I suggest to modify CMakelists.txt:335 as below
if (ENABLE_MONOTONIC_CLOCK)
    list(PREPEND CMAKE_REQUIRED_LIBRARIES "${CMAKE_THREAD_LIBS_INIT}")
    check_symbol_exists(pthread_condattr_setclock "pthread.h" HAVE_PTHREAD_CONDATTR_SETCLOCK)
    if (NOT (ENABLE_MONOTONIC_CLOCK_DEFAULT AND (HAVE_PTHREAD_CONDATTR_SETCLOCK OR ENABLE_STDCXX_SYNC)))
		message(FATAL_ERROR "Your platform does not support CLOCK_MONOTONIC. Build with -DENABLE_MONOTONIC_CLOCK=OFF.")
	endif()
	set (WITH_EXTRALIBS "${WITH_EXTRALIBS} ${MONOTONIC_CLOCK_LINKLIB}")
	add_definitions(-DENABLE_MONOTONIC_CLOCK=1)
endif()
I'm not sure about the changes made to CMAKE_REQUIRED_LIBRARIES with CMAKE_THREAD_LIBS_INIT. Maybe it would be better to use PTHREAD_LIBRARY and to move this block after its definition (near line 940).

Good idea. I have already made the changes.

ethouris · 2025-03-13T07:48:06Z

That error message should rather advice compiling with C++11, as compiling without monotonic clock is dangerous.

lilinxiong · 2025-03-13T08:29:16Z

That error message should rather advice compiling with C++11, as compiling without monotonic clock is dangerous.

So the modification that should be made here is, if both ENABLE_STDCXX_SYNC and ENABLE_MONOTONIC_CLOCK are set to OFF, as seen at CMakeLists.txt#L653-L654, then we should halt the configuration process and inform the developers about this risky behavior?

ethouris · 2025-03-13T08:33:31Z

Well, it could be quite complicated and "reinventing" if we set default C++11 for Android in this case, so my stance on it is that:

Monotonic clock should be default, at least on platforms where POSIX sync is default
If monotonic clock with POSIX is not available on the platform, exit with failure - even though this means that in certain cases it is a failure with default parameters
The error message should advice to use either ENABLE_CXX11_SYNC or ENABLE_MONOTONIC_CLOCK=OFF (DANGEROUS).

ethouris · 2025-03-13T08:37:04Z

BTW These last CI builds on Travis fail on the configuration level, but weirdly I see only the general error at the end of cmake, but no error message during configuration.

…he configuration.

lilinxiong · 2025-03-13T08:56:35Z

Well, it could be quite complicated and "reinventing" if we set default C++11 for Android in this case, so my stance on it is that:

Monotonic clock should be default, at least on platforms where POSIX sync is default

If monotonic clock with POSIX is not available on the platform, exit with failure - even though this means that in certain cases it is a failure with default parameters

The error message should advice to use either ENABLE_CXX11_SYNC or ENABLE_MONOTONIC_CLOCK=OFF (DANGEROUS).

cc? I have already made the changes.

lilinxiong · 2025-03-13T10:01:15Z

Look at sync.h#L30-L45 here. The platform and ENABLE_MONOTONIC_CLOCK should be separated. They should not be used interchangeably. We should find time to fix it.

maxsharabayko · 2025-03-13T12:05:05Z

@lilinxiong you are right.

1. CLOCK_MONOTONIC

SRT uses clock_gettime(CLOCK_MONOTONIC, &timeout) in sync::steady_clock via sync::rdtsc(uint64_t& t) if SRT_SYNC_CLOCK == SRT_SYNC_CLOCK_GETTIME_MONOTONIC.

The SRT_SYNC_CLOCK is defined in sync.h as follows:

// Defile clock type to use
#ifdef IA32
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_IA32_RDTSC
#define SRT_SYNC_CLOCK_STR "IA32_RDTSC"
#elif defined(IA64)
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_IA64_ITC
#define SRT_SYNC_CLOCK_STR "IA64_ITC"
#elif defined(AMD64)
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_AMD64_RDTSC
#define SRT_SYNC_CLOCK_STR "AMD64_RDTSC"
#elif defined(_WIN32)
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_WINQPC
#define SRT_SYNC_CLOCK_STR "WINQPC"
#elif TARGET_OS_MAC
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_MACH_ABSTIME
#define SRT_SYNC_CLOCK_STR "MACH_ABSTIME"
#elif defined(ENABLE_MONOTONIC_CLOCK)
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_GETTIME_MONOTONIC
#define SRT_SYNC_CLOCK_STR "GETTIME_MONOTONIC"
#else
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_POSIX_GETTIMEOFDAY
#define SRT_SYNC_CLOCK_STR "POSIX_GETTIMEOFDAY"
#endif

srt::sync::steady_clock is used everywhere to measure the elapsed time, in particular it is used to timestamp packets properly. Using monotonic clock source for sync::steady_clock is a critical thing that should be kept if possible.

2. `pthread_condattr_setclock`

SRT is setting pthread_condattr_setclock(&CondAttribs, CLOCK_MONOTONIC) on a CV so that it uses monotonic clock for waiting.
This is where the initial problem happens because pthread_condattr_setclock is not defined on older android platforms. Therefore CV with monotonic clock can't be used and must be disabled.
pthread_cond_timedwait(&m_cv, &lock.mutex()->ref(), &timeout) has to use gettimeofday instead. And this is where the collision of SRT_SYNC_CLOCK == SRT_SYNC_CLOCK_GETTIME_MONOTONIC happens.
We can and should use monotonic to measure intervals via sync::steady_clock, but we can't use it in sync::Condition.

BTW non-monotonic clock will be used for CV on IA32, IA64, AMD64, WIN32 with pthreads, MAC.

3. Conclusion

Monotonic clock must not be disabled completely as long as it can be at least used for srt::sync::steady_clock.

lilinxiong · 2025-03-13T14:51:31Z

@maxsharabayko Sure, I totally agree with what you said. However, I have another question.
The definition of SRT_SYNC_CLOCK in sync.h is as follows:

// Defile clock type to use
#ifdef IA32 // platform
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_IA32_RDTSC
#define SRT_SYNC_CLOCK_STR "IA32_RDTSC"
#elif defined(IA64) // platform
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_IA64_ITC
#define SRT_SYNC_CLOCK_STR "IA64_ITC"
#elif defined(AMD64) // platform
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_AMD64_RDTSC
#define SRT_SYNC_CLOCK_STR "AMD64_RDTSC"
#elif defined(_WIN32) // platform
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_WINQPC
#define SRT_SYNC_CLOCK_STR "WINQPC"
#elif TARGET_OS_MAC // platform
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_MACH_ABSTIME
#define SRT_SYNC_CLOCK_STR "MACH_ABSTIME"
#elif defined(ENABLE_MONOTONIC_CLOCK) // this is not platform
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_GETTIME_MONOTONIC
#define SRT_SYNC_CLOCK_STR "GETTIME_MONOTONIC"
#else
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_POSIX_GETTIMEOFDAY
#define SRT_SYNC_CLOCK_STR "POSIX_GETTIMEOFDAY"
#endif

Regarding the definition here, could we possibly make some modifications, such as changing it to the following:

...
#elif TARGET_OS_MAC // platform
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_MACH_ABSTIME
#define SRT_SYNC_CLOCK_STR "MACH_ABSTIME"
#elif defined(__ANDROID__) // platform
#if defined(ENABLE_MONOTONIC_CLOCK)
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_GETTIME_MONOTONIC
#define SRT_SYNC_CLOCK_STR "GETTIME_MONOTONIC"
#endif
#else
#define SRT_SYNC_CLOCK SRT_SYNC_CLOCK_POSIX_GETTIMEOFDAY
#define SRT_SYNC_CLOCK_STR "POSIX_GETTIMEOFDAY"
#endif

Because here, the priority of the platform is higher than our self-defined ENABLE_MONOTONIC_CLOCK.

Or to put it another way, in our CMakeLists.txt, we do not differentiate between platforms and always define ENABLE_MONOTONIC_CLOCK as either OFF or ON. This is actually ineffective on some systems and can also cause misunderstandings during the cmake configuration phase. As in my last commit, because we always check pthread_condattr_setclock without distinguishing the platform, on Mac, ENABLE_MONOTONIC_CLOCK is reset to OFF, which then leads to both ENABLE_STDCXX_SYNC and ENABLE_MONOTONIC_CLOCK being OFF, resulting in a cmake configuration failure.

check pthread_condattr_setclock
both ENABLE_STDCXX_SYNC and ENABLE_MONOTONIC_CLOCK is OFF

So I’ve made a temporary adjustment to only check for the pthread_condattr_setclock symbol on Android.

From 68b598ddede64632a149dcee8c8b2f1d5de335e2 Mon Sep 17 00:00:00 2001
From: lilinxiong <[email protected]>
Date: Thu, 13 Mar 2025 17:57:23 +0800
Subject: [PATCH] only check Android platforom

---
 CMakeLists.txt | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index b83b316..2035526 100755
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -142,7 +142,11 @@ elseif (POSIX)
 	test_requires_clock_gettime(ENABLE_MONOTONIC_CLOCK_DEFAULT MONOTONIC_CLOCK_LINKLIB)
 endif()
 
-if (ENABLE_MONOTONIC_CLOCK_DEFAULT)
+# If current platform is android both ENABLE_MONOTONIC_CLOCK_DEFAULT is ON so check
+# todo: This is not a very good approach, but in the current project,
+# todo: the platform is not defined independently, and there is a mix of platform and ENABLE_MONOTONIC_CLOCK in src/core/sync.h,
+# todo: which should not be used together. Therefore, for now, we have to write it this way.
+if (ANDROID AND ENABLE_MONOTONIC_CLOCK_DEFAULT)
 	message(STATUS "because enable monotonic clock so start check needed symbol.")
 	list(PREPEND CMAKE_REQUIRED_LIBRARIES "${CMAKE_THREAD_LIBS_INIT}")
 	check_symbol_exists(pthread_condattr_setclock "pthread.h" HAVE_PTHREAD_CONDATTR_SETCLOCK)
-- 
2.45.2

However, this is not very elegant because now only Android is being checked, and none of the others. The reason is that in the current CMakeLists.txt, I seem to be unable to predict on which platform ENABLE_MONOTONIC_CLOCK will be used, because in the CMakeLists.txt, MacOS also belongs to POSIX. Its definition is here:

...
set_if(POSIX       LINUX OR DARWIN OR BSD OR SUNOS OR ANDROID OR (CYGWIN AND CYGWIN_USE_POSIX) OR GNU_OS)
...

Conclusion

For the definition of ENABLE_MONOTONIC_CLOCK, we should only define it in CMakeLists.txt on the platforms where it will be used. Moreover, in terms of coding standards, the macro definitions in sync.h should have the same conditional checks. It cannot be that the condition is based on the platform in the front, while the condition at the back changes to non-platform. These are some of my insights.

ethouris · 2025-03-14T14:41:27Z

CMakeLists.txt

+if (POSIX)
+	# If current platform is POSIX, need check ENABLE_MONOTONIC_CLOCK is ON or ENABLE_STDCXX_SYNC ON
+	if (NOT ENABLE_STDCXX_SYNC AND NOT ENABLE_MONOTONIC_CLOCK)
+		message(FATAL_ERROR "Both ENABLE_STDCXX_SYNC and ENABLE_MONOTONIC_CLOCK are OFF. Exiting. pls Open ENABLE_STDCXX_SYNC ENABLE_MONOTONIC_CLOCK=OFF is DANGEROUS!!")


This message is not comprehensible.

The idea is:

if ENABLE_STDXCC_SYNC is ON, don't check for ENABLE_MONOTONIC_CLOCK presence nor availability of symbol

if ENABLE_STDCXX_SYNC is OFF, default ENABLE_MONOTONIC_CLOCK should be ON.

if ENABLE_MONOTONIC_CLOCK is ON, and the symbol is not available, issue a fatal error.

ethouris · 2025-03-14T14:45:21Z

srtcore/sync_posix.cpp

+    if (pthread_condattr_setclock(&CondAttribs, CLOCK_MONOTONIC) != 0)
+    {
+        pthread_condattr_destroy(&CondAttribs);
+        LOGC(inlog.Warn, log << "pthread_condattr_setclock failed!");


Note that if that call failed, it means that we don't have the monotonic clock set up for the CV. I think such a thing should be reported as Fatal (not Warn).

Suggested change

LOGC(inlog.Warn, log << "pthread_condattr_setclock failed!");

LOGC(inlog.Fatal, log << "IPE: pthread_condattr_setclock failed to set up a monotonic clock for a CV");

fix: disable pthread_condattr_setclock on lower versions of Android

db06f2a

maxsharabayko added this to the v1.5.5 milestone Mar 12, 2025

maxsharabayko added Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core labels Mar 12, 2025

maxsharabayko reviewed Mar 12, 2025

View reviewed changes

srtcore/sync_posix.cpp Outdated Show resolved Hide resolved

srtcore/sync_posix.cpp Outdated Show resolved Hide resolved

maxsharabayko requested changes Mar 12, 2025

View reviewed changes

fix: when we do not use the attr, we need to call destroy to release it.

d4b6863

lilinxiong requested a review from maxsharabayko March 12, 2025 11:15

fix: POSIX synchronization for broader platform support.

b589e9e

If both ENABLE_MONOTONIC_CLOCK and ENABLE_STDCXX_SYNC are OFF, stop t…

de1b39c

…he configuration.

only check Android platforom

68b598d

ethouris reviewed Mar 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: disable pthread_condattr_setclock on lower versions of Android #3138

fix: disable pthread_condattr_setclock on lower versions of Android #3138

lilinxiong commented Mar 12, 2025

maxsharabayko commented Mar 12, 2025 •

edited

Loading

lilinxiong commented Mar 12, 2025

maxsharabayko commented Mar 12, 2025

maxsharabayko left a comment

maxsharabayko left a comment

lilinxiong commented Mar 12, 2025

cl-ment commented Mar 12, 2025

lilinxiong commented Mar 13, 2025

ethouris commented Mar 13, 2025

lilinxiong commented Mar 13, 2025

ethouris commented Mar 13, 2025

ethouris commented Mar 13, 2025

lilinxiong commented Mar 13, 2025

lilinxiong commented Mar 13, 2025

maxsharabayko commented Mar 13, 2025 •

edited

Loading

lilinxiong commented Mar 13, 2025

ethouris Mar 14, 2025

ethouris Mar 14, 2025

	LOGC(inlog.Warn, log << "pthread_condattr_setclock failed!");
	LOGC(inlog.Fatal, log << "IPE: pthread_condattr_setclock failed to set up a monotonic clock for a CV");

fix: disable pthread_condattr_setclock on lower versions of Android #3138

Are you sure you want to change the base?

fix: disable pthread_condattr_setclock on lower versions of Android #3138

Conversation

lilinxiong commented Mar 12, 2025

maxsharabayko commented Mar 12, 2025 • edited Loading

lilinxiong commented Mar 12, 2025

maxsharabayko commented Mar 12, 2025

maxsharabayko left a comment

Choose a reason for hiding this comment

maxsharabayko left a comment

Choose a reason for hiding this comment

lilinxiong commented Mar 12, 2025

cl-ment commented Mar 12, 2025

lilinxiong commented Mar 13, 2025

ethouris commented Mar 13, 2025

lilinxiong commented Mar 13, 2025

ethouris commented Mar 13, 2025

ethouris commented Mar 13, 2025

lilinxiong commented Mar 13, 2025

lilinxiong commented Mar 13, 2025

maxsharabayko commented Mar 13, 2025 • edited Loading

1. CLOCK_MONOTONIC

2. pthread_condattr_setclock

3. Conclusion

lilinxiong commented Mar 13, 2025

Conclusion

ethouris Mar 14, 2025

Choose a reason for hiding this comment

ethouris Mar 14, 2025

Choose a reason for hiding this comment

maxsharabayko commented Mar 12, 2025 •

edited

Loading

maxsharabayko commented Mar 13, 2025 •

edited

Loading

2. `pthread_condattr_setclock`