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

[bugzilla-daemon at webkit.org]

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

Philippe Normand <pnormand at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #242907|                            |review-
              Flags|                            |

--- Comment #9 from Philippe Normand <pnormand at igalia.com> ---
Comment on attachment 242907
  --> https://bugs.webkit.org/attachment.cgi?id=242907
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.

-- 
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/b6e784f9/attachment-0002.html>