Bug 99065 – [GStreamer] Add support for Media Source API

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Zan Dobersek]

This bug covers adding support for the Media Source API. The feature was enabled for some time as there was some interest in it, but that kind of died off so the feature was disabled. Guarded by the ENABLE_MEDIA_SOURCE feature define.

[Nadia Hilario]

Hello, we also have interest in Media Source support for the GStreamer backend, although using the QtWebKit port.

I've added to the CC list, including Philippe Normand (philn) as discussed in the channel #webkitgtk+.

Hello Philippe, since I found this existing new bug, I just commented on it, rather than open another.

Thanks and regards

[Philippe Normand]

Nadia, any news to share about this bug?

[Nadia Hilario]

Created attachment 195677 [details]
Media Source Player for the GStreamer Backend (prototype architecture)

Hi Philippe,

I'm sorry for not following up earlier.  The reason is that we did start implementing a Media Source player prototype for the GStreamer backend, but had to put it on hold indefinitely as we changed to a proprietary solution that is still in progress.  Unfortunately, a proposal for the GStreamer Media Source implementation had not yet been prepared.

To summarize the work done, our prototype was implemented in QtWebKit1 (Qt 4.8) and was based on the Media Source specification version 0.5.  We referenced the Chromium implementation to help us understand how Media Source should work.

We know there have been many updates to the spec and to WebKit since then, including the recently added MediaSourcePrivate interface.  In general though, our approach was the following (please also see attached for a diagram of the implemented architecture) :

  *   The existing MediaPlayerPrivateGStreamer engine was modified to support Media Source URIs, in addition to the existing support for web (http/https) URIs.

  *   Similar to the existing WebKitWebSrc gstreamer element that handles web URIs, a new gstreamer element named WebKitMediaSourceSrc was implemented to handle Media Source URIs.

  *   WebKitMediaSourceSrc registers its URI protocol handler, allowing uridecodebin to dynamically create the appropriate source element, either WebKitMediaSourceSrc or WebKitWebSrc, based on the provided URI.

  *   A new class MediaSourceGStreamer was also added to encapsulate the Media Source API implementation.  If a Media Source URI is loaded, MediaPlayerPrivateGStreamer creates a MediaSourceGStreamer object,  to which it forwards Media Source API calls.

  *   Similar to the existing WebKitWebSrc element that uses a StreamingClient object to interface with WebKit's network layer and receive media data,  the WebKitMediaSourceSrc element uses a new MediaSourceClientGStreamer object to interface with MediaSourceGStreamer and receive media segments.

  *   The WebKitMediaSourceSrc element has “sometimes” source pads and creates one appsrc per Source Buffer, when the Media Source API function sourceAddId() is called.

  *   The GStreamer Source Buffer model was not fully analyzed.  For the prototype, a Source Buffer was simply implemented as a struct named WebKitSourceBuffer that is associated with an appsrc.

With this approach, we were able to play simple Media Source examples that append one or many non-overlapping media segments, but we did not finish implementing the full API, nor operations such as pause, seek or getting the buffered time ranges.

Unfortunately, this is all the news we have available at this time.  We were not able to prepare a proper proposal for sharing before changing our approach.

Thanks and best regards.

[Philippe Normand]

Thanks Nadia, the work you folks did seems quite reasonable and interesting! Have you considered upstreaming your current prototype? I believe there is some interest in the community to continue the work, if it can be based on the prototype it would be awesome.

[Nadia Hilario]

Hi Philippe,  
Glad to hear your feedback about the work done, thanks!  

Yes, Bluestreak can release the prototype to the community.  By upstreaming, does this mean submitting a patch and going through the code review process, even though the current functionality is incomplete, the software model is not finalized and the existing Media Source layout tests would fail?  The prototype code would also need to be integrated into the latest WebKit and GStreamer 1.0 ( we used GStreamer 0.10).

As well, I had not yet taken a good look at the Media Source layout tests; but since they already exist, I suppose it would not be required to add more tests with the patch?
 
