Bug 100261 – [GStreamer] MediaPlayer's code is not easily reusable by other GStreamer-based players

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Danilo de Paula]

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.

[Danilo de Paula]

Created attachment 170410 [details]
core patch

[WebKit Review Bot]

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.

[Philippe Normand]

Do you plan to factor the copy-pasted code from the mediaplayer to some common module?

[Danilo de Paula]

(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?

[Philippe Normand]

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 :)

[Danilo de Paula]

(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?

[Philippe Normand]

(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?

[kov's GTK+ EWS bot]

Comment on attachment 170410 [details]
core patch

Attachment 170410 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14546306

[Jonathon Jongsma (jonner)]

Created attachment 185577 [details]
Patch

[Philippe Normand]

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

[Jonathon Jongsma (jonner)]

Created attachment 186444 [details]
Patch

[WebKit Review Bot]

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.

[Build Bot]

Comment on attachment 186444 [details]
Patch

Attachment 186444 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16375453

[Build Bot]

Comment on attachment 186444 [details]
Patch

Attachment 186444 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16375482

[Philippe Normand]

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?

[Jonathon Jongsma (jonner)]

(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.

[Philippe Normand]

(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 :)

[Jonathon Jongsma (jonner)]

Created attachment 186691 [details]
Patch

[Philippe Normand]

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?

[Jonathon Jongsma (jonner)]

Created attachment 186864 [details]
Patch

[Philippe Normand]

Comment on attachment 186864 [details]
Patch

No need for r+ I think since you've set the Reviewed by field already.

[WebKit Review Bot]

Comment on attachment 186864 [details]
Patch

Clearing flags on attachment: 186864

Committed r142005: <http://trac.webkit.org/changeset/142005>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.