Bug 162901 – [GStreamer][MSE] Playback pipeline

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

[Enrique Ocaña]

Encapsulate the responsibility to interact with the GStreamer playback pipeline and the different streams managed by the WebKitMediaSrc element into a new PlaybackPipeline class.

[Enrique Ocaña]

Created attachment 290595 [details]
Patch

[Enrique Ocaña]

Comment on attachment 290595 [details]
Patch

Wait until all the patches in 157314 are ready.

[Xabier Rodríguez Calvar]

Comment on attachment 290595 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:40
> +#include <wtf/MainThread.h>
> +#include <wtf/RefCounted.h>
> +#include <wtf/glib/GMutexLocker.h>
> +#include <wtf/glib/GRefPtr.h>
> +#include <wtf/glib/GUniquePtr.h>

These lines are not sorted

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:72
> +    GstBuffer* buffer;
> +    GstCaps* caps;

Move these variables to the first place they are used.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:128
> +    GST_DEBUG_OBJECT(m_webKitMediaSrc.get(), "State %d", (int)GST_STATE(m_webKitMediaSrc.get()));

Do not use C-like casts, please.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:147
> +    gst_app_src_set_callbacks(GST_APP_SRC(stream->appsrc), &enabledAppsrcCallbacks, stream->parent, 0);

0 -> nullptr

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:180
> +    Deque<Stream*>::iterator streamPosition = priv->streams.begin();
> +
> +    while (streamPosition != priv->streams.end()) {
> +        if ((*streamPosition)->sourceBuffer == sourceBufferPrivate.get()) {
> +            stream = *streamPosition;
> +            break;
> +        }
> +        ++streamPosition;
> +    }

I think it is better to use a for loop and if possible a C++11 one.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:192
> +    Stream* stream = nullptr;

You don't need to initialize Stream here. Actually you can declare it when you assign it later.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:193
> +    unsigned padId = 0;

Ditto

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:194
> +    const gchar* mediaType = gst_structure_get_name(structure);

Move to before it is used.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:207
> +    GUniquePtr<gchar> parserBinName(g_strdup_printf("streamparser%u", padId));

Declare this after the GST_DEBUG_OBJECT

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:215
> +        GstElement* parser;
> +        GstElement* capsfilter;
> +        GstPad* pad = nullptr;
> +        GRefPtr<GstCaps> filterCaps = adoptGRef(gst_caps_new_simple("video/x-h264", "alignment", G_TYPE_STRING, "au", nullptr));

Declare these when used. pad should be a GRefPtr.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:236
> +        GstElement* parser;
> +        GstElement* capsfilter;
> +        GstPad* pad = nullptr;
> +        GRefPtr<GstCaps> filterCaps = adoptGRef(gst_caps_new_simple("video/x-h265", "alignment", G_TYPE_STRING, "au", nullptr));

Declare these when used. pad should be a GRefPtr.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:256
> +        GstElement* parser;
> +        GstPad* pad = nullptr;

Declare these when used. pad should be a GRefPtr.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:261
> +        if (mpegversion == 1) {
> +            parser = gst_element_factory_make("mpegaudioparse", nullptr);
> +        } else if (mpegversion == 2 || mpegversion == 4)

You don't need the { }

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:352
> +    ASSERT(stream);
> +    ASSERT(stream->type != Invalid);

You don't need the first ASSERT. Either you shortcircuit with stream && ... or you just use the second cause a segmentation fault is going to have almost the same result as the assertion.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:357
> +    // gst_app_src_set_caps(GST_APP_SRC(stream->appsrc), caps);

Why is this commented out? If it is not useful, then we can remove it.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:364
> +#if (!(LOG_DISABLED || GST_DISABLE_GST_DEBUG))

You don't need LOG_DISABLED here as we don't use LOG_* anymore.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:366
> +        gchar* stroldcaps = gst_caps_to_string(oldAppsrcCaps.get());
> +        gchar* strnewcaps = gst_caps_to_string(appsrcCaps.get());

GUniquePtr

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:430
> +    for (Vector<GstAppSrc*>::iterator iterator = appsrcs.begin(); iterator != appsrcs.end(); ++iterator)
> +        gst_app_src_end_of_stream(*iterator);

Use C++11 for if possible?

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:468
> +    for (Vector<RefPtr<MediaSample>>::iterator iterator = samples.begin(); iterator != samples.end(); ++iterator) {

Use C++11 for if possible?

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:496
> +    GST_TRACE("enqueing sample trackId=%s PTS=%f presentationSize=%.0fx%.0f at %" GST_TIME_FORMAT " duration: %" GST_TIME_FORMAT, trackId.string().utf8().data(), protectedPrSample->presentationTime().toFloat(), protectedPrSample->presentationSize().width(), protectedPrSample->presentationSize().height(), GST_TIME_ARGS(WebCore::toGstClockTime(protectedPrSample->presentationTime().toDouble())), GST_TIME_ARGS(WebCore::toGstClockTime(protectedPrSample->duration().toDouble())));

In WebKit we accept long times, but this is probably too much :)

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:515
> +        GstSample* gstSample = gst_sample_ref(sample->sample());

GRefPtr

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.h:32
> +

