Introduce a RealtimeMediaSource video sample observer
Created attachment 399008 [details] Patch
Created attachment 399017 [details] Patch
Comment on attachment 399017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399017&action=review > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:107 > if (m_audioSource) > m_audioSource->removeAudioSampleObserver(*this); > + if (m_videoSource) > + m_videoSource->removeVideoSampleObserver(*this); Maybe add a FIXME about removing these, since they should never be necessary? > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:210 > + auto locker = holdLock(m_videoSampleObserversLock); > + for (auto* observer : m_videoSampleObservers) > + observer->videoSampleAvailable(mediaSample); Clients may perform an arbitrary amount of processing during a callback, so holding the lock for the duration of this loop might be asking for trouble.
Thanks for keeping the GStreamer bits compiling!
(In reply to Philippe Normand from comment #4) > Thanks for keeping the GStreamer bits compiling! np, note that the following step might be to call videoSampleAvailable from a background thread. While this may not break compilation, this might require some GTK changes.
(In reply to Eric Carlson from comment #3) > Comment on attachment 399017 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=399017&action=review > > > Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:107 > > if (m_audioSource) > > m_audioSource->removeAudioSampleObserver(*this); > > + if (m_videoSource) > > + m_videoSource->removeVideoSampleObserver(*this); > > Maybe add a FIXME about removing these, since they should never be necessary? I added them as a just-in-case, maybe we should just keep the ASSERT. The child class should really do it since otherwise, a half-destructed object might still receive samples. > > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:210 > > + auto locker = holdLock(m_videoSampleObserversLock); > > + for (auto* observer : m_videoSampleObservers) > > + observer->videoSampleAvailable(mediaSample); > > Clients may perform an arbitrary amount of processing during a callback, so > holding the lock for the duration of this loop might be asking for trouble. This is the same design as for audio samples. I think it is fine like it is since receiving video samples should not trigger adding/removing observers, and hopefully these actions will be in different threads.
Comment on attachment 399017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399017&action=review >>> Source/WebCore/platform/mediarecorder/MediaRecorderPrivate.h:107 >>> + m_videoSource->removeVideoSampleObserver(*this); >> >> Maybe add a FIXME about removing these, since they should never be necessary? > > I added them as a just-in-case, maybe we should just keep the ASSERT. > The child class should really do it since otherwise, a half-destructed object might still receive samples. The child class destructor should do that early on otherwise, the object, while half destructed, might receive samples to process. I'll add a comment. >>> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:210 >>> + observer->videoSampleAvailable(mediaSample); >> >> Clients may perform an arbitrary amount of processing during a callback, so holding the lock for the duration of this loop might be asking for trouble. > > This is the same design as for audio samples. > I think it is fine like it is since receiving video samples should not trigger adding/removing observers, and hopefully these actions will be in different threads. This lock is locked in add/remove observers which should happen in the main thread while this one should happen in a background thread. Normally, videoSampleAvailable should never synchronously add/remove observers.
(In reply to youenn fablet from comment #5) > (In reply to Philippe Normand from comment #4) > > Thanks for keeping the GStreamer bits compiling! > > np, note that the following step might be to call videoSampleAvailable from > a background thread. > While this may not break compilation, this might require some GTK changes. Indeed, thanks. I'll look this patch today.
Created attachment 399111 [details] Patch for landing
Committed r261553: <https://trac.webkit.org/changeset/261553> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399111 [details].
<rdar://problem/63131874>