Bug 100261

Summary: [GStreamer] MediaPlayer's code is not easily reusable by other GStreamer-based players
Product: WebKit Reporter: Danilo de Paula <danilo.eu>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, feature-media-reviews, gtk-ews, gustavo, gyuyoung.kim, hta, jonathon, menard, mrobinson, pnormand, rakuco, s.choi, tommyw, webkit.review.bot, xan.lopez, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 87514    
Attachments:
Description Flags
core patch
gtk-ews: commit-queue-
Patch
none
Patch
none
Patch
none
Patch none

Description Danilo de Paula 2012-10-24 09:08:15 PDT
This patch should complete https://bugs.webkit.org/show_bug.cgi?id=94373, which adds the API for the GTK getUserMedia API.

It also contains part of the archtecture for the PeerConnection API,l which will be pushed on the following days.

A more complete patch, with commit message, latter.

Current tests should cover the API.
Comment 1 Danilo de Paula 2012-10-24 09:09:15 PDT
Created attachment 170410 [details]
core patch
Comment 2 WebKit Review Bot 2012-10-24 09:13:23 PDT
Attachment 170410 [details] did not pass style-queue:

Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:37:  Missing space before {  [whitespace/braces] [5]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:124:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:125:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:126:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:127:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:128:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:129:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:130:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:131:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:132:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:133:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:134:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:135:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:136:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:137:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:138:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:139:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:140:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:141:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.cpp:414:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.cpp:618:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:92:  Declaration has space between type name and * in MediaStreamComponent *component  [whitespace/declaration] [3]
Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:93:  Declaration has space between type name and * in MediaStreamSource *source  [wFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.am', u'Source/W..." exit_code: 1
hitespace/declaration] [3]
Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:100:  Declaration has space between type name and * in MediaStreamComponent *component  [whitespace/declaration] [3]
Source/WebCore/platform/mediastream/gstreamer/MediaStreamCenterPrivateGStreamer.cpp:101:  Declaration has space between type name and * in MediaStreamSource *source  [whitespace/declaration] [3]
Total errors found: 25 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Philippe Normand 2012-10-24 09:27:22 PDT
Do you plan to factor the copy-pasted code from the mediaplayer to some common module?
Comment 4 Danilo de Paula 2012-10-24 09:34:20 PDT
(In reply to comment #3)
> Do you plan to factor the copy-pasted code from the mediaplayer to some common module?

philippe: actually I think that StreamMediaPlayer and the regular MediaPlayer could be merged in a near future.

But I want to do it in a safe way.
My idea is to push it as a different player now, keep a bug to reminds me/us to remove it in the near future. The thing is that I don't want to mess up with the current working mediaplayer without testing StreamMediaPlayer pieces in the real world.

What do you think about that?
Comment 5 Philippe Normand 2012-10-24 09:41:22 PDT
We have buildbots reasonably testing the current player. If possible it'd be better to merge both players now already, in my opinion.

Or minimize the size of that new player, in that state i'm afraid we can't accept it like that. My humble opinion again :)
Comment 6 Danilo de Paula 2012-10-24 10:09:07 PDT
(In reply to comment #5)
> We have buildbots reasonably testing the current player. If possible it'd be better to merge both players now already, in my opinion.
> 
> Or minimize the size of that new player, in that state i'm afraid we can't accept it like that. My humble opinion again :)

I quite agree with you, but I'm afraid that reduce the player might not be possible. Maybe the right way to go is to merge them now. I will work on it.

Also, my concern is about the CentralPipeLine unit. Is a global/static unit that controls/splits the gst elements thought the units who might use it. Might looks bloat to getUserMedia API, but it's a important component for the PeerConnection API. Is it ok to keep static/global units on webkit1?
Comment 7 Philippe Normand 2012-10-24 10:23:19 PDT
(In reply to comment #6)
> (In reply to comment #5)
> Also, my concern is about the CentralPipeLine unit. Is a global/static unit that controls/splits the gst elements thought the units who might use it. Might looks bloat to getUserMedia API, but it's a important component for the PeerConnection API. Is it ok to keep static/global units on webkit1?

Hum I'm not familiar enough with those APIs to answer that question properly I think... How is it implemented in chromium?
Comment 8 kov's GTK+ EWS bot 2012-10-24 11:31:20 PDT
Comment on attachment 170410 [details]
core patch

Attachment 170410 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14546306
Comment 9 Jonathon Jongsma (jonner) 2013-01-30 14:32:22 PST
Created attachment 185577 [details]
Patch
Comment 10 Philippe Normand 2013-01-30 23:48:14 PST
Comment on attachment 185577 [details]
Patch

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

Ok it looks good but I haven't made a full review because the patch is not based on ToT. Can you rebase it please? There were several changes recently including audio pitch preservation and native fullscreen support (with GStreamerGWorld). Having a rebased patch will also allow the EWS to chew this up :)

> Source/WebCore/ChangeLog:9
> +        [gtk] Refactor the media player implementation so more of the
> +        internal functionality can be shared between the current media
> +        backend and the mediastream player backend.  Common code is
> +        broken out into a MediaPlayerPrivateGStreamerBase, and both
> +        MediaPlayerPrivateGStreamer and
> +        StreamMediaPlayerPrivateGStreamer inherit from this base class.
> +        https://bugs.webkit.org/show_bug.cgi?id=100261

The change description is ok but doesn't comply with the ChangeLog format :) Usually it's:

bug title
bug url

Reviewed by ....

change description

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

You can remove this line or simply state existing media tests cover this patch.

> Source/WebCore/GNUmakefile.list.am:6065
> +	Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp \
> +	Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h \

The Qt and EFL build systems need similar changes.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:262
> +#if 0 // implementation from StreamMediaPlayer

Perhaps this block can be removed for now

> Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:28
> +typedef struct _WebKitVideoSink WebKitVideoSink;

You can remove this I think

> Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:35
> +class GStreamerGWorld;

Ditto
Comment 11 Jonathon Jongsma (jonner) 2013-02-04 13:11:40 PST
Created attachment 186444 [details]
Patch
Comment 12 WebKit Review Bot 2013-02-04 13:14:54 PST
Attachment 186444 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/Target.pri', u'Source/WebCore/platform/graphics/MediaPlayer.cpp', u'Source/WebCore/platform/graphics/gstreamer/FullscreenVideoControllerGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/FullscreenVideoControllerGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp', u'Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h', u'Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.cpp', u'Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h', u'Source/WebCore/platform/graphics/gtk/FullscreenVideoControllerGtk.cpp', u'Source/WebCore/platform/graphics/gtk/FullscreenVideoControllerGtk.h', u'configure.ac']" exit_code: 1
Source/WebCore/platform/graphics/gstreamer/FullscreenVideoControllerGStreamer.h:38:  The parameter name "player" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Build Bot 2013-02-04 18:13:02 PST
Comment on attachment 186444 [details]
Patch

Attachment 186444 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16375453
Comment 14 Build Bot 2013-02-04 19:36:48 PST
Comment on attachment 186444 [details]
Patch

Attachment 186444 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16375482
Comment 15 Philippe Normand 2013-02-05 06:45:41 PST
Comment on attachment 186444 [details]
Patch

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

Looks good overall, the Stream player seems quite incomplete though, I wonder if it'd make sense to have only the refactoring in this patch and keep a minimal-working stream player for a separate patch?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:156
> +            guint m_videoCapsHandler;

This doesn't seem used anywhere?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:283
> +    // FIXME: use implementation from StreamMediaPlayer??

I don't understand this FIXME, is it a left-over from a previous version of the patch?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:368
> +// creates and initializes a bunch of internal internal variables, and returns a

Double "internal" :) Also comments should be full sentences.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:110
> +    GRefPtr<GstStreamVolume> m_volume;

This name could be confusing, what about m_volumeElement? Also, OOC don't we need a GRefPtr specific implementation for GstStreamVolume, which is an interface, I'm not sure how it combines with GRefPtr in this patch.

> Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.cpp:126
> +        // go go go

Hum :)

> Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:89
> +    GstElement* m_videoSinkBin;
> +    GstElement* m_audioSinkBin;

Why not reuse the ones from the base class?
Comment 16 Jonathon Jongsma (jonner) 2013-02-05 08:42:49 PST
(In reply to comment #15)
> Looks good overall, the Stream player seems quite incomplete though, I wonder if it'd make sense to have only the refactoring in this patch and keep a minimal-working stream player for a separate patch?

Yeah, I was debating that as well.  I can do that if you'd like.

> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:156
> > +            guint m_videoCapsHandler;
> 
> This doesn't seem used anywhere?

Oops, left over from a different approach 

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:283
> > +    // FIXME: use implementation from StreamMediaPlayer??
> 
> I don't understand this FIXME, is it a left-over from a previous version of the patch?

yeah, I'll remove.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:110
> > +    GRefPtr<GstStreamVolume> m_volume;
> 
> This name could be confusing, what about m_volumeElement? Also, OOC don't we need a GRefPtr specific implementation for GstStreamVolume, which is an interface, I'm not sure how it combines with GRefPtr in this patch.

Well, since there's no template specialization for GstStreamVolume, it should just use the un-specialized implementation (GRefPtr<T>), which is the implementation for GObject (g_object_ref_sink/g_object_unref).  Since GstStreamVolume has 'GObject' as its only prerequisite, this is probably correct behavior, right?

> > Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.cpp:126
> > +        // go go go
> 
> Hum :)

Hmm, left over from the implementation.  if I remove the stream player from this patch, we can deal with that later :)

