Bug 163000 - [GTK] Build fix for X11 and GStreamerGL after r183731
Summary: [GTK] Build fix for X11 and GStreamerGL after r183731
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gwang Yoon Hwang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-06 00:13 PDT by Gwang Yoon Hwang
Modified: 2016-10-06 03:47 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.72 KB, patch)
2016-10-06 00:14 PDT, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff
Patch (1.37 KB, patch)
2016-10-06 03:17 PDT, Gwang Yoon Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gwang Yoon Hwang 2016-10-06 00:13:17 PDT
[GTK] Build fix for X11 and GStreamerGL after r183731
Comment 1 Gwang Yoon Hwang 2016-10-06 00:14:15 PDT
Created attachment 290788 [details]
Patch
Comment 2 Carlos Garcia Campos 2016-10-06 00:58:11 PDT
Comment on attachment 290788 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:267
> -#endif
> -
> -#if PLATFORM(WAYLAND)
> +#elif PLATFORM(WAYLAND)

What's exactly the build failure? I don't think this change is correct, X11 and Wayland are not mutually exclusive, so the ifdefs are correct here. Why is the display created twice? if both X11 and wayland are enabled, shared display will be either X11 or wayland.
Comment 3 Gwang Yoon Hwang 2016-10-06 01:08:04 PDT
(In reply to comment #2)
> Comment on attachment 290788 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290788&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:267
> > -#endif
> > -
> > -#if PLATFORM(WAYLAND)
> > +#elif PLATFORM(WAYLAND)
> 
> What's exactly the build failure? I don't think this change is correct, X11
> and Wayland are not mutually exclusive, so the ifdefs are correct here. Why
> is the display created twice? if both X11 and wayland are enabled, shared
> display will be either X11 or wayland.

As you mentioned, PlatformDisplay is only created just for once.
And I misunderstood that gst_gl_display_x11_new_with_display and gst_gl_display_egl_new_with_egl_display would be executed at same time.

The build problem is, MediaPlayerPrivateGStreamerBase does not include gstgldisplay_egl.h if we are using GLX. So gst_gl_display_egl_new_with_egl_display is not visible from GSteamerBase.

I'll take a look if there is no problem to include both of gstgldisplay_x11 and gstgldisplay_egl
Comment 4 Gwang Yoon Hwang 2016-10-06 01:27:16 PDT
(In reply to comment #3)
> (In reply to comment #2)
> As you mentioned, PlatformDisplay is only created just for once.
> And I misunderstood that gst_gl_display_x11_new_with_display and
> gst_gl_display_egl_new_with_egl_display would be executed at same time.
> 
> The build problem is, MediaPlayerPrivateGStreamerBase does not include
> gstgldisplay_egl.h if we are using GLX. So
> gst_gl_display_egl_new_with_egl_display is not visible from GSteamerBase.
> 
> I'll take a look if there is no problem to include both of gstgldisplay_x11
> and gstgldisplay_egl

There could be a build problem if the platform has only X11 or EGL.
Since gstgldisplay_x11.h includes headers of X11 and gstgldisplay_egl.h includes headers of EGL, we cannot include both of them at same time if the target does not have both.
Comment 5 Philippe Normand 2016-10-06 01:28:35 PDT
Comment on attachment 290788 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:267
>>> +#elif PLATFORM(WAYLAND)
>> 
>> What's exactly the build failure? I don't think this change is correct, X11 and Wayland are not mutually exclusive, so the ifdefs are correct here. Why is the display created twice? if both X11 and wayland are enabled, shared display will be either X11 or wayland.
> 
> As you mentioned, PlatformDisplay is only created just for once.
> And I misunderstood that gst_gl_display_x11_new_with_display and gst_gl_display_egl_new_with_egl_display would be executed at same time.
> 
> The build problem is, MediaPlayerPrivateGStreamerBase does not include gstgldisplay_egl.h if we are using GLX. So gst_gl_display_egl_new_with_egl_display is not visible from GSteamerBase.
> 
> I'll take a look if there is no problem to include both of gstgldisplay_x11 and gstgldisplay_egl

This is wrong because we can have both X11 and WAYLAND enabled. IIUC anyway :)
Comment 6 Gwang Yoon Hwang 2016-10-06 03:17:46 PDT
Created attachment 290807 [details]
Patch
Comment 7 Gwang Yoon Hwang 2016-10-06 03:18:58 PDT
(In reply to comment #6)
> Created attachment 290807 [details]
> Patch

Found much simpler solution. Just include gstdisplay_egl.h if we have EGL.
Ready for review.
Comment 8 WebKit Commit Bot 2016-10-06 03:47:01 PDT
Comment on attachment 290807 [details]
Patch

Clearing flags on attachment: 290807

Committed r206861: <http://trac.webkit.org/changeset/206861>
Comment 9 WebKit Commit Bot 2016-10-06 03:47:06 PDT
All reviewed patches have been landed.  Closing bug.