.
Created attachment 451815 [details] Patch
Comment on attachment 451815 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451815&action=review > Source/WebCore/Modules/mediarecorder/MediaRecorderProvider.cpp:59 > +#if HAVE(AVASSETWRITERDELEGATE) Looks like this introduces test failures on the mac-wk2 EWS. Maybe this should be PLATFORM(COCOA)... or !USE(GSTREAMER_TRANSCODER)
Comment on attachment 451815 [details] Patch There's a deadlock during the transcoder shutdown, need to debug this.
Created attachment 451896 [details] Patch
Comment on attachment 451896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451896&action=review > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:51 > +#if PLATFORM(COCOA) || USE(GSTREAMER_TRANSCODER) Maybe it is time to define an ENABLE flag?
Comment on attachment 451896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451896&action=review >> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:51 >> +#if PLATFORM(COCOA) || USE(GSTREAMER_TRANSCODER) > > Maybe it is time to define an ENABLE flag? Maybe so! Is ENABLE(MEDIA_RECORDER) a good name?
Comment on attachment 451896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451896&action=review Would be interesting to honor what Eric is saying. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivateGStreamer.cpp:85 > + auto scopeExit = makeScopeExit([this, weakThis = WeakPtr { this }, completionHandler = WTFMove(completionHandler)]() mutable { Interesting to see creating a WeakPtr for a scope exit. Can this really happen? Can this object be destroyed while executing this method? If this can happen I would be even more worried about accessing the other attributes, like m_eos, m_eosLock, etc. If this is so, then I guess what we should do is doing a hard ref and making up some invalidation system or at least locking in the destructor. Calling this method on a dead object would be the worst scenario, obviously. > Source/WebCore/platform/mediarecorder/MediaRecorderPrivateGStreamer.cpp:110 > + bool isEOS = false; > + { > + LockHolder lock(m_eosLock); > + isEOS = m_eos; > + } > + while (!isEOS) { > + LockHolder lock(m_eosLock); > + m_eosCondition.waitFor(m_eosLock, 200_ms, [weakThis = WeakPtr { this }]() -> bool { I don't think you need to do this because the waitFor checks the predicate before waiting. > Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:198 > + callOnMainThread([&] { > + if (m_track.hasAudio()) > + m_track.source().removeAudioSampleObserver(*this); > + else if (m_track.hasVideo()) > + m_track.source().removeVideoSampleObserver(*this); > + m_track.removeObserver(*this); > + }); Nope! You call this from the destructor, deferring this to the main thread would mean you're accessing stuff after the object is destroyed. Maybe AndWait can help, but this means r- .
Comment on attachment 451896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451896&action=review >> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateGStreamer.cpp:110 >> + m_eosCondition.waitFor(m_eosLock, 200_ms, [weakThis = WeakPtr { this }]() -> bool { > > I don't think you need to do this because the waitFor checks the predicate before waiting. "this"? >> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:198 >> + }); > > Nope! You call this from the destructor, deferring this to the main thread would mean you're accessing stuff after the object is destroyed. Maybe AndWait can help, but this means r- . Not only from the destructor. As mentioned in the comment a few lines above, the transcoder uses a dedicated thread :) The pipeline state is changed from that thread.
(In reply to Xabier Rodríguez Calvar from comment #7) > Comment on attachment 451896 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451896&action=review > > Would be interesting to honor what Eric is saying. https://bugs.webkit.org/show_bug.cgi?id=236652 :)
(In reply to Philippe Normand from comment #8) > Comment on attachment 451896 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451896&action=review > > >> Source/WebCore/platform/mediarecorder/MediaRecorderPrivateGStreamer.cpp:110 > >> + m_eosCondition.waitFor(m_eosLock, 200_ms, [weakThis = WeakPtr { this }]() -> bool { > > > > I don't think you need to do this because the waitFor checks the predicate before waiting. > > "this"? You don't need the first part, you can initialize as false, let the loop begin, then do the waitFor because it will check the predicate before blocking. > >> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:198 > >> + }); > > > > Nope! You call this from the destructor, deferring this to the main thread would mean you're accessing stuff after the object is destroyed. Maybe AndWait can help, but this means r- . > > Not only from the destructor. As mentioned in the comment a few lines above, > the transcoder uses a dedicated thread :) The pipeline state is changed from > that thread. Well, the problem here is just the destructor, right?
Comment on attachment 451896 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451896&action=review >>>> Source/WebCore/platform/mediastream/gstreamer/GStreamerMediaStreamSource.cpp:198 >>>> + }); >>> >>> Nope! You call this from the destructor, deferring this to the main thread would mean you're accessing stuff after the object is destroyed. Maybe AndWait can help, but this means r- . >> >> Not only from the destructor. As mentioned in the comment a few lines above, the transcoder uses a dedicated thread :) The pipeline state is changed from that thread. > > Well, the problem here is just the destructor, right? There is no issue with the destructor. The issue is that webkitMediaStreamSrcChangeState() is not always called from the main thread, and it has this: if (transition == GST_STATE_CHANGE_PAUSED_TO_READY) { GST_OBJECT_LOCK(self); for (auto& item : self->priv->sources) item->stopObserving(); GST_OBJECT_UNLOCK(self); } We could do the main thread dispatch there, but I thought it was nicer to do it in stopObserving() because that's the method that directly calls to RealtimeMediaSource.
Created attachment 452529 [details] Patch
Created attachment 452633 [details] Patch
Committed r290323 (247645@main): <https://commits.webkit.org/247645@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 452633 [details].
<rdar://problem/89307131>