[GStreamer] Do not ref the player count from background threads.
Created attachment 377451 [details] Patch
Comment on attachment 377451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377451&action=review Good! Only a couple of things to tweak before landing. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:353 > + if (m_cdmAttachmentSemaphore.waitFor(4_s) > + && m_notifier->isValid() // Check the player is not being destroyed. > + && !m_cdmInstance->keySystem().isEmpty()) { We need to consider here, to avoid any other crashes, that the semaphore can be either signaled by the destructor or that the timeout can be hit, both without the instance being set. It looks like the third clause with crash is m_cdmInstance is still null. Besides, all this can be one line. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:308 > + Lock m_protectionMutex; // Guards access to m_handledProtectionEvents .
Comment on attachment 377451 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377451&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:307 > + And you can remove this line
(In reply to Xabier Rodríguez Calvar from comment #2) > Comment on attachment 377451 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377451&action=review > > Good! Only a couple of things to tweak before landing. > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:353 > > + if (m_cdmAttachmentSemaphore.waitFor(4_s) > > + && m_notifier->isValid() // Check the player is not being destroyed. > > + && !m_cdmInstance->keySystem().isEmpty()) { > > We need to consider here, to avoid any other crashes, that the semaphore can > be either signaled by the destructor or that the timeout can be hit, both > without the instance being set. It looks like the third clause with crash is > m_cdmInstance is still null. There are two cases, 1) Signal from cdmInstanceAttached. In this case, we know the cdmInstance is not null 2) Signal from dtor, in this case the notifier will have been cancelled in the dtor already, and the second conditional will bail out. So we don't need a further check imo. > Besides, all this can be one line. We then lose the informative comment about why isValid() is there, it's much easier to read conditionals vertically aligned like this and to comment on their conditions.
(In reply to Charlie Turner from comment #4) > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:353 > > > + if (m_cdmAttachmentSemaphore.waitFor(4_s) > > > + && m_notifier->isValid() // Check the player is not being destroyed. > > > + && !m_cdmInstance->keySystem().isEmpty()) { > > > > We need to consider here, to avoid any other crashes, that the semaphore can > > be either signaled by the destructor or that the timeout can be hit, both > > without the instance being set. It looks like the third clause with crash is > > m_cdmInstance is still null. > > There are two cases, > 1) Signal from cdmInstanceAttached. In this case, we know the cdmInstance > is not null > 2) Signal from dtor, in this case the notifier will have been cancelled in > the dtor already, and the second conditional will bail out. 3) waitFor timeout runs out without being signalled from anywhere. > So we don't need a further check imo. > > > Besides, all this can be one line. > > We then lose the informative comment about why isValid() is there, it's much > easier to read conditionals vertically aligned like this and to comment on > their conditions. Right.
(In reply to Xabier Rodríguez Calvar from comment #5) > (In reply to Charlie Turner from comment #4) > > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:353 > > > > + if (m_cdmAttachmentSemaphore.waitFor(4_s) > > > > + && m_notifier->isValid() // Check the player is not being destroyed. > > > > + && !m_cdmInstance->keySystem().isEmpty()) { > > > > > > We need to consider here, to avoid any other crashes, that the semaphore can > > > be either signaled by the destructor or that the timeout can be hit, both > > > without the instance being set. It looks like the third clause with crash is > > > m_cdmInstance is still null. > > > > There are two cases, > > 1) Signal from cdmInstanceAttached. In this case, we know the cdmInstance > > is not null > > 2) Signal from dtor, in this case the notifier will have been cancelled in > > the dtor already, and the second conditional will bail out. > > 3) waitFor timeout runs out without being signalled from anywhere. And in that case, waitFor() returns false. > > > > So we don't need a further check imo. > > > > > Besides, all this can be one line. > > > > We then lose the informative comment about why isValid() is there, it's much > > easier to read conditionals vertically aligned like this and to comment on > > their conditions. > > Right.
We good then.
Created attachment 377692 [details] Patch for landing
Created attachment 377693 [details] Patch for landing
Comment on attachment 377693 [details] Patch for landing Clearing flags on attachment: 377693 Committed r249321: <https://trac.webkit.org/changeset/249321>
All reviewed patches have been landed. Closing bug.