Bug 162900

Summary: [GStreamer][MSE][EME] MSE specialization of Media Player Private for GStreamer
Product: WebKit Reporter: Enrique Ocaña <eocanha>
Component: PlatformAssignee: Enrique Ocaña <eocanha>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 157314    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Enrique Ocaña 2016-10-04 04:55:22 PDT
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.
Comment 1 Enrique Ocaña 2016-10-04 05:02:36 PDT
Created attachment 290592 [details]
Patch
Comment 2 Enrique Ocaña 2016-10-04 05:39:32 PDT
Created attachment 290594 [details]
Patch
Comment 3 Enrique Ocaña 2016-10-04 07:08:07 PDT
Comment on attachment 290594 [details]
Patch

Wait until all the patches in 157314 are ready.
Comment 4 Xabier Rodríguez Calvar 2016-10-05 03:09:42 PDT
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.
Comment 5 Enrique Ocaña 2016-10-05 10:05:18 PDT
(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
Comment 6 Enrique Ocaña 2016-10-06 05:38:12 PDT
(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.
Comment 7 Xabier Rodríguez Calvar 2016-10-08 05:33:11 PDT
(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.
Comment 8 Enrique Ocaña 2016-10-16 12:06:49 PDT
Created attachment 291763 [details]
Patch
Comment 9 Enrique Ocaña 2016-10-20 13:57:39 PDT
Created attachment 292256 [details]
Patch
Comment 10 Zan Dobersek 2016-10-21 00:38:42 PDT
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.
Comment 11 Enrique Ocaña 2016-10-21 14:26:45 PDT
Created attachment 292409 [details]
Patch
Comment 12 Enrique Ocaña 2016-10-26 01:20:56 PDT
Created attachment 292894 [details]
Patch
Comment 13 Enrique Ocaña 2016-10-26 01:44:52 PDT
Comment on attachment 292894 [details]
Patch

Clearing flags on attachment: 292894

Committed r207881: <http://trac.webkit.org/changeset/207881>
Comment 14 Enrique Ocaña 2016-10-26 01:45:00 PDT
All reviewed patches have been landed.  Closing bug.