Bug 236558 – [GStreamer] Initial MediaRecorder implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

.

[Philippe Normand]

Created attachment 451815 [details]
Patch

[Philippe Normand]

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)

[Philippe Normand]

Comment on attachment 451815 [details]
Patch

There's a deadlock during the transcoder shutdown, need to debug this.

[Philippe Normand]

Created attachment 451896 [details]
Patch

[Eric Carlson]

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?

[Philippe Normand]

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?

[Xabier Rodríguez Calvar]

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- .

[Philippe Normand]

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.

[Philippe Normand]

(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 :)

[Xabier Rodríguez Calvar]

(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?

[Philippe Normand]

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.

[Philippe Normand]

Created attachment 452529 [details]
Patch

[Philippe Normand]

Created attachment 452633 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/89307131>