Bug 124534 – [GStreamer] Remove 0.10 codepath

• Component: New Bugs
• Status: RESOLVED FIXED

[Xabier Rodríguez Calvar]

[GStreamer] Remove 0.10 codepath

[Xabier Rodríguez Calvar]

Created attachment 217247 [details]
Patch

Removed the GStreamer 0.10 path.

[EFL EWS Bot]

Comment on attachment 217247 [details]
Patch

Attachment 217247 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/28618017

[Gyuyoung Kim]

(In reply to comment #2)
> (From update of attachment 217247 [details])
> Attachment 217247 [details] did not pass efl-ews (efl):
> Output: http://webkit-queues.appspot.com/results/28618017

Please remove FullscreenVideoControllerEfl.cpp from Source/WebKit/PlatformEfl.cmake as well.

[Martin Robinson]

Comment on attachment 217247 [details]
Patch

This patch makes me happy.

[Philippe Normand]

Comment on attachment 217247 [details]
Patch

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

> Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:40
>  static const char* gDecodebinName = "decodebin";

This doesn't need to be a static global anymore

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:34
>      gst_object_ref_sink(gstObject);

For functions that are now a single line implementation I'm not sure it's worth to keep them, what do you think?

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:62
>      return adoptGRef(gst_pipeline_get_bus(pipeline));

Same question :)

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-982
> -        if (!priv->haveAppSrc27) {

That member can be removed too

[Philippe Normand]

Please also remove GST_API_VERSION_1 from the EFL cmake stuff

[Xabier Rodríguez Calvar]

(In reply to comment #5)
> (From update of attachment 217247 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217247&action=review
> 
> > Source/WebCore/platform/audio/gstreamer/AudioFileReaderGStreamer.cpp:40
> >  static const char* gDecodebinName = "decodebin";

Missed it.

> This doesn't need to be a static global anymore
> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:34
> >      gst_object_ref_sink(gstObject);
> 
> For functions that are now a single line implementation I'm not sure it's worth to keep them, what do you think?
> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:62
> >      return adoptGRef(gst_pipeline_get_bus(pipeline));
> 
> Same question :)

Actually I was thinking if the whole versioning still made sense, but I think there's still some stuff. I'll have a deeper look anyway and I agree on fixing these one liners.

> > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:-982
> > -        if (!priv->haveAppSrc27) {
> 
> That member can be removed too

Roger.

(In reply to comment #3)
> Please remove FullscreenVideoControllerEfl.cpp from Source/WebKit/PlatformEfl.cmake as well.

Roger.

(In reply to comment #6)
> Please also remove GST_API_VERSION_1 from the EFL cmake stuff

Roger.

[Xabier Rodríguez Calvar]

Created attachment 217632 [details]
Patch

Honored comments and removed versioning.

[Xabier Rodríguez Calvar]

Created attachment 217635 [details]
Patch

Honored comments and removed versioning. Fixed a typo in ver project filters.

[Philippe Normand]

Comment on attachment 217635 [details]
Patch

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

Looks nice! Just a small comment/question

> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h:74
> +void mapGstBuffer(GstBuffer*);
> +void unmapGstBuffer(GstBuffer*);
> +
>  namespace WebCore {
>  bool initializeGStreamer();
>  }

I wonder why the moved functions aren't in the WebCore namespace like the initializeGStreamer function.

[Xabier Rodríguez Calvar]

Comment on attachment 217635 [details]
Patch

(In reply to comment #10)
Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h:74
> > +void mapGstBuffer(GstBuffer*);
> > +void unmapGstBuffer(GstBuffer*);
> > +
> >  namespace WebCore {
> >  bool initializeGStreamer();
> >  }
> 
> I wonder why the moved functions aren't in the WebCore namespace like the initializeGStreamer function.

Cause they weren't before. Do you prefer them inside?

Btw, I spotted a little problem with the IntSize class, which is now included in the .h instead of forwarded. I'll upload a new patch.

[Xabier Rodríguez Calvar]

Created attachment 217658 [details]
Patch

Moved IntSize.h include and forwarded declaration.

[Philippe Normand]

(In reply to comment #11)
> (From update of attachment 217635 [details])
> (In reply to comment #10)
> Source/WebCore/platform/graphics/gstreamer/GStreamerUtilities.h:74
> > > +void mapGstBuffer(GstBuffer*);
> > > +void unmapGstBuffer(GstBuffer*);
> > > +
> > >  namespace WebCore {
> > >  bool initializeGStreamer();
> > >  }
> > 
> > I wonder why the moved functions aren't in the WebCore namespace like the initializeGStreamer function.
> 
> Cause they weren't before. Do you prefer them inside?

Yes please it would be coherent.

[Xabier Rodríguez Calvar]

Created attachment 217692 [details]
Patch

Added GStreamer utility functions to WebCore namespace

[WebKit Commit Bot]

Comment on attachment 217692 [details]
Patch

Clearing flags on attachment: 217692

Committed r159730: <http://trac.webkit.org/changeset/159730>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #15)
> (From update of attachment 217692 [details])
> Clearing flags on attachment: 217692
> 
> Committed r159730: <http://trac.webkit.org/changeset/159730>

It broke the WinCairo build:

8>c:\projects\buildslave\win-cairo-release\build\source\webcore\platform\graphics\gstreamer\GStreamerUtilities.h(21): fatal error C1083: Cannot open include file: 'gst/gst.h': No such file or directory

Brent, any idea?

[Xabier Rodríguez Calvar]

Reopening to attach new patch.

[Xabier Rodríguez Calvar]

Created attachment 217813 [details]
patch for wrong bug

Rebased

[Xabier Rodríguez Calvar]

(In reply to comment #19)
> Rebased

Ooops, patch for wrong bug O:)