Apart from the webpage http://www.webkit.org/coding/contributing.html, would you have additional guidelines to follow for contributing code?

Thanks and best regards.

[Philippe Normand]

At this point I would focus on gst 1.0 support only, if possible.
The contributor guidelines should provide you a good enough procedure for the patch submission. If the patch can unskip at least some mediasource layout tests, the better. No need to write new tests.

[Nadia Hilario]

Ok. I'll take a look at gst 1.0.

[Stephane Jadaud]

I had a discussion with Nadia to confirm my interest on working on this bug. 
We have a project which consists on developping an adaptative streaming application (MPEG-DASH) using the QtWebkit port.
I have not a relevant experience on WebKit and before I start working on it, I want to be sure of architecture solution that will be implemented.

Could someone confirm that Nadia's solution is a good starting point:
- Create a MediaSourceGStreamer object
- Create a new gstreamer element: WebKitMediaSourceSrc

Thanks and best regards

[Philippe Normand]

(In reply to comment #8)
> I had a discussion with Nadia to confirm my interest on working on this bug. 
> We have a project which consists on developping an adaptative streaming application (MPEG-DASH) using the QtWebkit port.
> I have not a relevant experience on WebKit and before I start working on it, I want to be sure of architecture solution that will be implemented.
> 
> Could someone confirm that Nadia's solution is a good starting point:
> - Create a MediaSourceGStreamer object
> - Create a new gstreamer element: WebKitMediaSourceSrc
> 
> Thanks and best regards

Yes that would be a good approach I think.

[Stephane Jadaud]

Created attachment 206381 [details]
Patch

[WebKit Commit Bot]

Attachment 206381 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.pri', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/WebKitSourceBuffer.cpp', u'Source/WebCore/platform/graphics/gstreamer/WebKitSourceBuffer.h', u'Source/WebCore/platform/qt/MIMETypeRegistryQt.cpp']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:50:  webkit_media_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:83:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:118:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:121:  webkit_media_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:132:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:134:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:139:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:141:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:150:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:362:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 11 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephane Jadaud]

I have pushed a partial implementation with restrictions: seek, buffered time ranges and Multisource not supported.

This patch has been tested on websites:
http://html5-demos.appspot.com/static/media-source.html
http://www-itec.uni-klu.ac.at/dash/js/dashtest.html

http://youtube.com, videos are played without audio (due to multisource not supported)

failed on websites:
http://dash-mse-test.appspot.com/dash-player.html
http://downloads.webmproject.org/adaptive-demo/adaptive/dash-player.html

Now, i will investigate these issues

[Philippe Normand]

Comment on attachment 206381 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:192
> +        isValid &= gst_element_register(0, "webkitwebsrc", GST_RANK_PRIMARY + 100, WEBKIT_TYPE_WEB_SRC);

In which case the element would fail to be registered? Not sure this isValid variable is needed.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:196
> +    srcFactory = gst_element_factory_find("webkitmediasrc");

I think srcFactory leaks here. Please use a GRefPtr like above

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:333
> +    if (!initializeGStreamerAndRegisterWebKitElements())

How about calling ::load(url) instead of copy/pasting code here?

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:56
> +MediaSourceGStreamer::AddStatus MediaSourceGStreamer::addSourceBuffer(const String& type, const CodecsArray& codecsArray, OwnPtr<SourceBufferPrivate>* sourceBufferPrivate)

Wow, an OwnPtr<>* ? First time I see that.

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:62
> +double MediaSourceGStreamer::duration()

This should be const I suppose :) Usually for one-line getters we define/implement directly in the header.

[Philippe Normand]

Also, there seems to be a lot of code in common between our http src element and the new blobsrc element. It'd be great if it can be avoided, perhaps using a common base class?

[Sebastian Dröge (slomo)]

