Bug 139441 – [GStreamer] Rewrite MediaSource implementation

• Component: Media
• Status: RESOLVED FIXED

[Sebastian Dröge (slomo)]

Created attachment 242907 [details]
WIP patch

I'm currently working on cleaning that up, which ended up being a complete rewrite. It works already on YouTube (without seeking), and some of the LayoutTests are succeeding. Much more work is needed tough.

[Sebastian Dröge (slomo)]

I think to handle all this correctly we need to implement an autoplugger for demuxers and parsers inside the source. That way we can notify about the initialization segment information and the media sample information.

[Sebastian Dröge (slomo)]

... and a custom MediaPlayer

[Philippe Normand]

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

[Sebastian Dröge (slomo)]

Created attachment 243023 [details]
WIP patch

Another WIP patch. This one is full of memory leaks and missing locks and assertions, but should be the direction into which this should go.

The previous patch is IMHO ok to be merged, it's clean and works at least better than what was there before. The new patch needs a lot more work to be mergeable but should use the MediaSource APIs properly.

[Sebastian Dröge (slomo)]

Should I prepare a merge-able patch from the first one? IMHO it's good to go, it's at least working better than what was there before and works on YouTube (except for seeking).

[Philippe Normand]

(In reply to comment #5)
> Should I prepare a merge-able patch from the first one? IMHO it's good to
> go, it's at least working better than what was there before and works on
> YouTube (except for seeking).

Ok, can you open a global bug for the pending TODOs and FIXMEs and mention it in the comments?

Also I'll need some time to review the patch, it's not small :)

[Sebastian Dröge (slomo)]

(In reply to comment #6)
> (In reply to comment #5)
> > Should I prepare a merge-able patch from the first one? IMHO it's good to
> > go, it's at least working better than what was there before and works on
> > YouTube (except for seeking).
> 
> Ok, can you open a global bug for the pending TODOs and FIXMEs and mention
> it in the comments?

Will do when I'm at my laptop again later :)

> Also I'll need some time to review the patch, it's not small :)

Just to be sure, I meant https://bugs.webkit.org/attachment.cgi?id=242907 . The other one is even bigger (but can be done on top of the first), and is definitely not ready yet.

[Philippe Normand]

I'll do a first pass on the smallest patch now.

[Philippe Normand]

Comment on attachment 242907 [details]
WIP patch

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

Thanks for working on this! The patch is going in the good direction :)

> Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:61
> +    SourceBufferPrivateClient::AppendResult result;

Please move this below, same line as assignment.

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:29
>  #include "TimeRanges.h"

