This patch introduces RAII locker classes that wrap GST_OBJECT_LOCK and GST_PAD_STREAM_LOCK to match the style, safety and convenience of locks from WTF. This patch introduces no behavior changes.
Created attachment 428265 [details] Patch
Comment on attachment 428265 [details] Patch Art.
Comment on attachment 428265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428265&action=review > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:323 > + explicit ExternalLocker(T* lockable) : m_lockable(lockable) { lock(); } I wonder if we should ASSERT(lockable)... > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:332 > + T* lockable() { return m_lockable; } > + > + explicit operator bool() const { return !!m_lockable; } Why do you need these? > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:387 > +inline GstObjectLocker holdGstObjectLock(void* gstObject) WARN_UNUSED_RETURN; > +inline GstObjectLocker holdGstObjectLock(void* gstObject) > +{ > + return GstObjectLocker(gstObject); > +} Can't we collapse declaration and implementation? Independently of asserting above, it could be interesting ASSERT(GST_IS_OBJECT(gstObject)) > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:393 > +inline GstPadStreamLocker holdGstPadStreamLock(GstPad*) WARN_UNUSED_RETURN; > +inline GstPadStreamLocker holdGstPadStreamLock(GstPad* pad) > +{ > + return GstPadStreamLocker(pad); > +} Ditto. Ditto.
Comment on attachment 428265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428265&action=review >> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:323 >> + explicit ExternalLocker(T* lockable) : m_lockable(lockable) { lock(); } > > I wonder if we should ASSERT(lockable)... We definitely can. >> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:332 >> + explicit operator bool() const { return !!m_lockable; } > > Why do you need these? It's here just for completeness to match WTF::Locker, even though we don't need all the fancy features at the moment (such as moving locks). This tells you if this locker still holds a lock: it doesn't if it has been moved. >> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:387 >> +} > > Can't we collapse declaration and implementation? > > Independently of asserting above, it could be interesting ASSERT(GST_IS_OBJECT(gstObject)) We can't collapse declaration and implementation because WARN_UNUSED_RETURN can only be used in declarations, not in function definitions.
Comment on attachment 428265 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428265&action=review >>> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:332 >>> + explicit operator bool() const { return !!m_lockable; } >> >> Why do you need these? > > It's here just for completeness to match WTF::Locker, even though we don't need all the fancy features at the moment (such as moving locks). This tells you if this locker still holds a lock: it doesn't if it has been moved. You can keep them. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:384 > +// We can't pass macros as template parameters, so we need to wrap them in inline functions. > +inline void gstObjectLock(void* object) { GST_OBJECT_LOCK(object); } > +inline void gstObjectUnlock(void* object) { GST_OBJECT_UNLOCK(object); } > +inline void gstPadStreamLock(GstPad* pad) { GST_PAD_STREAM_LOCK(pad); } > +inline void gstPadStreamUnlock(GstPad* pad) { GST_PAD_STREAM_UNLOCK(pad); } > + > +using GstObjectLocker = ExternalLocker<void, gstObjectLock, gstObjectUnlock>; > +using GstPadStreamLocker = ExternalLocker<GstPad, gstPadStreamLock, gstPadStreamUnlock>; > + > +inline GstObjectLocker holdGstObjectLock(void* gstObject) WARN_UNUSED_RETURN; > +inline GstObjectLocker holdGstObjectLock(void* gstObject) I would also be tempted of moving ExternalLocker to WTF and just keeping these functions here. WDYT? (It would need a new review, easy I hope) Besides, I think you can just use holdLock as a name and let function overload do its job. WDYT?
Created attachment 428817 [details] Patch
Comment on attachment 428817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428817&action=review > Source/WTF/wtf/Locker.h:188 > + explicit ExternalLocker(T* lockable) : m_lockable(lockable) { lock(); } You missed this ASSERT. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:331 > + return GstObjectLocker(gstObject); Ditto. > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:337 > + return GstPadStreamLocker(pad); Ditto.
Can you clarify the difference between this locker and the WTF::Locker we already have? As far as I know WTF supports all types of locks, not just WTF::Lock.
(In reply to Chris Dumez from comment #8) > Can you clarify the difference between this locker and the WTF::Locker we > already have? As far as I know WTF supports all types of locks, not just > WTF::Lock. I see, you need custom lock()/unlock() functions.
Comment on attachment 428817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428817&action=review > Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:329 > +inline GstObjectLocker holdGstObjectLock(void* gstObject) What do those hold functions bring? Why not call the constructor directly at call sites like we now do for the regular Locker: GstObjectLocker locker { object };
Comment on attachment 428817 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428817&action=review >> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:329 >> +inline GstObjectLocker holdGstObjectLock(void* gstObject) > > What do those hold functions bring? Why not call the constructor directly at call sites like we now do for the regular Locker: > GstObjectLocker locker { object }; They were there for consistency with holdLock(), which was used all across WebKit, until it stopped being used last Friday because the clang thread safety checker couldn't support it. https://bugs.webkit.org/show_bug.cgi?id=226119 Now I have to rework this patch a bit to adapt it to the new checker.
(In reply to Alicia Boya García from comment #11) > Comment on attachment 428817 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428817&action=review > > >> Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.h:329 > >> +inline GstObjectLocker holdGstObjectLock(void* gstObject) > > > > What do those hold functions bring? Why not call the constructor directly at call sites like we now do for the regular Locker: > > GstObjectLocker locker { object }; > > They were there for consistency with holdLock(), which was used all across > WebKit, until it stopped being used last Friday because the clang thread > safety checker couldn't support it. > https://bugs.webkit.org/show_bug.cgi?id=226119 > > Now I have to rework this patch a bit to adapt it to the new checker. I don’t believe clang thread safety analysis will work with GstObject. However, I think it would still be nice to be as consistent as possible with the rest of WebKit. So I think the patch is fine as is but I would just remove the hold functions. I would also have moved the new ExternalLocker to its own header probably.
(In reply to Chris Dumez from comment #12) > I don’t believe clang thread safety analysis will work with GstObject. > However, I think it would still be nice to be as consistent as possible with > the rest of WebKit. So I think the patch is fine as is but I would just > remove the hold functions. I would also have moved the new ExternalLocker to > its own header probably. LGTM
Created attachment 430851 [details] Patch
Committed r278655 (238637@main): <https://commits.webkit.org/238637@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 430851 [details].
<rdar://problem/79068061>
> I don’t believe clang thread safety analysis will work with GstObject. You're right, as even though it would be possible to add lock/unlock annotations, all library GStreamer code would not contain any require_lock annotations.