(3/3) Fix concurrency webcam windows#684
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #684 +/- ##
=======================================
Coverage 42.13% 42.13%
=======================================
Files 86 86
Lines 5186 5186
=======================================
Hits 2185 2185
Misses 2839 2839
Partials 162 162 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1d80925 to
354b5c4
Compare
354b5c4 to
56b68cb
Compare
56b68cb to
1e3f733
Compare
|
Hi @JoTurk this will be among my final contributions to mediadevices for a while :,) could you take a look this week? |
…tyBag, with fallback to display name. Update LDFLAGS in camera_windows.go to include oleaut32 library.
- Introduced mutex for thread safety in camera struct to protect shared resources. - Updated Open method to initialize done channel and manage camera state. - Enhanced Close method to ensure proper cleanup and prevent double closing. - Improved resolution listing logic in camera_windows.cpp by ensuring media types are freed correctly. - Changed memory deletion from single to array deletion for camera properties.
145e352 to
430c428
Compare
:( thank you for all the work you've done.
Yeah, I'll try it to test it today or tomorrow, I have a 2nd machine with Windows, So testing and reviewing these PRs is a lot easier for me now :) |
| C.freeCamera(cam) | ||
| } | ||
|
|
||
| C.free(cbuf) |
There was a problem hiding this comment.
Wouldn't this run into a potential UAF if we have an already running imageCallback callback?
There was a problem hiding this comment.
This is actually safe because freeCamera calls IMediaControl::Stop(), which blocks until any in-flight BufferCB returns. close(done) before freeCamera ensures the callback can't deadlock in the select. So C.free(cbuf) only runs after all callbacks are done.
That said, I moved the RUnlock past the copy so the safety is obvious from the Go side without needing to know DirectShow's Stop() semantics.
There was a problem hiding this comment.
|
Hey @JoTurk could I get a second pass review here? TY |
Summary
CoInitializeExwas only called once ininit(). Go goroutines can run on any OS thread, soOpen()andVideoRecord()now callCoInitializeExon their current thread before making COM calls.COINIT_MULTITHREADEDis idempotent so repeated calls are safe.sync.Mutexprotecting shared camera state (cam,closed,ch,done) to prevent races between concurrentOpen/Close/VideoRecord/Propertiescalls.Closesnapshots fields under the lock, nils them out, then tears down outside the lock. Double close is a no-op.imageCallbackpreviously did a blocking send onch. If no reader was consuming, this blocked the DirectShow streaming thread forever. Added adonechannel to the select soClosecan unblock it.BufferCBcallback writes into a frame buffer from a non-Go thread. Previously this buffer was a Go heap slice whose backing array the GC could relocate, causing silent memory corruption. Now allocated withC.mallocand freed explicitly inClose.delete cam->propstodelete[] cam->props(mismatched new[]/delete). Removed unusedLPOLESTR namevariable inlistResolution.Stacked on top of #683