I think the GThreadSafeMainLoopSource.h #include can be removed now.

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:231
> -    case GST_QUERY_URI: {
> +    case GST_QUERY_URI:

Why removing the block?

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:237
> +    default:{

Please leave that white space alone :)

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:330
> +#if 0
> +    if (GST_STATE(m_src.get()) > GST_STATE_READY) {
> +        GST_ERROR_OBJECT(m_src.get(), "Adding new source buffers after READY state not supported");
> +        return MediaSourcePrivate::NotSupported;
>      }
> +#endif

Please remove this dead code.

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:332
> +    GST_ERROR_OBJECT(m_src.get(), "State %d", (int)GST_STATE(m_src.get()));

We avoid C-style casts in WebKit, I think a reinterpret_cast would work here.

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:365
> +    GstClockTime gstDuration = gst_util_uint64_scale (duration.timeValue(), GST_SECOND, duration.timeScale());

No space before ( :)

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:381
> +    GList *l;

Misplaced *

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:389
> +    for (l = priv->sources; l; l = l->next) {

l could be declared here, how about iter for the name?

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:390
> +        Source *tmp = static_cast<Source*>(l->data);

Misplaced *

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:404
> +    GST_ERROR_OBJECT(m_src.get(), "push buffer %d\n", (int)ret);

No C-style cast please. Also, why using ERROR_OBJECT?

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:409
> +    else
> +        return SourceBufferPrivateClient::ReadStreamFailed;

This can just be return ...Failed; (without else)

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:426
> +    for (l = priv->sources; l; l = l->next) {
> +        Source *source = static_cast<Source*>(l->data);

Same comments about variable declaration, name and misplaced * :)

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:439
> +    GList *l;
> +
> +    for (l = priv->sources; l; l = l->next) {
> +        Source *tmp = static_cast<Source*>(l->data);

Ditto

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:446
> +    if (!source || !source->src)

Perhaps there should be an ASSERT for this.

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:456
> +namespace WTF {
> +template <> GRefPtr<WebKitMediaSrc> adoptGRef(WebKitMediaSrc* ptr)

Hum :) So there's no way to use a GRefPtr<GstElement> ?

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:71
> +class MediaSourceClientGStreamer: public RefCounted<MediaSourceClientGStreamer> {

I think this should have a separate cpp/h files for MediaSourceClientGStreamer, if possible.

[Sebastian Dröge (slomo)]

Next version of that patch will come with a changelog and everything later.

(In reply to comment #9)

> > Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:29
> >  #include "TimeRanges.h"
> 
> I think the GThreadSafeMainLoopSource.h #include can be removed now.

It can, but it will come back later :)

> > Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:231
> > -    case GST_QUERY_URI: {
> > +    case GST_QUERY_URI:
> 
> Why removing the block?

Because no new variables were declared in that block. I'll add it back if you think that's a good idea

> > Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:237
> > +    default:{
> 
> Please leave that white space alone :)

Oops :)

> > Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:404
> > +    GST_ERROR_OBJECT(m_src.get(), "push buffer %d\n", (int)ret);
> 
> No C-style cast please. Also, why using ERROR_OBJECT?

Debug code

> > Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:446
> > +    if (!source || !source->src)
> 
> Perhaps there should be an ASSERT for this.

Indeed

> > Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:456
> > +namespace WTF {
> > +template <> GRefPtr<WebKitMediaSrc> adoptGRef(WebKitMediaSrc* ptr)
> 
> Hum :) So there's no way to use a GRefPtr<GstElement> ?

That would require casting it always back to a WebKitMediaSrc, but it would be possible.

> > Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:71
> > +class MediaSourceClientGStreamer: public RefCounted<MediaSourceClientGStreamer> {
> 
> I think this should have a separate cpp/h files for
> MediaSourceClientGStreamer, if possible.

The main reason why I kept it in this file is that it's just a C++ wrapper for interacting with the GStreamer source element from the other WebKit code without using any GStreamer/GObject API. If I was to move it to a separate file, I would have to move the struct definitions from the source element into a header too or add some C API for the C++ wrapper (but then it becomes riciculous :) )

So I would prefer to have it like this, that C++ class is really the public interface of that source file and the GStreamer source element is just the implementation that is behind it. But maybe we should rename the file to something else then?

[Philippe Normand]

(In reply to comment #10)

> > > Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:71
> > > +class MediaSourceClientGStreamer: public RefCounted<MediaSourceClientGStreamer> {
> > 
> > I think this should have a separate cpp/h files for
> > MediaSourceClientGStreamer, if possible.
> 
> The main reason why I kept it in this file is that it's just a C++ wrapper
> for interacting with the GStreamer source element from the other WebKit code
> without using any GStreamer/GObject API. If I was to move it to a separate
> file, I would have to move the struct definitions from the source element
> into a header too or add some C API for the C++ wrapper (but then it becomes
> riciculous :) )
> 
> So I would prefer to have it like this, that C++ class is really the public
> interface of that source file and the GStreamer source element is just the
> implementation that is behind it. But maybe we should rename the file to
> something else then?

Ok then let's just keep this as it is :)

[Sebastian Dröge (slomo)]

(In reply to comment #11)
> Ok then let's just keep this as it is :)

Ok, that's what I was waiting for :) Will prepare a new patch later then

[Sebastian Dröge (slomo)]

Created attachment 243674 [details]
Patch

[Sebastian Dröge (slomo)]

Comment on attachment 243023 [details]
WIP patch

This patch is not obsolete, it's WIP on top of the newer patch (which should go in already). Future work should happen based on this patch.

[Philippe Normand]

Comment on attachment 243674 [details]
Patch

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

> Source/WebCore/ChangeLog:101
> +        This now is a clean reimplementation around appsrc that works good
> +        enough for YouTube (except for seeking), but it still does not
> +        implement the complete API correctly. Further work is required on
> +        top of this and the Bugzilla ticket linked above contains some
> +        further work in the right direction.

Usually this kind of explanation goes before the list of changed files.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:557
> +    // TODO: Should do more than that, need to notify the media source
> +    // and probably flush the pipeline at least

Missing full stop :) Also for the remaining TODO and FIXMEs can you please open  a new bug and mention it in the comments? I suppose your other patch will address those?

[Sebastian Dröge (slomo)]

Created attachment 243793 [details]
Patch

[Philippe Normand]

Comment on attachment 243793 [details]
Patch

Ok! Let's move the other patch to a new bug then :) Thanks!

[WebKit Commit Bot]

Comment on attachment 243793 [details]
Patch

Clearing flags on attachment: 243793

Committed r177790: <http://trac.webkit.org/changeset/177790>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.