[MSE][GStreamer] WebKitMediaSrc rework v2

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alicia Boya García]

This patch (re)introduces a new source element for the MSE playback
pipeline. This is an iteration on the previous work on r249205 that
had to be reverted in r251365 because of flaky playbin3 bugs.

This new version avoids these pitfalls by avoiding flushes in
situations where they are avoidable and known to expose bugs.

This re-land shares many of the advantages that motivated the first
patch: seeks massively oversimplified and greater control by no longer
relying on appsrc. It can handle flushes of individual streams
natively.

Work has also gone into decoupling the state of the frame queue from
the state of WebKitMediaSrc so that the backend can survive
transitions from PAUSED to READY and back which
MediaPlayerPrivateGStreamer creates when playback ends. This latter
effort is not complete, and instead this version adds a special case
to inhibit downwards and back upwards transitions, avoiding any
issues.

It is expected that at this point this new backend is at least as
stable as the previous one. No LayoutTest has got worsened
expectations from adding this backend and some have improved.

[Alicia Boya García]

Created attachment 427349 [details]
Patch

[Philippe Normand]

Comment on attachment 427349 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427349&action=review

> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:253
> +        GST_DEBUG_OBJECT(sink, "new-preroll with PTS=%" GST_TIME_FORMAT, GST_TIME_ARGS(GST_BUFFER_PTS(buffer)));