(In reply to comment #14)
> Also, there seems to be a lot of code in common between our http src element and the new blobsrc element. It'd be great if it can be avoided, perhaps using a common base class?

Or just add blob support to webkitwebsrc?

[Stephane Jadaud]

To respect architecture, i need to multiplex an Audio stream and a video stream with playbin but playbin only support one appsrc. do you have any idea ?

[Nicolas Dufresne]

(In reply to comment #16)
> To respect architecture, i need to multiplex an Audio stream and a video stream with playbin but playbin only support one appsrc. do you have any idea ?

It might be a good timing to consider writing your own element. It would be more flexible then appsrc I think.

[Sebastian Dröge (slomo)]

Or you could output a multiplexed stream with both audio/video from appsrc, and then have your custom demuxer to split it again later.

[Gustavo Noronha (kov)]

Comment on attachment 206381 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitSourceBuffer.h:40
> +class WebKitSourceBuffer : public SourceBufferPrivate {

This should probably be called SourceBufferPrivateGStreamer instead.

[Sebastian Dröge (slomo)]

(In reply to comment #18)
> Or you could output a multiplexed stream with both audio/video from appsrc, and then have your custom demuxer to split it again later.

Note that this is independent of using appsrc or not, in general I don't see why appsrc shouldn't be flexible enough for this.

[Istvan Sarkany]

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:534
> +    KURL url(KURL(), uri);

I wonder if this shouldn't be:
KURL url(KURL(), uri + 5);
because else the url.protocolIsInHTTPFamily() call will fail. At least on my setup it does.

I have tested this on QT4.8 with qtwebkit 2.3.2 (I have patched up the media source api from webkit), gstreamer 0.10.36 and I was wondering about the following:
When testing with youtube.com/tv the load() function is called twice, with two different blob urls. Is this normal?
Also how does one find the url for the audio? I'm just beginning to read about the Media Source Extensions and I haven't understood it all yet.
Also what version of the API is used currently in webkit, since it's changing by the day? The current last revision is July 26th.
I'm also interested in this, but I've just begun to study the problem.

[Stephane Jadaud]

(In reply to comment #21)
> > Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:534
> > +    KURL url(KURL(), uri);
> 
> I wonder if this shouldn't be:
> KURL url(KURL(), uri + 5);
> because else the url.protocolIsInHTTPFamily() call will fail. At least on my setup it does.

You are right, I forgot to remove the test with "url.protocolIsInHTTPFamily" when GST_API_VERSION_1 is undefined.


> I have tested this on QT4.8 with qtwebkit 2.3.2 (I have patched up the media source api from webkit), gstreamer 0.10.36 and I was wondering about the following:
> When testing with youtube.com/tv the load() function is called twice, with two different blob urls. Is this normal?

I see the same thing, i don't have any explanations

[Philippe Normand]

Comment on attachment 206381 [details]
Patch

r- as per review comments. Looking forward the next iteration!

[Jer Noble]

Just a heads up: I'm working on merging in changes in MediaSource from Blink in bug #118752, which may break this patch, as some of the class structure of MediaSource objects will change. The breakage should be minimal and easily fixed however.

[Stephane Jadaud]

Here is a status of my progress.
I had worked on a custom demuxer solution: I had a specific header on audio and video buffers in appsrc. The demux detetects headers and redirects buffers on audio or video pad.  I am having troubles with  following demuxer (qtdemux,..) although i set the offset and renegociated caps. There is no problem if the stream is only composed of video buffers or audio buffers. i am stuck on this problem.

I have tried another solution: Create a bin with 2 appsrc. Uridecodebin manages sources with multiple pads if they are dynamics. This solution works on a test application. When i integrate it in Webkit and test with the website http://dashif.org/reference/players/javascript/0.2.5/index.html, the delay to feed first video and next audio seams to cause problems.

[Stephane Jadaud]

Created attachment 211776 [details]
Patch

[WebKit Commit Bot]

Attachment 211776 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/mediasource/MediaSource.cpp', u'Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp', u'Source/WebCore/Modules/mediasource/SourceBuffer.h', u'Source/WebCore/Modules/mediasource/SourceBufferList.h', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.pri', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h', u'Source/WebCore/platform/qt/MIMETypeRegistryQt.cpp']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:50:  webkit_media_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:56:  Do not use 'using namespace WebCore;'.  [build/using_namespace] [4]
Source/WebCore/Modules/mediasource/SourceBuffer.h:40:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:92:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:93:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:94:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:152:  webkit_media_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:184:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:254:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 9 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephane Jadaud]

The new delivery takes into account the various comments.

Regarding the comment:
> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:56
> +MediaSourceGStreamer::AddStatus MediaSourceGStreamer::addSourceBuffer(const String& type, const CodecsArray& codecsArray, OwnPtr<SourceBufferPrivate>* sourceBufferPrivate)
Wow, an OwnPtr<>* ? First time I see that.

I have taken what is already done in the source code file "MediaSourcePrivateImpl.cpp" in Chromium

As said earlier, I deliver a solution with a bin including 2 appsrc. However, this solution works with a test application but returns systematically "streaming task paused, reason not-linked (-1)" in webkit.

As a consequence i have deactivated audio in this delivery (AUDIO_APPSRC not set)
Does someone have a solution to progress
For your information, I realized my tests with gstreamer-1.0.6 on the website http://dashif.org/reference/players/javascript/0.2.5/index.html

[Stephane Jadaud]

Created attachment 211784 [details]
GStreamer pipeline diagram with 2 appsrc

I join a GStreamer pipeline diagram of the bin with 2 appsrc in case of application test.

[Stephane Jadaud]

Created attachment 214551 [details]
Patch

[WebKit Commit Bot]

Attachment 214551 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/mediasource/MediaSource.idl', u'Source/WebCore/Modules/mediasource/SourceBuffer.h', u'Source/WebCore/Modules/mediasource/SourceBuffer.idl', u'Source/WebCore/Modules/mediasource/SourceBufferList.idl', u'Source/WebCore/Modules/mediasource/WebKitMediaSource.idl', u'Source/WebCore/Modules/mediasource/WebKitSourceBufferList.idl', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:50:  webkit_media_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:56:  Do not use 'using namespace WebCore;'.  [build/using_namespace] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:94:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:95:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:96:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:154:  webkit_media_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:186:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephane Jadaud]

(In reply to comment #30)
> Created an attachment (id=214551) [details]
> Patch

I deliver a new version that support both Audio and Video.
The delivery also includes a patch that fixes a regression included in bug #118752 (seen with Jer Noble)

[Philippe Normand]

Comment on attachment 214551 [details]
Patch

Can you please submit the changes not specific to the GStreamer backend in a separate bug?

[Stephane Jadaud]

(In reply to comment #33)
> (From update of attachment 214551 [details])
> Can you please submit the changes not specific to the GStreamer backend in a separate bug?

The bug related to the regression is bug #123021

[Stephane Jadaud]

Created attachment 214575 [details]
Patch

[Stephane Jadaud]

(In reply to comment #35)
> Created an attachment (id=214575) [details]
> Patch

new version depends on bug #123021

[WebKit Commit Bot]

Attachment 214575 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/mediasource/SourceBuffer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:50:  webkit_media_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:56:  Do not use 'using namespace WebCore;'.  [build/using_namespace] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:94:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:95:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:96:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:154:  webkit_media_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:186:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 7 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jer Noble]

Comment on attachment 214575 [details]
Patch

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

> Source/WebCore/Modules/mediasource/SourceBuffer.h:43
> +#include <runtime/Uint8Array.h>

What's this for?

[Stephane Jadaud]

(In reply to comment #38)
> (From update of attachment 214575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214575&action=review
> 
> > 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.

[Philippe Normand]

Comment on attachment 214575 [details]
Patch

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?

[Stephane Jadaud]

Created attachment 215491 [details]
Patch

[WebKit Commit Bot]

Attachment 215491 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/platform/graphics/MediaSourcePrivate.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h', u'Source/WebCore/platform/gtk/MIMETypeRegistryGtk.cpp']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:50:  webkit_media_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:56:  Do not use 'using namespace WebCore;'.  [build/using_namespace] [4]
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:36:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:37:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:39:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:91:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:92:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:136:  webkit_media_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:199:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:36:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:40:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 14 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

- Please fix as much style errors as possible
- Provide documentation and an explanation of the patch in the changelog
- media-source needs to be enabled by default in build-webkit --gtk
- about the blob uri scheme, I think the element should handle a different scheme (mediasource:// ?) and the player should transparently transform the scheme when loading the resource (see how it is done in bug 108088)
- I'm not sure a bin is the best approach for an element with sometimes-pads, have you checked how other elements are implemented?

[Stephane Jadaud]

Created attachment 216400 [details]
Patch

[WebKit Commit Bot]

Attachment 216400 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/platform/graphics/MediaSourcePrivate.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h', u'Tools/ChangeLog', u'Tools/Scripts/webkitperl/FeatureList.pm']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:50:  webkit_media_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:56:  Do not use 'using namespace WebCore;'.  [build/using_namespace] [4]
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:36:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:38:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:40:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:91:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:92:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:136:  webkit_media_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:199:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:36:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:40:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 14 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Stephane Jadaud]

(In reply to comment #43)

New update

> - about the blob uri scheme, I think the element should handle a different scheme (mediasource:// ?) and the player should transparently transform the scheme when loading the resource (see how it is done in bug 108088)

The new uri scheme is now "mediasourceblob://"

> - I'm not sure a bin is the best approach for an element with sometimes-pads, have you checked how other elements are implemented?

I am inspired by what is done in WebKitWebSrc, and the solution of a bin with 2 appsrc seems to be the best approach.

[Build Bot]

Comment on attachment 216400 [details]
Patch

Attachment 216400 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22718492

[Jer Noble]

Comment on attachment 216400 [details]
Patch

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

> Source/WebCore/platform/graphics/MediaSourcePrivate.h:54
> -    virtual double duration() = 0;
> +    virtual double duration() const { return 0; }

Why was this done? (This is the cause of the failing Mac EWS bot.)

[Build Bot]

Comment on attachment 216400 [details]
Patch

Attachment 216400 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22778455

[kov's GTK+ EWS bot]

Comment on attachment 216400 [details]
Patch

Attachment 216400 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/22728494

[Nick Diego Yamane (diegoyam)]

(In reply to comment #46)
> (In reply to comment #43)
> 
> New update
> 
> > - about the blob uri scheme, I think the element should handle a different scheme (mediasource:// ?) and the player should transparently transform the scheme when loading the resource (see how it is done in bug 108088)
> 
> The new uri scheme is now "mediasourceblob://"
> 
> > - I'm not sure a bin is the best approach for an element with sometimes-pads, have you checked how other elements are implemented?
> 
> I am inspired by what is done in WebKitWebSrc, and the solution of a bin with 2 appsrc seems to be the best approach.

WebKitWebAudioSrc is currently being reworkded by Philip.
See https://bugs.webkit.org/show_bug.cgi?id=123015

[Philippe Normand]

(In reply to comment #48)
> (From update of attachment 216400 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216400&action=review
> 
> > Source/WebCore/platform/graphics/MediaSourcePrivate.h:54
> > -    virtual double duration() = 0;
> > +    virtual double duration() const { return 0; }
> 
> Why was this done? (This is the cause of the failing Mac EWS bot.)

Oh I think it's a suggestion I made earlier. Stephane, feel free to revert that part of the patch :)

(In reply to comment #51)
> > 
> > I am inspired by what is done in WebKitWebSrc, and the solution of a bin with 2 appsrc seems to be the best approach.
> 
> WebKitWebAudioSrc is currently being reworkded by Philip.
> See https://bugs.webkit.org/show_bug.cgi?id=123015

TheWebAudioSrc is a totally different beast than the WebSrc element. You're confused :)

[Philippe Normand]

The GTK EWS fails because of:

../../Source/WebCore/testing/Internals.cpp:158:40: fatal error: MockMediaPlayerMediaSource.h: No such file or directory

Can you please fix that in your patch?

[Philippe Normand]

Comment on attachment 216400 [details]
Patch

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

Can this feature be limited to gst 1.x? We plan to remove support for 0.10 so I'd rather not increase the amount of to-be-removed code.

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:92
> +MediaPlayer::ReadyState MediaSourceGStreamer::readyState() const
> +{
> +    return m_readyState;
> +}
> +
> +void MediaSourceGStreamer::setReadyState(MediaPlayer::ReadyState readyState)
> +{
> +    m_readyState = readyState;
> +}

Simple getters and setters like this should be implemented in the header file.

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h:17
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.

Are you sure this copyright is what you want to use?

> Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:15
> + *     * Neither the name of Google Inc. nor the names of its

Ditto

> Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:75
> +MediaPlayer::ReadyState SourceBufferPrivateGStreamer::readyState() const
> +{
> +    return m_readyState;
> +}
> +
> +void SourceBufferPrivateGStreamer::setReadyState(MediaPlayer::ReadyState readyState)
> +{
> +    m_readyState = readyState;
> +}

Ditto about implementation in header

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:36
> + * - webkitsrc may be created from any thread inside gstreamer

This is false, un-needed copy/paste from webkitsrc

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:52
> +typedef struct _Sources {

Why plural?

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:55
> +    guint sourceid;        /* To control the GSource */

sourceId

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:68
> +    guint startID;
> +    guint stopID;
> +    guint needDataID;
> +    guint enoughDataID;
> +    guint seekID;

Can these be renamed as well?

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:78
> +    Sources source[MAX_SOURCES];

sources

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:164
> +    GstPad* gpad;

ghostPad

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:471
> +        priv->source[MEDIA_TYPE_VIDEO].startID = g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc) webKitMediaVideoSrcStart, gst_object_ref(src), (GDestroyNotify) gst_object_unref);
> +        priv->source[MEDIA_TYPE_AUDIO].startID = g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc) webKitMediaAudioSrcStart, gst_object_ref(src), (GDestroyNotify) gst_object_unref);

It'd be good to give a name to each source

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:478
> +        priv->source[MEDIA_TYPE_VIDEO].stopID = g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc) webKitMediaVideoSrcStop, gst_object_ref(src), (GDestroyNotify) gst_object_unref);
> +        priv->source[MEDIA_TYPE_AUDIO].stopID = g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc) webKitMediaAudioSrcStop, gst_object_ref(src), (GDestroyNotify) gst_object_unref);

Ditto

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:693
> +    priv->source[MEDIA_TYPE_VIDEO].needDataID = g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc) webKitMediaVideoSrcNeedDataMainCb, gst_object_ref(src), (GDestroyNotify) gst_object_unref);

Ditto

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:710
> +    priv->source[MEDIA_TYPE_AUDIO].needDataID = g_timeout_add_full(G_PRIORITY_DEFAULT, 0, (GSourceFunc) webKitMediaAudioSrcNeedDataMainCb, gst_object_ref(src), (GDestroyNotify) gst_object_unref);

And here too

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:888
> +    priv->duration = duration >= 0.0 ? (gint64)(duration*GST_SECOND) : 0;

Please use C++ style casts

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:897
> +    if (!strncmp(type, "video", 5)) {

Please use a String here

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:911
> +        priv->source[MEDIA_TYPE_VIDEO].buffer = adoptGRef(createGstBufferForData(data, length));
> +        GST_OBJECT_UNLOCK(m_src);
> +
> +        ret = gst_app_src_push_buffer(GST_APP_SRC(priv->source[MEDIA_TYPE_VIDEO].appsrc),
> +            priv->source[MEDIA_TYPE_VIDEO].buffer.leakRef());

What's the point of keeping the buffer in the source if it's release just after being pushed?

[Stephane Jadaud]

Created attachment 216676 [details]
Patch

[WebKit Commit Bot]

Attachment 216676 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.am', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h', u'Tools/ChangeLog', u'Tools/Scripts/webkitperl/FeatureList.pm']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:50:  webkit_media_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:56:  Do not use 'using namespace WebCore;'.  [build/using_namespace] [4]
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:36:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:38:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:40:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:73:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:74:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:115:  webkit_media_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:174:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:36:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:38:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:40:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 14 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Almost all the style errors are quite easy to fix, shouldn't take much time.

[Stephane Jadaud]

Created attachment 216796 [details]
Patch

[WebKit Commit Bot]

Attachment 216796 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.am', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h', u'Tools/ChangeLog', u'Tools/Scripts/webkitperl/FeatureList.pm']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:50:  webkit_media_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:115:  webkit_media_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:174:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Can you please consider the review I did in comment 54? Thanks.

[Philippe Normand]

Comment on attachment 216796 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:158
> +    gst_element_set_state(element, GST_STATE_PLAYING);

Hum is this really needed? Syncing the state with parent's should be enough.

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:191
> +static void webKitMediaSrcDispose(GObject* object)
> +{
> +    GST_CALL_PARENT(G_OBJECT_CLASS, dispose, (object));
> +}

No need to have this if all the function does is chaining to parent, I guess :)

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:334
> +        if (priv->sourceAudio.appsrc) {
> +            gst_app_src_set_caps(GST_APP_SRC(priv->sourceAudio.appsrc), 0);
> +            if (!seeking)
> +                gst_app_src_set_size(GST_APP_SRC(priv->sourceAudio.appsrc), -1);
> +        }

Dedent this block?

[Stephane Jadaud]

Created attachment 216807 [details]
Patch

[WebKit Commit Bot]

Attachment 216807 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.am', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h', u'Tools/ChangeLog', u'Tools/Scripts/webkitperl/FeatureList.pm']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.h:50:  webkit_media_src_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:114:  webkit_media_src_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:171:  webkit_media_src_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Comment on attachment 216807 [details]
Patch

Ok, I think we can land this and start improving step by step towards better test coverage. Will you carry this on?

[WebKit Commit Bot]

Comment on attachment 216807 [details]
Patch

Clearing flags on attachment: 216807

Committed r159335: <http://trac.webkit.org/changeset/159335>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

(In reply to comment #28)
> For your information, I realized my tests with gstreamer-1.0.6 on the website http://dashif.org/reference/players/javascript/0.2.5/index.html

That doesn't work here

[Philippe Normand]

(In reply to comment #67)
> (In reply to comment #28)
> > For your information, I realized my tests with gstreamer-1.0.6 on the website http://dashif.org/reference/players/javascript/0.2.5/index.html
> 
> That doesn't work here

Sorry I was testing this in the wrong branch, please ignore this comment.

[Michael Catanzaro]

We have a bajillion skipped tests marked against this bug. Reopening. Please either move the tests to a different bug, or else unskip them before closing this one.

I'm also confused why we are skipping all of imported/w3c/web-platform-tests/media-source/, but only a few selected tests under media/media-source/.

[Miguel Gomez]

Skipping also test media/media-source/media-source-seek-detach-crash.html which has been timing out since r207621

[Carlos Alberto Lopez Perez]

(In reply to comment #69)
> We have a bajillion skipped tests marked against this bug. Reopening. Please
> either move the tests to a different bug, or else unskip them before closing
> this one.
> 
> I'm also confused why we are skipping all of
> imported/w3c/web-platform-tests/media-source/, but only a few selected tests
> under media/media-source/.

Opened bug 167108 to keep track of those tests.

Closing this.