[Webkit-unassigned] [Bug 139441] [GStreamer] Rewrite MediaSource implementation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=139441

--- Comment #10 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20141215/3c12c31a/attachment-0002.html>