Bug 162900 – [GStreamer][MSE][EME] MSE specialization of Media Player Private for GStreamer

• Component: Platform
• Status: RESOLVED FIXED

[Enrique Ocaña]

Refactor MSE-specific logic to its own GStreamer player. This new MSE player coordinates data appending, media playback and interaction with MediaSource by delegating to AppendPipeline (one instance per SourceBuffer), PlaybackPipeline and MediaSourceClientGStreamerMSE/MediaSourcePrivateClient respectively.

[Enrique Ocaña]

Created attachment 290592 [details]
Patch

[Enrique Ocaña]

Created attachment 290594 [details]
Patch

[Enrique Ocaña]

Comment on attachment 290594 [details]
Patch

Wait until all the patches in 157314 are ready.

[Xabier Rodríguez Calvar]

Comment on attachment 290594 [details]
Patch

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

These things can be fixed at landing time.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:35
> +#include "MIMETypeRegistry.h"

This include is unsorted

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:49
> +

You don't need this extra line

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:111
> +    GST_DEBUG("%p", this);

Meaningful message and also GST_TRACE

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:116
> +    GST_DEBUG("destroying the player");

GST_TRACE

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:136
> +        GST_DEBUG("Unsupported url: %s", urlString.utf8().data());

Shouldn't this be a GST_WARNING or GST_ERROR?

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:162
> +        return { };

I think it is clearer if we return MediaTime();

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:197
> +        GST_DEBUG("Seeking to %f failed", time);

GST_WARNING?

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:213
> +        g_object_set(G_OBJECT(playsink.get()), "send-event-mode", 0, NULL);

NULL -> nullptr

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:371
> +        GST_DEBUG("Returning false");
> +        return false;
> +    }
> +
> +    // The samples will be enqueued in notifySeekNeedsData().
> +    GST_DEBUG("Returning true");

Make these two message more meaningful, please.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:393
> +            GST_DEBUG("[Seek] Seeking to %f failed", m_seekTime);

GST_WARNING?

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:449
> +        GST_DEBUG("Changing pipeline to PAUSED...");
> +        bool ok = changePipelineState(GST_STATE_PAUSED);
> +        GST_DEBUG("Changed pipeline to PAUSED: %s", ok ? "Success" : "Error");

These two messages are bit redundant. If you really think you need them, probably first should be a _TRACE.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:527
> +        m_resetPipeline = (state <= GST_STATE_READY);
> +        if (state <= GST_STATE_READY)

These two lines are funny :) Changing them is your call. I don't mind.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:692
> +        GST_DEBUG("m_mediaSourceClient is null, not doing anything");

