Bug 135766 – [WinCairo] Make it easier to enable/disable GStreamer.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

Since the Media Foundation implementation doesn't appear to be completed, could we enable GStreamer?

Created attachment 236306 [details]
Patch

[Alex Christensen]

Comment on attachment 236306 [details]
Patch

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

I would r+ most of this, except Platform.h.  I left the GStreamer property sheets in to not remove the future possibility of using GStreamer.

> Source/WTF/wtf/Platform.h:861
> +#define WTF_USE_GLIB 1

I think people want glib to not be a dependency.  See the discussion after https://lists.webkit.org/pipermail/webkit-dev/2014-April/026478.html
I'm ok with putting the rest of these changes in to make your work easier, but I do not want to switch the default build to GStreamer unless it works with seeking and loading correctly.  
https://bugs.webkit.org/show_bug.cgi?id=127376 (which I'm not working on any more)

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:134
> +    <Import Project="..\..\..\WebKitLibraries\win\tools\vsprops\GStreamer32.props" />

Let's import GStreamerCommon.props here, too.  It's not needed, but we might want to do global changes later without wondering why WebCore is not changing.

Created attachment 236332 [details]
Patch

(In reply to comment #2)
> (From update of attachment 236306 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236306&action=review
> 

Thanks for reviewing :)

> I would r+ most of this, except Platform.h.  I left the GStreamer property sheets in to not remove the future possibility of using GStreamer.
> 
> > Source/WTF/wtf/Platform.h:861
> > +#define WTF_USE_GLIB 1
> 

This seems to be needed, I get compile errors without it.

> I think people want glib to not be a dependency.  See the discussion after https://lists.webkit.org/pipermail/webkit-dev/2014-April/026478.html
> I'm ok with putting the rest of these changes in to make your work easier, but I do not want to switch the default build to GStreamer unless it works with seeking and loading correctly.  
> https://bugs.webkit.org/show_bug.cgi?id=127376 (which I'm not working on any more)

I believe this was caused by a bug in the Curl disc cache implementation, which I think is fixed now.

I'll have a look at the Media Foundation implementation soon.

[Philippe Normand]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 236306 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=236306&action=review
> > 
> 
> Thanks for reviewing :)
> 
> > I would r+ most of this, except Platform.h.  I left the GStreamer property sheets in to not remove the future possibility of using GStreamer.
> > 
> > > Source/WTF/wtf/Platform.h:861
> > > +#define WTF_USE_GLIB 1
> > 
> 
> This seems to be needed, I get compile errors without it.
> 

What compile errors?

[Alex Christensen]

Comment on attachment 236332 [details]
Patch

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

This looks good except changing the build to require GStreamer and Glib.  I'll try playing with GStreamer on Windows again soon.

> Source/WTF/wtf/Platform.h:859
> +#define WTF_USE_MEDIA_FOUNDATION 0

This is the real issue: I would like to not change the default WinCairo build to use GStreamer.  I'm in favor of all these changes except this line.

> Source/WTF/wtf/Platform.h:860
> +#if !WTF_USE_MEDIA_FOUNDATION

#if USE(MEDIA_FOUNDATION)

> Source/WTF/wtf/Platform.h:861
> +#define WTF_USE_GLIB 1

GLIB is indeed needed with GStreamer otherwise there are compile errors because things are missing.  This is good.

> Websites/webkit.org/building/tools.html:65
> +<ul><li><a href="http://gstreamer.freedesktop.org/data/pkg/windows/1.2.1/gstreamer-1.0-devel-x86-1.2.1.msi">gstreamer-1.0-devel-x86-1.2.1.msi</a></li>

Philippe, does the GTK port still use GStreamer 1.2.1?

[Philippe Normand]

Yes the bots use GStreamer 1.2.1 still but now that 1.4.0 was released I think we should bump our requirement. 1.4.x is the new stable branch now, I'd advise you to try it :)

Created attachment 237952 [details]
Patch

This patch is no longer about enabling GStreamer, but to make it easier to enable/disable GStreamer.
This can be done by editing a new user macro called ENABLE_GSTREAMER_WINCAIRO.
I put it in GStreamerCommon.props, but maybe it belongs in FeatureDefinesCairo.props?

[Philippe Normand]

Comment on attachment 237952 [details]
Patch

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

> Source/WTF/wtf/Platform.h:879
> +#define GST_API_VERSION_1 1

This define is not needed. It was introduced when when we had to support both 0.10 and 1.0 gst APIs but now that 0.10 support was removed there's no need for such macro.

Created attachment 237956 [details]
Patch

(In reply to comment #11)
> (From update of attachment 237952 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237952&action=review
> 
> > Source/WTF/wtf/Platform.h:879
> > +#define GST_API_VERSION_1 1
> 
> This define is not needed. It was introduced when when we had to support both 0.10 and 1.0 gst APIs but now that 0.10 support was removed there's no need for such macro.

Thanks :) Updated patch.

[Philippe Normand]

Comment on attachment 237956 [details]
Patch

LGTM but I'll let Brent do a real review :)

[Brent Fulgham]

Comment on attachment 237956 [details]
Patch

This looks fine, although I really was hoping to standardize on the built-in Windows media stack. r=me.

(In reply to comment #15)
> (From update of attachment 237956 [details])
> This looks fine, although I really was hoping to standardize on the built-in Windows media stack. r=me.

Thanks for reviewing :) I believe the next step will be to complete the Media Foundation implementation.

[WebKit Commit Bot]

Comment on attachment 237956 [details]
Patch

Clearing flags on attachment: 237956

Committed r173616: <http://trac.webkit.org/changeset/173616>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.