The new-sample log call uses TRACE, I suppose you deliberately use DEBUG here because pre-roll is going to be much less frequent?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1545
> +    if (isMediaSource() && message->src != GST_OBJECT(m_source.get())) {

Nit: use GST_MESSAGE_SRC()

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1823
> +        GST_DEBUG("Changed state from %s to %s", gst_element_state_get_name(currentState), gst_element_state_get_name(newState));

GST_DEBUG_OBJECT()

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:432
> +    if (m_appsinkCaps && strcmp(capsMediaType(caps.get()), capsMediaType(m_appsinkCaps.get()))) {

Maybe use gst_caps_is_equal()?

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:177
> +    // GStreamer doesn't support zero as a valid playback rate. Instead, that is implemented in WebKit by pausing
> +    // the pipeline.
> +    if (rate <= 0)
> +        rate = 1.0;

Would it make sense to move this to the parent class?

> Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:163
> +    track->enqueueObject(adoptGRef(GST_MINI_OBJECT(gstSample.leakRef())));

Why the cast here? There's GRefPtr template specialization for GstSample, I don't think one is needed for GstMiniObject? :)

> Source/WebCore/platform/graphics/gstreamer/mse/TrackQueue.cpp:49
> +    ASSERT(GST_IS_SAMPLE(object.get()) || GST_IS_EVENT(object.get()));

Ah I understand now the GstMiniObject GRefPtr...

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:150
> +G_DEFINE_TYPE_WITH_CODE(WebKitMediaSrc, webkit_media_src, GST_TYPE_ELEMENT,

WEBKIT_DEFINE_TYPE maybe?

[Alicia Boya García]

Comment on attachment 427349 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427349&action=review

>> Source/WebCore/platform/graphics/gstreamer/GLVideoSinkGStreamer.cpp:253
>> +        GST_DEBUG_OBJECT(sink, "new-preroll with PTS=%" GST_TIME_FORMAT, GST_TIME_ARGS(GST_BUFFER_PTS(buffer)));
> 
> The new-sample log call uses TRACE, I suppose you deliberately use DEBUG here because pre-roll is going to be much less frequent?

Yes.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:432
>> +    if (m_appsinkCaps && strcmp(capsMediaType(caps.get()), capsMediaType(m_appsinkCaps.get()))) {
> 
> Maybe use gst_caps_is_equal()?

Not quite, as some caps are unequal but compatible, e.g. two sets of h264 with different PPS. capsMediaType() also unpacks the inner type when encrypted media is used.

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:177
>> +        rate = 1.0;
> 
> Would it make sense to move this to the parent class?

It's not a clear-cut case. An important difference is that in the general case the pipeline gets the seek event, while in MSE we want the source element to get it, in order to handle seeks before pre-roll. We could add a special case in the general player, but I'm not sure it's the way to go at this point.

>> Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:163
>> +    track->enqueueObject(adoptGRef(GST_MINI_OBJECT(gstSample.leakRef())));
> 
> Why the cast here? There's GRefPtr template specialization for GstSample, I don't think one is needed for GstMiniObject? :)

It would be very neat if that was the case and I could write `track->enqueueObject(WTFMove(gstSample));`. Unfortunately that doesn't work with the current library.

WTF/Headers/wtf/glib/GRefPtr.h:71:68: error: cannot convert ‘_GstSample*’ to ‘_GstMiniObject*’ in initialization
   71 |     template <typename U> GRefPtr(GRefPtr<U>&& o) : m_ptr(o.leakRef()) { }

[Alicia Boya García]

Created attachment 427423 [details]
Patch

[Alicia Boya García]

Created attachment 427545 [details]
Patch

[Alicia Boya García]

Reuploaded because the last time I ran webkit-patch from Source/ instead of from the repo root.

[Xabier Rodríguez Calvar]

Comment on attachment 427545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427545&action=review

Some nits but LGTM. If you decide to implement the WTF suggestions, we'd need another review round.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2731
> +    // In the MSE case stream collection messages are emitted from the main thread right before the initilization segement

segment

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:432
> +    if (m_appsinkCaps && strcmp(capsMediaType(caps.get()), capsMediaType(m_appsinkCaps.get()))) {

Please, g_strcmp

> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:797
> +    case MediaSourceStreamTypeGStreamer::Audio: {
> +        auto specificTrack = WebCore::AudioTrackPrivateGStreamer::create(makeWeakPtr(*m_playerPrivate), id(), sinkSinkPad.get());
> +        gstreamerTrack = specificTrack.ptr();
> +        m_track = makeRefPtr(static_cast<TrackPrivateBase*>(specificTrack.ptr()));
>          break;
> -    case WebCore::MediaSourceStreamTypeGStreamer::Video:
> -        m_track = WebCore::VideoTrackPrivateGStreamer::create(makeWeakPtr(*m_playerPrivate), id(), sinkSinkPad.get());
> +    }
> +    case MediaSourceStreamTypeGStreamer::Video: {
> +        auto specificTrack = WebCore::VideoTrackPrivateGStreamer::create(makeWeakPtr(*m_playerPrivate), id(), sinkSinkPad.get());
> +        gstreamerTrack = specificTrack.ptr();
> +        m_track = makeRefPtr(static_cast<TrackPrivateBase*>(specificTrack.ptr()));
>          break;
> -    case WebCore::MediaSourceStreamTypeGStreamer::Text:
> -        m_track = WebCore::InbandTextTrackPrivateGStreamer::create(id(), sinkSinkPad.get());
> +    }
> +    case MediaSourceStreamTypeGStreamer::Text: {
> +        auto specificTrack = WebCore::InbandTextTrackPrivateGStreamer::create(id(), sinkSinkPad.get());
> +        gstreamerTrack = specificTrack.ptr();
> +        m_track = makeRefPtr(static_cast<TrackPrivateBase*>(specificTrack.ptr()));

I think for these three cases we could do something like:

case MediaSourceStreamTypeGStreamer::Audio:
    gstreamerTrack = WebCore::AudioTrackPrivateGStreamer::create(makeWeakPtr(*m_playerPrivate), id(), sinkSinkPad.get()).ptr();
    break;

Same for video and text.

And then move the m_track assigment to after the switch.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:6
> + * Copyright (C) 2009, 2010, 2011, 2012, 2013, 2016, 2017, 2018, 2019, 2020 Igalia S.L

What's up with us and 2021?

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:206
> +    GST_DEBUG("MediaSource called setReadyState(%p): %s -> %s Current player state: %s Waiting for preroll: %s", this, dumpReadyState(m_mediaSourceReadyState), dumpReadyState(mediaSourceReadyState), dumpReadyState(m_readyState), boolForPrinting(m_isWaitingForPreroll));

I thought I would never say this but this line seems to be very long.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:280
> +        if (!changePipelineState(GST_STATE_PLAYING))
> +            GST_ERROR_OBJECT(pipeline(), "Setting the pipeline to PLAYING failed");

Should't we go harder on this as it looks unrecoverable? Maybe a GST_ELEMENT_ERROR?

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:284
> +        if (!changePipelineState(GST_STATE_PAUSED))
> +            GST_ERROR_OBJECT(pipeline(), "Setting the pipeline to PAUSED failed");

Ditto.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:338
> +    GRefPtr<GstCaps> caps = appendPipeline.appsinkCaps();

Can the caps disappear from the appendPipeline while we are running this? Maybe we don't event need to increse the reference. Not strong opinion anyway.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourcePrivateGStreamer.cpp:146
> +    // See MediaPlayerPrivateGStreamerMSE::asyncStateChangeDone()

. at the end :)

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceTrackGStreamer.cpp:64
> +    WTF::DataMutex<TrackQueue>::LockedWrapper queue(m_queueDataMutex);

I think it could be interesting to create a holdLock function for this so that we can just do something like

auto queue = holdLock(m_queueDataMutex);

Ditto for the other cases below.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceTrackGStreamer.h:45
> +    ~MediaSourceTrackGStreamer();

Shouldn't this be virtual?

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:156
> +    Stream(WebKitMediaSrc* source, GRefPtr<GstPad>&& pad, Ref<MediaSourceTrackGStreamer> track, GRefPtr<GstStream>&& streamInfo)

track can be a &&, right?

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:294
> +    for (RefPtr<MediaSourceTrackGStreamer> track : tracks) {

Can't we make this a const auto&

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:314
> +        // Workaround: gst_element_add_pad() should already call gst_pad_set_active() if the element is PAUSED or

By style, this kind of comments should begin with a FIXME.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:369
> +            DataMutex<Stream::StreamingMembers>::LockedWrapper streamingMembers(stream->streamingMembersDataMutex);

If only we had a holdLock...

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:379
> +    DataMutex<Stream::StreamingMembers>::LockedWrapper streamingMembers(stream->streamingMembersDataMutex);

Ditto

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:383
> +// Called with STREAM_LOCK.

We could be interesting to ASSERT on this, maybe for a TRYLOCK to fail.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:389
> +    DataMutex<Stream::StreamingMembers>::LockedWrapper streamingMembers(stream->streamingMembersDataMutex);

Ditto holdLock.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:397
> +    GST_OBJECT_LOCK(pad);

These LOCKS give me the creeps. I had already told Phil in the past, maybe I am luckier with you :) No strong opinion either.

We could create a ScopedExit to ensure we UNLOCK but for this we would need ScopedExit to have a runEarly() method that is not there unless you write it ;)

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:430
> +        GUniquePtr<char> streamId(g_strdup_printf("mse/%s", stream->track->trackId().string().utf8().data()));

Why U no use makeString?

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:463
> +                DataMutex<Stream::StreamingMembers>::LockedWrapper streamingMembers(stream->streamingMembersDataMutex);

I promise I won't say it again :)

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:493
> +        bool ret = gst_pad_push_event(pad, gst_event_new_segment(&streamingMembers->segment));

ret -> some fuller name

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:531
> +            GUniquePtr<char> fileName { g_strdup_printf("playback-pipeline-pushing-buffer-failed-%s", stream->track->trackId().string().utf8().data()) };

makeString ?

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:626
> +        GST_PAD_STREAM_LOCK(stream->pad.get());

Here you can actually use the ScopedExit without the runEarly!

[Enrique Ocaña]

Comment on attachment 427545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427545&action=review

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:176
> +    if (rate <= 0)

Just to be sure that this observation has been had into account: Are negative rates (backwards playback) unsupported? I'm not saying that they should be. Maybe that's out of scope for now and it's fine that they're unsupported. That would be fine.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourcePrivateGStreamer.cpp:137
>  void MediaSourcePrivateGStreamer::waitForSeekCompleted()

Maybe it makes sense to implement this empty method directly in the header file as waitForSeekCompleted() { }

> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceTrackGStreamer.h:56
> +    void enqueueObject(GRefPtr<GstMiniObject>&&);

If you consider it a good readability improvement, maybe it would be nice to split this method in enqueueSample() and enqueueEvent(). Ideally, the TrackQueue method would also be split in two public methods and have a unified private enqueueObject() implementation.

> Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:152
> +    GST_TRACE("enqueing sample trackId=%s PTS=%f presentationSize=%.0fx%.0f at %" GST_TIME_FORMAT " duration: %" GST_TIME_FORMAT,

Maybe this "PTS=%f" and its sample->presentationTime().toFloat() is code inherited from the time when we were still using floats for times instead of MediaTime. It would be better to use "PTS=%s" and sample->presentationTime().toString().utf8().data().

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:187
> +        GRefPtr<GstCaps> previousCaps; // Caps from enqueued samples are campared to these to push CAPS events as needed.

Typo: "campared" --> "compared"

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:311
> +    for (auto& pair : source->priv->streams) {

Only values are used in this loop. Maybe you can directly iterate on source->priv->streams.values().

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:662
> +    for (auto& pair : source->priv->streams) {

Only values are used in this loop. Maybe you can directly iterate on source->priv->streams.values().

[Xabier Rodríguez Calvar]

Comment on attachment 427545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427545&action=review

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceTrackGStreamer.h:56
>> +    void enqueueObject(GRefPtr<GstMiniObject>&&);
> 
> If you consider it a good readability improvement, maybe it would be nice to split this method in enqueueSample() and enqueueEvent(). Ideally, the TrackQueue method would also be split in two public methods and have a unified private enqueueObject() implementation.

Not that I see it, wouldn't it be even more interesting to use overload the methods and just create two enqueues, one taking GRefPtr<GstSample>&& and other taking GRefPtr<GstEvent>&&?

[Alicia Boya García]

Comment on attachment 427545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427545&action=review

I will have a look at the yak shaving of enqueueObject() and the lock wrappers tomorrow.

>> Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp:797
>> +        m_track = makeRefPtr(static_cast<TrackPrivateBase*>(specificTrack.ptr()));
> 
> I think for these three cases we could do something like:
> 
> case MediaSourceStreamTypeGStreamer::Audio:
>     gstreamerTrack = WebCore::AudioTrackPrivateGStreamer::create(makeWeakPtr(*m_playerPrivate), id(), sinkSinkPad.get()).ptr();
>     break;
> 
> Same for video and text.
> 
> And then move the m_track assigment to after the switch.

That would require a reinterpret_cast<> because TrackPrivateBaseGStreamer is not a derived type of TrackPrivateBase.

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:176
>> +    if (rate <= 0)
> 
> Just to be sure that this observation has been had into account: Are negative rates (backwards playback) unsupported? I'm not saying that they should be. Maybe that's out of scope for now and it's fine that they're unsupported. That would be fine.

The spec doesn't pronounce itself on the issue. We don't support reverse playback on MSE. Neither does Firefox, Chrome or the AVFoundation port of WebKit for that matter. I haven't seen any demand for that feature in the spec GitHub either.

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:206
>> +    GST_DEBUG("MediaSource called setReadyState(%p): %s -> %s Current player state: %s Waiting for preroll: %s", this, dumpReadyState(m_mediaSourceReadyState), dumpReadyState(mediaSourceReadyState), dumpReadyState(m_readyState), boolForPrinting(m_isWaitingForPreroll));
> 
> I thought I would never say this but this line seems to be very long.

I never thought you would ever say this indeed. I'll split it.

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:280
>> +            GST_ERROR_OBJECT(pipeline(), "Setting the pipeline to PLAYING failed");
> 
> Should't we go harder on this as it looks unrecoverable? Maybe a GST_ELEMENT_ERROR?

If this was WebKitMediaSrc, sure. But this is the player, and sending error messages to the bus of the pipeline from outside when a state transition fails feels a bit uncomfortable...

>> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:338
>> +    GRefPtr<GstCaps> caps = appendPipeline.appsinkCaps();
> 
> Can the caps disappear from the appendPipeline while we are running this? Maybe we don't event need to increse the reference. Not strong opinion anyway.

You are right, I will turn it back into a normal pointer.

>> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:314
>> +        // Workaround: gst_element_add_pad() should already call gst_pad_set_active() if the element is PAUSED or
> 
> By style, this kind of comments should begin with a FIXME.

Note this is about something to be fixed in GStreamer, not WebKit. I'm adding a FIXME anyway:

// FIXME: Remove this workaround when the bug gets fixed and versions without the bug are no longer in use.

>> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:383
>> +// Called with STREAM_LOCK.
> 
> We could be interesting to ASSERT on this, maybe for a TRYLOCK to fail.

Unfortunately glib doesn't provide an API to do that. TRYLOCK is not an appropriate mechanism for checking if mutex are locked because:

a) if the mutex is a GRecMutex (as is the case of STREAM_LOCK) trylock will only fail if it's held by a different thread, but not if it's not held at all. Also you need to unlock it afterwards to decrease the lock count.
b) if the mutex is a GMutex, the mutex is not guaranteed to be neither recursive nor not-recursive, and calling trylock from the same thread the mutex is locked is undefined behavior.

>> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:430
>> +        GUniquePtr<char> streamId(g_strdup_printf("mse/%s", stream->track->trackId().string().utf8().data()));
> 
> Why U no use makeString?

Because it wouldn't help me a lot. I would need to do:

GUniquePtr<char> streamId { g_strdup(makeString("mse/", stream->track->trackId()).utf8().data()) };

Which is not much shorter, and involves a string copy. In fact, using makeString() with GStreamer you may even be tempted to think you can get away with this:

GUniquePtr<char> streamId { makeString("mse/", stream->track->trackId()).utf8().data()) };

Which looks idiomatic on the surface, but it's a use-after-free immediately after initialization and a cross-allocator double-free when released. The reason is that the CString we're calling .data() from is a temporary whose lifetime is bound to the expression in which it's defined, and .data() returns a pointer to the internal memory of that CString -- which is only valid for as long that CString is valid.

This is fine if that .data() is passed as an argument to a function that will read it once and then be done with it, it's not fine if we keep the pointer past its lifetime (use-after-free).

Also, GUniquePtr's are freed with g_free(), which we definitely don't want to call with memory from WebKit's fast-malloc that was already freed there (cross-allocator double-free).

>> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:531
>> +            GUniquePtr<char> fileName { g_strdup_printf("playback-pipeline-pushing-buffer-failed-%s", stream->track->trackId().string().utf8().data()) };
> 
> makeString ?

Same problem as in the previous case.

[Philippe Normand]

A couple warnings from clang:

In file included from ../../Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceTrackGStreamer.cpp:33:
../../Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceTrackGStreamer.h:75:10: warning: private field 'm_isReadyForMoreSamples' is not used [-Wunused-private-field]
    bool m_isReadyForMoreSamples { true };
         ^
1 warning generated.
[64/743] Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp.o
In file included from ../../Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:36:
../../Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.h:108:32: warning: private field 'm_sourceBufferPrivateClient' is not used [-Wunused-private-field]
    SourceBufferPrivateClient* m_sourceBufferPrivateClient { nullptr };
                               ^

[Alicia Boya García]

Created attachment 427779 [details]
Patch for landing

[Alicia Boya García]

To not risk delaying things further, I am landing the patch now with all the corrections (including last message about clang warnings) minus the syntactic sugar for lock holders which would require new classes.

[EWS]

Committed r277031 (237344@main): <https://commits.webkit.org/237344@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427779 [details].

[Philippe Normand]

*** Bug 210644 has been marked as a duplicate of this bug. ***