Remove extra line

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.h:65
> +    // From MediaSourceGStreamer
> +    void markEndOfStream(MediaSourcePrivate::EndOfStreamStatus);
> +
> +    // From SourceBufferPrivateGStreamer

Periods.

[Enrique Ocaña]

Created attachment 291764 [details]
Patch

[Zan Dobersek]

Comment on attachment 291764 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:94
> +PassRefPtr<PlaybackPipeline> PlaybackPipeline::create()
> +{
> +    return adoptRef(new PlaybackPipeline());
> +}

Return Ref<>. This is usually implemented in the header.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:99
> +PlaybackPipeline::PlaybackPipeline()
> +    : RefCounted<PlaybackPipeline>()
> +{
> +}

The RefCounted<> initialization is always implicit. This constructor definition can be defaulted:

PlaybackPipeline::PlaybackPipeline() = default;

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:103
> +PlaybackPipeline::~PlaybackPipeline()
> +{
> +}

Destructor can be defaulted as well.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:191
> +    GST_OBJECT_LOCK(webKitMediaSrc);
> +    Stream* stream = getStreamBySourceBufferPrivate(webKitMediaSrc, sourceBufferPrivate.get());
> +    GST_OBJECT_UNLOCK(webKitMediaSrc);

I don't think scoped locks are used much with GstObject locks in the current GStreamer code, but IMO they should be. Something for a follow-up patch.

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:314
> +    if (g_str_has_prefix(mediaType, "audio")) {
> +        GST_OBJECT_LOCK(webKitMediaSrc);
> +        stream->type = Audio;
> +        stream->parent->priv->numberOfAudioStreams++;
> +        GST_OBJECT_UNLOCK(webKitMediaSrc);
> +        signal = SIGNAL_AUDIO_CHANGED;
> +
> +        stream->audioTrack = RefPtr<WebCore::AudioTrackPrivateGStreamer>(static_cast<WebCore::AudioTrackPrivateGStreamer*>(trackPrivate.get()));
> +    } else if (g_str_has_prefix(mediaType, "video")) {
> +        GST_OBJECT_LOCK(webKitMediaSrc);
> +        stream->type = Video;
> +        stream->parent->priv->numberOfVideoStreams++;
> +        GST_OBJECT_UNLOCK(webKitMediaSrc);
> +        signal = SIGNAL_VIDEO_CHANGED;
> +
> +        stream->videoTrack = RefPtr<WebCore::VideoTrackPrivateGStreamer>(static_cast<WebCore::VideoTrackPrivateGStreamer*>(trackPrivate.get()));
> +    } else if (g_str_has_prefix(mediaType, "text")) {
> +        GST_OBJECT_LOCK(webKitMediaSrc);
> +        stream->type = Text;
> +        stream->parent->priv->numberOfTextStreams++;
> +        GST_OBJECT_UNLOCK(webKitMediaSrc);
> +        signal = SIGNAL_TEXT_CHANGED;
> +
> +        // FIXME: Support text tracks.
> +    }

Can the webKitMediaSrc object lock be held for this whole block?

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:350
> +    GRefPtr<GstCaps> oldAppsrcCaps = adoptGRef(gst_app_src_get_caps(GST_APP_SRC(stream->appsrc)));
> +    // The caps change is managed by gst_appsrc_push_sample() in enqueueSample() and
> +    // flushAndEnqueueNonDisplayingSamples(), so the caps aren't set from here.
> +    GRefPtr<GstCaps> appsrcCaps = adoptGRef(gst_app_src_get_caps(GST_APP_SRC(stream->appsrc)));
> +    const gchar* mediaType = gst_structure_get_name(gst_caps_get_structure(appsrcCaps.get(), 0));
> +
> +    if (!gst_caps_is_equal(oldAppsrcCaps.get(), appsrcCaps.get())) {
> +        GST_DEBUG("Caps have changed, but reconstructing the sequence of elements is not supported yet");
> +
> +#ifndef GST_DISABLE_GST_DEBUG
> +        GUniquePtr<gchar> stroldcaps(gst_caps_to_string(oldAppsrcCaps.get()));
> +        GUniquePtr<gchar> strnewcaps(gst_caps_to_string(appsrcCaps.get()));
> +        GST_DEBUG("oldcaps: %s", stroldcaps.get());
> +        GST_DEBUG("newcaps: %s", strnewcaps.get());
> +#endif
> +    }

What's the point of this? How can the appsrc caps change between the two gst_app_src_get_caps() calls?

> Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:471
> +    RefPtr<MediaSample> protectedPrSample = prSample;

The method should be receiving RefPtr<> as an argument.

[Enrique Ocaña]

Created attachment 292410 [details]
Patch

[Enrique Ocaña]

Created attachment 292415 [details]
Patch

[Enrique Ocaña]

Created attachment 292782 [details]
Patch

[Enrique Ocaña]

Created attachment 292895 [details]
Patch

[Enrique Ocaña]

Comment on attachment 292895 [details]
Patch

Clearing flags on attachment: 292895

Committed r207882: <http://trac.webkit.org/changeset/207882>

[Enrique Ocaña]

All reviewed patches have been landed.  Closing bug.