Bug 99065

Summary: [GStreamer] Add support for Media Source API
Product: WebKit Reporter: Zan Dobersek <zan>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bst-openproject, buildbot, cdumez, clopez, commit-queue, david.corvoysier, dbates, dguehennec, eojin.ham, eric.carlson, esprehn+autocc, glenn, gouache95, gtk-ews, gustavo, gyuyoung.kim, jer.noble, kbalazs, kondapallykalyan, laszlo.gombos, lauro.neto, magomez, mcatanzaro, menard, mnhilario, mrobinson, mtheriault, nick.diego, nicolas, pnormand, rego+ews, rniwa, sarkany.istvan, sergio, sjadaud, slomo, xan.lopez, zan
Priority: P2 Keywords: Gtk, LayoutTestFailure
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=167108
Bug Depends on:    
Bug Blocks: 167107    
Attachments:
Description Flags
Media Source Player for the GStreamer Backend (prototype architecture)
none
Patch
none
Patch
none
GStreamer pipeline diagram with 2 appsrc
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Zan Dobersek 2012-10-11 06:58:14 PDT
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.
Comment 1 Nadia Hilario 2012-11-15 15:23:48 PST
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
Comment 2 Philippe Normand 2013-03-25 07:14:36 PDT
Nadia, any news to share about this bug?
Comment 3 Nadia Hilario 2013-03-28 16:44:24 PDT
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.
Comment 4 Philippe Normand 2013-03-29 00:51:36 PDT
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.
Comment 5 Nadia Hilario 2013-04-03 05:08:24 PDT
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.
Comment 6 Philippe Normand 2013-04-03 07:36:16 PDT
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.
Comment 7 Nadia Hilario 2013-04-03 20:49:35 PDT
Ok. I'll take a look at gst 1.0.
Comment 8 Stephane Jadaud 2013-06-18 09:07:18 PDT
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
Comment 9 Philippe Normand 2013-06-19 01:56:31 PDT
(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.
Comment 10 Stephane Jadaud 2013-07-10 05:39:43 PDT
Created attachment 206381 [details]
Patch
Comment 11 WebKit Commit Bot 2013-07-10 05:41:43 PDT
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.
Comment 12 Stephane Jadaud 2013-07-10 06:00:44 PDT
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
Comment 13 Philippe Normand 2013-07-16 02:48:33 PDT
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.
Comment 14 Philippe Normand 2013-07-16 03:51:10 PDT
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?
Comment 15 Sebastian Dröge (slomo) 2013-07-16 04:07:55 PDT
(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?
Comment 16 Stephane Jadaud 2013-07-19 06:22:53 PDT
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 ?
Comment 17 Nicolas Dufresne 2013-07-19 06:31:32 PDT
(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.
Comment 18 Sebastian Dröge (slomo) 2013-07-19 07:35:00 PDT
Or you could output a multiplexed stream with both audio/video from appsrc, and then have your custom demuxer to split it again later.
Comment 19 Gustavo Noronha (kov) 2013-07-22 15:28:01 PDT
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.
Comment 20 Sebastian Dröge (slomo) 2013-07-22 23:38:07 PDT
(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.
Comment 21 Istvan Sarkany 2013-07-31 04:33:07 PDT
> 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.
Comment 22 Stephane Jadaud 2013-08-01 00:51:08 PDT
(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
Comment 23 Philippe Normand 2013-08-02 06:27:12 PDT
Comment on attachment 206381 [details]
Patch

r- as per review comments. Looking forward the next iteration!
Comment 24 Jer Noble 2013-08-06 10:37:44 PDT
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.
Comment 25 Stephane Jadaud 2013-08-14 10:08:04 PDT
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.
Comment 26 Stephane Jadaud 2013-09-16 06:39:32 PDT
Created attachment 211776 [details]
Patch
Comment 27 WebKit Commit Bot 2013-09-16 06:41:35 PDT
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.
Comment 28 Stephane Jadaud 2013-09-16 06:46:20 PDT
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
Comment 29 Stephane Jadaud 2013-09-16 08:09:16 PDT
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.
Comment 30 Stephane Jadaud 2013-10-18 02:22:37 PDT
Created attachment 214551 [details]
Patch
Comment 31 WebKit Commit Bot 2013-10-18 02:25:25 PDT
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.
Comment 32 Stephane Jadaud 2013-10-18 02:38:47 PDT
(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)
Comment 33 Philippe Normand 2013-10-18 02:44:16 PDT
Comment on attachment 214551 [details]
Patch

Can you please submit the changes not specific to the GStreamer backend in a separate bug?
Comment 34 Stephane Jadaud 2013-10-18 08:14:12 PDT
(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
Comment 35 Stephane Jadaud 2013-10-18 08:35:16 PDT
Created attachment 214575 [details]
Patch
Comment 36 Stephane Jadaud 2013-10-18 08:37:03 PDT
(In reply to comment #35)
> Created an attachment (id=214575) [details]
> Patch

new version depends on bug #123021
Comment 37 WebKit Commit Bot 2013-10-18 08:37:58 PDT
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.
Comment 38 Jer Noble 2013-10-18 12:48:59 PDT
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?
Comment 39 Stephane Jadaud 2013-10-21 01:58:51 PDT
(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.
Comment 40 Philippe Normand 2013-10-22 00:35:46 PDT
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?
Comment 41 Stephane Jadaud 2013-10-30 03:27:52 PDT
Created attachment 215491 [details]
Patch
Comment 42 WebKit Commit Bot 2013-10-30 03:29:23 PDT
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.
Comment 43 Philippe Normand 2013-11-01 02:31:48 PDT
- 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?
Comment 44 Stephane Jadaud 2013-11-08 08:10:49 PST
Created attachment 216400 [details]
Patch
Comment 45 WebKit Commit Bot 2013-11-08 08:12:30 PST
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.
Comment 46 Stephane Jadaud 2013-11-08 08:15:20 PST
(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.
Comment 47 Build Bot 2013-11-08 08:40:33 PST
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
Comment 48 Jer Noble 2013-11-08 08:53:27 PST
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.)
Comment 49 Build Bot 2013-11-08 08:59:53 PST
Comment on attachment 216400 [details]
Patch

Attachment 216400 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22778455
Comment 50 kov's GTK+ EWS bot 2013-11-08 09:38:13 PST
Comment on attachment 216400 [details]
Patch

Attachment 216400 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/22728494
Comment 51 Nick Diego Yamane (diegoyam) 2013-11-08 11:26:12 PST
(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
Comment 52 Philippe Normand 2013-11-09 01:31:53 PST
(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 :)
Comment 53 Philippe Normand 2013-11-11 00:14:45 PST
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?
Comment 54 Philippe Normand 2013-11-12 03:25:42 PST
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?
Comment 55 Stephane Jadaud 2013-11-12 08:35:43 PST
Created attachment 216676 [details]
Patch
Comment 56 WebKit Commit Bot 2013-11-12 08:37:57 PST
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.
Comment 57 Philippe Normand 2013-11-12 08:43:20 PST
Almost all the style errors are quite easy to fix, shouldn't take much time.
Comment 58 Stephane Jadaud 2013-11-13 06:41:46 PST
Created attachment 216796 [details]
Patch
Comment 59 WebKit Commit Bot 2013-11-13 06:44:35 PST
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.
Comment 60 Philippe Normand 2013-11-13 07:15:06 PST
Can you please consider the review I did in comment 54? Thanks.
Comment 61 Philippe Normand 2013-11-13 08:04:20 PST
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?
Comment 62 Stephane Jadaud 2013-11-13 09:00:03 PST
Created attachment 216807 [details]
Patch
Comment 63 WebKit Commit Bot 2013-11-13 09:03:26 PST
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.
Comment 64 Philippe Normand 2013-11-14 01:14:28 PST
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?
Comment 65 WebKit Commit Bot 2013-11-15 07:03:40 PST
Comment on attachment 216807 [details]
Patch

Clearing flags on attachment: 216807

Committed r159335: <http://trac.webkit.org/changeset/159335>
Comment 66 WebKit Commit Bot 2013-11-15 07:03:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 67 Philippe Normand 2013-11-18 04:26:26 PST
(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
Comment 68 Philippe Normand 2013-11-18 06:27:34 PST
(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.
Comment 69 Michael Catanzaro 2016-10-16 18:50:58 PDT
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/.
Comment 70 Miguel Gomez 2016-10-21 07:56:31 PDT
Skipping also test media/media-source/media-source-seek-detach-crash.html which has been timing out since r207621
Comment 71 Carlos Alberto Lopez Perez 2017-01-16 19:17:22 PST
(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.