> > Source/WebCore/platform/graphics/gstreamer/StreamMediaPlayerPrivateGStreamer.h:89
> > +    GstElement* m_videoSinkBin;
> > +    GstElement* m_audioSinkBin;
> 
> Why not reuse the ones from the base class?

Oops, thanks for catching that.
Comment 17 Philippe Normand 2013-02-05 08:55:39 PST
(In reply to comment #16)
> (In reply to comment #15)
> > Looks good overall, the Stream player seems quite incomplete though, I wonder if it'd make sense to have only the refactoring in this patch and keep a minimal-working stream player for a separate patch?
> 
> Yeah, I was debating that as well.  I can do that if you'd like.
> 

Yes please!

Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:110
> > > +    GRefPtr<GstStreamVolume> m_volume;
> > 
> > This name could be confusing, what about m_volumeElement? Also, OOC don't we need a GRefPtr specific implementation for GstStreamVolume, which is an interface, I'm not sure how it combines with GRefPtr in this patch.
> 
> Well, since there's no template specialization for GstStreamVolume, it should just use the un-specialized implementation (GRefPtr<T>), which is the implementation for GObject (g_object_ref_sink/g_object_unref).  Since GstStreamVolume has 'GObject' as its only prerequisite, this is probably correct behavior, right?
> 

Ah right indeed. Thanks for clarifying :)
Comment 18 Jonathon Jongsma (jonner) 2013-02-05 13:23:44 PST
Created attachment 186691 [details]
Patch
Comment 19 Philippe Normand 2013-02-06 00:18:54 PST
Comment on attachment 186691 [details]
Patch

Ok it looks good but then the bug title doesn't make sense anymore since the patch is now a pure refactoring, can you retitle and update ChangeLogs accordingly before landing this?
Comment 20 Jonathon Jongsma (jonner) 2013-02-06 08:49:50 PST
Created attachment 186864 [details]
Patch
Comment 21 Philippe Normand 2013-02-06 09:06:28 PST
Comment on attachment 186864 [details]
Patch

No need for r+ I think since you've set the Reviewed by field already.
Comment 22 WebKit Review Bot 2013-02-06 10:17:07 PST
Comment on attachment 186864 [details]
Patch

Clearing flags on attachment: 186864

Committed r142005: <http://trac.webkit.org/changeset/142005>
Comment 23 WebKit Review Bot 2013-02-06 10:17:14 PST
All reviewed patches have been landed.  Closing bug.