threads: add new mp_thread abstraction #12709

kasper93 · 2023-10-22T04:05:24Z

To resolve issue with hijacking pthread headers/symbols and overall simplify the thread API and win32 implementation in particular. No more hacks, no more internal state, no more strange time conversions.

github-actions · 2023-10-22T04:15:05Z

Download the artifacts for this pull request:

Windows

sfan5 · 2023-10-22T08:40:11Z

I wanted to mention it before you started your work but: is it worth it when we could just switch to pl_thread in the future?

kasper93 · 2023-10-22T14:28:17Z

I wanted to mention it before you started your work but: is it worth it when we could just switch to pl_thread in the future?

I was thinking about it. This would be very good idea, and I initially wanted to do that. Problem is the pl_thread is internal utility header of libplacebo. (also is less capable, than mp_thread currently, but can be extended) Unless it is exported in public API, we cannot feasibly use it. And I don't see why would libplacebo export this in API. In general this is quit lightweight wrapper tailored for our needs.

kasper93 · 2023-10-28T19:41:23Z

Rebase only, no additional changes.

N-R-K · 2023-11-01T13:34:57Z

DOCS/tech-overview.txt

-because its sole purpose is to interrupt a thread waiting via pthread_cond_wait()
+are not variables that signal a condition. mp_cond does not have any
+state per-se. Maybe mp_cond would better be named mp_interrupt,
+because its sole purpose is to interrupt a thread waiting via mp_cond_wait()


Now that we have our own interface, we aren't bound to copy pthread's naming. So this "maybe" can be turned into reality.

It is not an interrupt. It is a way to signal that a new condition exists and that threads waiting for that condition can continue.

Frankly speaking I have not read this tech-overview.txt. mp_thread is a thin pthread wrapper, which with all intention and purpose should work in similar way. Obviously it is limited to a subset of pthread and with higher level abstraction, like offloading the clock conversion from the caller.

I can take a look into updating this docs, but I was surprised in the first place that we have something like that in mpv, it is not really related specifically to mpv and mostly standard threading stuff that we are no place to explain.

Now that we have our own interface, we aren't bound to copy pthread's naming. So this "maybe" can be turned into reality.

I frankly don't agree with this mp_interrupt commentary. Also cond or conditional variable naming is standardized, not only by pthread, but anything else really. Including C++ std::condition_variable and so on. This make easier to understand what mp_cond represents. And it definitely is NOT interrupt.

N-R-K · 2023-11-01T13:41:09Z

DOCS/tech-overview.txt

@@ -555,13 +555,13 @@ Condition variables
 -------------------

 They're used whenever a thread needs to wait for something, without nonsense
-like sleep calls or busy waiting. mpv uses the standard pthread API for this.
+like sleep calls or busy waiting. mpv uses the mp_thread API for this.
 There's a lot of literature on it. Read it.


There isn't lots of literature on mp_thread API :)

Which brings up another point, we probably need some documentation on this new interface, i.e what type of guarantees it makes and doesn't make etc. We can go two routes:

Start fresh. Make the absolute minimum amount of guarantees that mpv code expects/needs and cut the churn.

Make it "based on pthreads" and document deviations.

The latter is probably easier to start off with, but pthreads have lots of subtle details that can be easy to overlook and/or get wrong.

As it is more or less pthreads document it based on that.
After this extra layer is here just because windows is missing it and somewhat difficult to implement in a good way directly.

The latter is probably easier to start off with, but pthreads have lots of subtle details that can be easy to overlook and/or get wrong.

The functionality that mpv uses is pretty basic, we just have mutex/condvar and thread, that's all.

After this extra layer is here just because windows is missing it and somewhat difficult to implement in a good way directly.

@DanOscarsson: Sorry, I don't understand what you mean. You mean that this extra layer is here only for windows, because it is hard to implement otherwise or you mean the new interface is hard to implement on windows directly. Both old and new version was implemented for win32.

And no, this is not a wrapper for Windows. The main point is to abstract implementation details of pthread and clock used. Use mp_time as an internal clock and convert only when needed. Of course there are some adjustment to make Windows wrapper lightweight without the need of tracking thread pids. The main difference that with pthread you can get current thread handle, but not pid and on Windows is the other way around, you can get current thread pid and not handle.

There isn't lots of literature on mp_thread API :)

