https://bugs.winehq.org/show_bug.cgi?id=30557
--- Comment #118 from Andrew Eikum aeikum@codeweavers.com --- (In reply to Sebastian Lackner from comment #117)
While reviewing patch 2/2, I've found a couple of things which probably should be fixed. I am aware that its a very early proof of concept version, but decided to share my feedback nevertheless:
Thanks very much for the review. It needs a little more thought and cleanup, but otherwise I think it's in OK shape for review.
- If wine_get_current_teb() returns != NULL, pthread lock/condition variable
functions are executed on uninitialized memory.
Yup, thanks.
- Instead of CreateThreadpoolWork, I would suggest to use
TrySubmitThreadpoolCallback(). It basically does the same in your case in a single command, instead of three.
OK, I'll look at that.
- "Small" functions should probably be executed directly in the dispatcher
thread, to avoid the additional overhead. Also, it probably should be used as a fallback if creating a threadpool task fails.
This one I'm not sure about. I like the consistency of ALL callbacks going through call_cb, so we only need to avoid Wine code in gst_cbs.c and call_cb. It may be important if any of the cbs are on performance-critical paths, though. I'll do some analysis.
- The thread handle returned by CreateThread is currently leaked. It would
also be useful to implement a mechanism to terminate the dispatcher thread when its no longer required.
I thought about this. One thing is that winegstreamer never unloads (see Gstreamer_init), so having one extra thread idling in the background doesn't sound too terrible to me. Otherwise we have to determine when all objects with callbacks have been destroyed. I'll take another look at it, anyway.
- Functions and global variables in gst_cbs.h should be marked as
DECLSPEC_HIDDEN.
I wanted to avoid including Windows headers in gst_cbs.{h,c}, so any Windows code calls would fail to compile. Unfortunately DECLSPEC_HIDDEN lives in a Windows header. Probably I can just include a Windows header, but I liked the cleanliness of avoiding it :)