My preference: not doing anything -> doing nothing

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:785
> +    for (HashMap<RefPtr<SourceBufferPrivateGStreamer>, RefPtr<AppendPipeline> >::iterator it = m_appendPipelinesMap.begin(); it != m_appendPipelinesMap.end(); ++it) {

> >   ->    >>

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:809
> +    auto position = MediaPlayerPrivateGStreamer::currentMediaTime();

Type this, please.

[Enrique Ocaña]

(In reply to comment #4)

Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:35
> > +#include "MIMETypeRegistry.h"
> 
> This include is unsorted

Sorry, but the style checker thinks different (it uses case-sensitive string comparison):

#include "MediaPlayer.h"
#include "MIMETypeRegistry.h"
#include "NotImplemented.h"

ERROR: Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]

This explanation applies to all the other bugs where you've suggested a case-insensitive header sorting.

Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:213
> > +        g_object_set(G_OBJECT(playsink.get()), "send-event-mode", 0, NULL);
> 
> NULL -> nullptr

The g_object_set() API says (literally) that the last parameter must be NULL. Maybe in some strange platform NULL has a different value than nullptr. Anyway, I'll use nullptr.

https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.html#g-object-set

[Enrique Ocaña]

(In reply to comment #4)

Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:449
> > +        GST_DEBUG("Changing pipeline to PAUSED...");
> > +        bool ok = changePipelineState(GST_STATE_PAUSED);
> > +        GST_DEBUG("Changed pipeline to PAUSED: %s", ok ? "Success" : "Error");
> 
> These two messages are bit redundant. If you really think you need them,
> probably first should be a _TRACE.

changePipelineState() does a lot of things internally, can potentially take some time to complete and even fail internally in the middle. It's interesting to have begin/end log messages to ease the interpretation of the gst log messages of other elements between the begin/end log messages. So ok, I'll change them to GST_TRACE().

Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:785
> > +    for (HashMap<RefPtr<SourceBufferPrivateGStreamer>, RefPtr<AppendPipeline> >::iterator it = m_appendPipelinesMap.begin(); it != m_appendPipelinesMap.end(); ++it) {

I guess you'd like me to use "iterator" instead of "it" here.

[Xabier Rodríguez Calvar]

(In reply to comment #5)
> Source/WebCore/platform/graphics/gstreamer/mse/
> MediaPlayerPrivateGStreamerMSE.cpp:213
> > > +        g_object_set(G_OBJECT(playsink.get()), "send-event-mode", 0, NULL);
> > 
> > NULL -> nullptr
> 
> The g_object_set() API says (literally) that the last parameter must be
> NULL. Maybe in some strange platform NULL has a different value than
> nullptr. Anyway, I'll use nullptr.
> 
> https://developer.gnome.org/gobject/stable/gobject-The-Base-Object-Type.
> html#g-object-set

I agree with you but we have decided some time ago that we won't use NULL but nullptr instead. It is a matter of coding style then.

[Enrique Ocaña]

Created attachment 291763 [details]
Patch

[Enrique Ocaña]

Created attachment 292256 [details]
Patch

[Zan Dobersek]

Comment on attachment 292256 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:109
> +    , m_eosMarked(false)
> +    , m_eosPending(false)
> +    , m_gstSeekCompleted(true)
> +    , m_mseSeekCompleted(true)

These can all be defined in the class declaration in the header.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:119
> +    for (HashMap<RefPtr<SourceBufferPrivateGStreamer>, RefPtr<AppendPipeline>>::iterator iterator = m_appendPipelinesMap.begin(); iterator != m_appendPipelinesMap.end(); ++iterator)
> +        iterator->value->clearPlayerPrivate();

for (auto it : m_appendPipelinesMap)
    it.value->clearPlayerPrivate();

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:150
> +    String mediasourceUri = String::format("mediasource%s", url.utf8().data());
> +    m_mediaSource = mediaSource;
> +    load(mediasourceUri);

Just inline String::format() into the load() call.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:175
> +    float current = currentMediaTime().toDouble();

You're clipping a double value to a float. Using currentMediaTime().toFloat() is the first step. The best solution (for a follow-up patch) would be to override seekMediaTime() and use MediaTime for this.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:351
> +    gint64 startTime, endTime;
> +
> +    if (rate > 0) {
> +        startTime = position;
> +        endTime = GST_CLOCK_TIME_NONE;
> +    } else {
> +        startTime = 0;
> +        endTime = position;
> +    }

Alternatively, initialize startTime to 0 and endTime to `position`, and then override them in case `rate > 0`.

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:491
> +    m_source.clear();

m_source = nullptr;

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:785
> +    for (HashMap<RefPtr<SourceBufferPrivateGStreamer>, RefPtr<AppendPipeline> >::iterator iterator = m_appendPipelinesMap.begin(); iterator != m_appendPipelinesMap.end(); ++iterator) {

for (auto it : m_appendPipelinesMap)

> Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:832
> +    float result = durationMediaTime().toDouble();

Same here, clipping double to a float. Use toFloat() for now, but in the long-term we should switch to the MediaTime version of this method.

[Enrique Ocaña]

Created attachment 292409 [details]
Patch

[Enrique Ocaña]

Created attachment 292894 [details]
Patch

[Enrique Ocaña]

Comment on attachment 292894 [details]
Patch

Clearing flags on attachment: 292894

Committed r207881: <http://trac.webkit.org/changeset/207881>

[Enrique Ocaña]

All reviewed patches have been landed.  Closing bug.