Updated this line. I've also skimmed through the rest of the threading overview and it flows nicely. Our API is basic ans uses pretty standard primitives and nor the API, nor the users of it cares about small intricacies. I still don't feel like mpv's technical overview is the place to teach threading and in the same time while we define custom mp_thread now, all primitives of it are pretty standardized and knowledge transferable. I don't think we have to do full description of it and developers would be better of it they learn pthreads or std::thread or even win32 threads first... and then usage of this mp_thread is quite standard.

Dudemanguy

I like not hijacking global symbols so overall pretty good. Just minor things. I also noticed that a few of the <pthread.h> removals were in some commits after the use new mp_thread abstraction, so maybe some of the comments I initially made were taken care of later. I would be nicer if all of those were in the use new mp_thread abstraction commit though.

osdep/threads-posix.h

audio/out/ao_pulse.c

demux/demux_lavf.c

player/command.c

video/vaapi.h

osdep/threads-posix.h

Dudemanguy · 2023-11-04T16:15:47Z

Also this would unbreak windows 7 support right? At least in terms of removing the GetSystemTimePreciseAsFileTime call.

kasper93 · 2023-11-04T17:29:49Z

I like not hijacking global symbols so overall pretty good. Just minor things. I also noticed that a few of the <pthread.h> removals were in some commits after the use new mp_thread abstraction, so maybe some of the comments I initially made were taken care of later. I would be nicer if all of those were in the use new mp_thread abstraction commit though.

Not possible. On use new mp_thread abstraction the old win32 pthread wrapper still works. And for it to make work our pthread.h has to be included before any other pthread thread. So we have to keep (or in few places even add) phread.h include the fix the compilation. After mp_thread: add win32 implementation the stray phread.h includes are removed, because at this point we can do that. This is the all mess with hijacking pthread.h ...

Also this would unbreak windows 7 support right? At least in terms of removing the GetSystemTimePreciseAsFileTime call.

Yes, probably.

Dudemanguy · 2023-11-04T17:59:11Z

And for it to make work our pthread.h has to be included before any other pthread thread. So we have to keep (or in few places even add) phread.h include the fix the compilation.

Ah I see. Yeah, the old way it worked always bothered me too.

osdep/threads.h

Dudemanguy

LGTM

kasper93 · 2023-11-04T20:36:38Z

Removed one unused header from test/timer.c, no other changes.

Dudemanguy · 2023-11-05T16:01:52Z

@kasper93: Ping. Looks like the sub thing I just merged caused a merged conflict.

This will allow to avoid hacky pthreads symbols/header override. Inspired by pl_thread from libplacebo.

This change essentially removes mp_thread_self() and instead add mp_thread_id to track threads and have ability to query current thread id during runtime. This will be useful for upcoming win32 implementation, where accessing thread handle is different than on pthreads. Greatly reduces complexity. Otherweis locked map of tid <-> handle is required which is completely unnecessary for all mpv use-cases. Note that this is the mp_thread_id, not to confuse with system tid. For example on threads-posix implementation it is simply pthread_t.

And remove redundant define while at it.

kasper93 · 2023-11-05T17:21:35Z

rebased

audio/out/ao_audiotrack.c

input/ipc-win.c

osdep/threads-win32.h

sfan5 · 2023-11-06T11:15:29Z

osdep/threads.h

-#define pthread_mutex_trylock(...) \
-    mp_ptwrap_mutex_trylock(__FILE__, __LINE__, __VA_ARGS__)
+#define mp_mutex_init_type(mutex, mtype) \
+    assert(!mp_mutex_init_type_internal(mutex, mtype))


threads-win32.h already defines this, mistake?

also if this is never supposed to fail why not drop the _internal split and make the return value void?

Yes, this is fixed. It was quite a big mess up ;p a718677

so is mp_mutex_init_type supposed to return void?

kasper93 mentioned this pull request Oct 22, 2023

timer: adjust mp_time_ns_to_realtime for past time #12664

Closed

kasper93 force-pushed the threads branch 3 times, most recently from c2bbfa6 to 425b4c0 Compare October 22, 2023 05:53

Dudemanguy mentioned this pull request Oct 22, 2023

more macOS stuff #12705

Merged

kasper93 force-pushed the threads branch 4 times, most recently from cd7ca89 to e79eb35 Compare October 28, 2023 19:40

kasper93 force-pushed the threads branch 3 times, most recently from 9871082 to 24ad34c Compare October 28, 2023 19:54

N-R-K reviewed Nov 1, 2023

View reviewed changes

