Bug 124534 - [GStreamer] Remove 0.10 codepath
Summary: [GStreamer] Remove 0.10 codepath
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xabier Rodríguez Calvar
URL:
Keywords:
Depends on:
Blocks: 124654
  Show dependency treegraph
 
Reported: 2013-11-18 13:01 PST by Xabier Rodríguez Calvar
Modified: 2013-11-25 09:25 PST (History)
20 users (show)

See Also:


Attachments
Patch (77.77 KB, patch)
2013-11-18 16:30 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (111.57 KB, patch)
2013-11-21 17:11 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (111.33 KB, patch)
2013-11-21 17:32 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (111.26 KB, patch)
2013-11-22 00:59 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (113.47 KB, patch)
2013-11-22 09:18 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
patch for wrong bug (1.72 KB, patch)
2013-11-25 09:20 PST, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xabier Rodríguez Calvar 2013-11-18 13:01:44 PST
[GStreamer] Remove 0.10 codepath
Comment 1 Xabier Rodríguez Calvar 2013-11-18 16:30:16 PST
Created attachment 217247 [details]
Patch

Removed the GStreamer 0.10 path.
Comment 2 EFL EWS Bot 2013-11-18 16:37:14 PST
Comment on attachment 217247 [details]
Patch

Attachment 217247 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/28618017
Comment 3 Gyuyoung Kim 2013-11-18 17:02:34 PST
(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.
Comment 4 Martin Robinson 2013-11-18 17:18:23 PST
Comment on attachment 217247 [details]
Patch

This patch makes me happy.
Comment 5 Philippe Normand 2013-11-18 23:15:15 PST
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
Comment 6 Philippe Normand 2013-11-19 08:01:47 PST
Please also remove GST_API_VERSION_1 from the EFL cmake stuff
Comment 7 Xabier Rodríguez Calvar 2013-11-19 08:18:24 PST
(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.
Comment 8 Xabier Rodríguez Calvar 2013-11-21 17:11:49 PST
Created attachment 217632 [details]
Patch

Honored comments and removed versioning.
Comment 9 Xabier Rodríguez Calvar 2013-11-21 17:32:13 PST
Created attachment 217635 [details]
Patch

Honored comments and removed versioning. Fixed a typo in ver project filters.
Comment 10 Philippe Normand 2013-11-21 23:40:39 PST
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.
Comment 11 Xabier Rodríguez Calvar 2013-11-22 00:50:34 PST
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.
Comment 12 Xabier Rodríguez Calvar 2013-11-22 00:59:34 PST
Created attachment 217658 [details]
Patch

Moved IntSize.h include and forwarded declaration.
Comment 13 Philippe Normand 2013-11-22 02:08:54 PST
(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.
Comment 14 Xabier Rodríguez Calvar 2013-11-22 09:18:34 PST
Created attachment 217692 [details]
Patch

Added GStreamer utility functions to WebCore namespace
Comment 15 WebKit Commit Bot 2013-11-23 06:33:01 PST
Comment on attachment 217692 [details]
Patch

Clearing flags on attachment: 217692

Committed r159730: <http://trac.webkit.org/changeset/159730>
Comment 16 WebKit Commit Bot 2013-11-23 06:33:05 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Csaba Osztrogonác 2013-11-25 09:05:07 PST
(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?
Comment 18 Xabier Rodríguez Calvar 2013-11-25 09:20:46 PST
Reopening to attach new patch.
Comment 19 Xabier Rodríguez Calvar 2013-11-25 09:20:51 PST
Created attachment 217813 [details]
patch for wrong bug

Rebased
Comment 20 Xabier Rodríguez Calvar 2013-11-25 09:23:52 PST
(In reply to comment #19)
> Rebased

Ooops, patch for wrong bug O:)