[webkit-reviews] review granted: [Bug 225192] [MSE][GStreamer] WebKitMediaSrc rework v2 : [Attachment 427545] Patch

Mon May 3 09:10:31 PDT 2021

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted Alicia Boya García
<aboya at igalia.com>'s request for review:
Bug 225192: [MSE][GStreamer] WebKitMediaSrc rework v2
https://bugs.webkit.org/show_bug.cgi?id=225192

Attachment 427545: Patch

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

--- Comment #7 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 427545
  --> https://bugs.webkit.org/attachment.cgi?id=427545
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.c
pp: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.c
pp: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.c
pp: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.c
pp:284
> +	   if (!changePipelineState(GST_STATE_PAUSED))
> +	       GST_ERROR_OBJECT(pipeline(), "Setting the pipeline to PAUSED
failed");

Ditto.

>
Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.c
pp: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:1
56
> +    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:2
94
> +    for (RefPtr<MediaSourceTrackGStreamer> track : tracks) {

Can't we make this a const auto&

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:3
14
> +	   // 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:3
69
> +	       DataMutex<Stream::StreamingMembers>::LockedWrapper
streamingMembers(stream->streamingMembersDataMutex);

If only we had a holdLock...

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

Ditto

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:3
83
> +// 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:3
89
> +    DataMutex<Stream::StreamingMembers>::LockedWrapper
streamingMembers(stream->streamingMembersDataMutex);

Ditto holdLock.

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:3
97
> +    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:4
30
> +	   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:4
63
> +		   DataMutex<Stream::StreamingMembers>::LockedWrapper
streamingMembers(stream->streamingMembersDataMutex);

I promise I won't say it again :)

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:4
93
> +	   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:5
31
> +	       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:6
26
> +	   GST_PAD_STREAM_LOCK(stream->pad.get());

Here you can actually use the ScopedExit without the runEarly!