kasper93 force-pushed the threads branch from 24ad34c to 560667e Compare November 3, 2023 15:54

Dudemanguy reviewed Nov 4, 2023

View reviewed changes

kasper93 force-pushed the threads branch from 560667e to 8a28c32 Compare November 4, 2023 17:25

kasper93 force-pushed the threads branch from 8a28c32 to 3ffd561 Compare November 4, 2023 17:41

Dudemanguy reviewed Nov 4, 2023

View reviewed changes

osdep/threads.h Outdated Show resolved Hide resolved

kasper93 force-pushed the threads branch from 3ffd561 to 50dfcfb Compare November 4, 2023 18:34

Dudemanguy reviewed Nov 4, 2023

View reviewed changes

osdep/threads.h Outdated Show resolved Hide resolved

kasper93 force-pushed the threads branch 2 times, most recently from b3ed00c to bba3e99 Compare November 4, 2023 18:44

Dudemanguy approved these changes Nov 4, 2023

View reviewed changes

kasper93 force-pushed the threads branch from bba3e99 to a64394c Compare November 4, 2023 20:35

kasper93 added 10 commits November 5, 2023 18:19

threads: add new mp_thread abstraction

419690b

This will allow to avoid hacky pthreads symbols/header override. Inspired by pl_thread from libplacebo.

ALL: use new mp_thread abstraction

aa8703b

mp_thread: add win32 implementation

d57c484

threads: move pthread debug to threads-posix.h

730fdd7

And remove redundant define while at it.

threads: rename threads.c to threads-posix.c

d882325

threads: remove unused code

ea64082

timer: remove unused code

cacd05e

win32/pthread: remove unused code

de93bdb

timer: remove unnecesary time conversions

061c2f5

kasper93 force-pushed the threads branch from a64394c to 061c2f5 Compare November 5, 2023 17:21

Dudemanguy merged commit 076be24 into mpv-player:master Nov 5, 2023
14 checks passed

kasper93 deleted the threads branch November 5, 2023 17:39

sfan5 reviewed Nov 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

threads: add new mp_thread abstraction #12709

threads: add new mp_thread abstraction #12709

kasper93 commented Oct 22, 2023

github-actions bot commented Oct 22, 2023 •

edited

Loading

sfan5 commented Oct 22, 2023

kasper93 commented Oct 22, 2023 •

edited

Loading

kasper93 commented Oct 28, 2023

N-R-K Nov 1, 2023

DanOscarsson Nov 1, 2023

kasper93 Nov 1, 2023

kasper93 Nov 3, 2023

N-R-K Nov 1, 2023 •

edited

Loading

DanOscarsson Nov 1, 2023

kasper93 Nov 1, 2023

kasper93 Nov 3, 2023 •

edited

Loading

Dudemanguy left a comment •

edited

Loading

Dudemanguy commented Nov 4, 2023

kasper93 commented Nov 4, 2023 •

edited

Loading

Dudemanguy commented Nov 4, 2023

Dudemanguy left a comment

kasper93 commented Nov 4, 2023 •

edited

Loading

Dudemanguy commented Nov 5, 2023

kasper93 commented Nov 5, 2023

sfan5 Nov 6, 2023

kasper93 Nov 6, 2023

sfan5 Nov 6, 2023

threads: add new mp_thread abstraction #12709

threads: add new mp_thread abstraction #12709

Conversation

kasper93 commented Oct 22, 2023

github-actions bot commented Oct 22, 2023 • edited Loading

sfan5 commented Oct 22, 2023

kasper93 commented Oct 22, 2023 • edited Loading

kasper93 commented Oct 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-R-K Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kasper93 Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Dudemanguy left a comment • edited Loading

Choose a reason for hiding this comment

Dudemanguy commented Nov 4, 2023

kasper93 commented Nov 4, 2023 • edited Loading

Dudemanguy commented Nov 4, 2023

Dudemanguy left a comment

Choose a reason for hiding this comment

kasper93 commented Nov 4, 2023 • edited Loading

Dudemanguy commented Nov 5, 2023

kasper93 commented Nov 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 22, 2023 •

edited

Loading

kasper93 commented Oct 22, 2023 •

edited

Loading

N-R-K Nov 1, 2023 •

edited

Loading

kasper93 Nov 3, 2023 •

edited

Loading

Dudemanguy left a comment •

edited

Loading

kasper93 commented Nov 4, 2023 •

edited

Loading

kasper93 commented Nov 4, 2023 •

edited

Loading