[Webkit-unassigned] [Bug 99065] [GStreamer] Add support for Media Source API

[bugzilla-daemon at webkit.org]

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


Philippe Normand <pnormand at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #214575|review?                     |review-
               Flag|                            |




--- Comment #40 from Philippe Normand <pnormand at igalia.com>  2013-10-22 00:35:46 PST ---
(From update of attachment 214575)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.