[webkit-reviews] review denied: [Bug 99065] [GStreamer] Add support for Media Source API : [Attachment 214575] Patch

[bugzilla-daemon at webkit.org]

Philippe Normand <pnormand at igalia.com> has denied Stephane Jadaud
<sjadaud at sii.fr>'s request for review:
Bug 99065: [GStreamer] Add support for Media Source API
https://bugs.webkit.org/show_bug.cgi?id=99065

Attachment 214575: Patch
https://bugs.webkit.org/attachment.cgi?id=214575&action=review

------- Additional Comments from Philippe Normand <pnormand at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=214575&action=review


> Source/WebCore/ChangeLog:8
> +	   No new tests (OOPS!).

Can you please check which mediasource layout tests can run with this patch and
unskip them?

> Source/WebCore/ChangeLog:10
> +	   * Modules/mediasource/SourceBuffer.h:

It would be nice to provide a small description for each function.

>>> Source/WebCore/Modules/mediasource/SourceBuffer.h:43
>>> +#include <runtime/Uint8Array.h>
>> 
>> What's this for?
> 
> It is no longer necessary since integration of bug #118752.

Ok, so please remove it :)

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1229

> +	   MediaSourceGStreamer::create(m_mediaSource.get(),
WEBKIT_MEDIA_SRC(m_source.get()));

This is uncommon, usually the create function passes a RefPtr to the caller.

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:35
> +#if ENABLE(MEDIA_SOURCE)

Missing && USE(GSTREAMER)

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h:31
> +#ifndef MediaSourceGStreamer_h

Add an empty line between the header and the ifndef please

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h:34
> +#if ENABLE(MEDIA_SOURCE)

Missing && USE(GSTREAMER)

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h:36
> +#include "WebKitMediaSourceGStreamer.h"

Please forward declare the classes you need and include the headers only in the
cpp file.

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h:40
> +class MediaSourceGStreamer : public MediaSourcePrivate {

This class can be marked FINAL I guess

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h:45
> +    double duration() { return m_duration; }

Should be const

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h:53
> +    MediaSourceClientGstreamer* m_client;
> +    MediaSourceGStreamer(HTMLMediaSource*, WebKitMediaSrc*);
> +    HTMLMediaSource* m_mediaSource;

Can RefPtrs be used here instead of raw pointers?

>
Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:51
> +    RefPtr<TimeRanges> clientRanges = m_client->buffered((const char
*)m_type.characters8());

We don't use C-style casts. Here a reinterpret_cast might work I think

>
Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:58
> +    m_client->didReceiveData((const char *)data, length, (const char
*)m_type.characters8());

Ditto...

> Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.h:31
> +#ifndef SourceBufferPrivateGStreamer_h

Empty line

> Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.h:34
> +#if ENABLE(MEDIA_SOURCE)

USE(GSTREAMER)

> Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.h:37
> +#include "SourceBufferPrivate.h"
> +#include "WebKitMediaSourceGStreamer.h"

Please forward-declare.

> Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.h:41
> +class SourceBufferPrivateGStreamer : public SourceBufferPrivate {

FINAL

> Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.h:55
> +    MediaSourceClientGstreamer* m_client;

RefPtr?

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:99
> +static GstStaticPadTemplate srcTemplate = GST_STATIC_PAD_TEMPLATE("src",
GST_PAD_SRC, GST_PAD_SOMETIMES, GST_STATIC_CAPS_ANY);

This template is clearly wrong if your element has multiple source pads. This
is why you can't currently enable both video and audio at the same time.... 
I think that for each appsrc element you create you need to add a ghost pad to
your element, not overwrite the existing one....

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:202

> +    g_object_set(src, "add_src", priv->source[MEDIA_TYPE_AUDIO].appsrc,
NULL);

So in theory you want to use a single property to store 2 things? This won't
work I'm afraid.

>
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:1082
> +    ret = gst_element_query_position(priv->playbin, GST_FORMAT_TIME,
&position);

No, this is wrong. A Source element doesn't query the pipeline. What are you
trying to do here?