Bug 44508 – [EFL] Add PlatformVideoWindowEfl.cpp for WebKit EFL

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

It seems to me that WebKit EFL also needs to have PlatformVideoWindowEfl.cpp in order to use gstreamer as it's video framework.

[Gyuyoung Kim]

Created attachment 65250 [details]
Patch

I modify PlatformVideoWindow.h and create PlatformVideoWindowEfl.cpp.

[Philippe Normand]

This is needed only for fullscreen video display. You should already be able to use the gst webkitvideo sink. For painting Cairo is used, see ImageGStreamerCairo.cpp ... For EFL you might need a new ImageGStreamer implementation, dunno.

[Philippe Normand]

Comment on attachment 65250 [details]
Patch


>+#if PLATFORM(EFL)
>+#if ENABLE(GLIB_SUPPORT)
>+#include <glib.h>
>+#endif
>+#endif
>+

I think the guards can be removed because glib.h is required for the use of gulong below anyway.
The only reason why it works currently is that gtk.h includes glib.h

> namespace WebCore {
> 
> class PlatformVideoWindow : public RefCounted<PlatformVideoWindow> {
>@@ -39,12 +45,16 @@ class PlatformVideoWindow : public RefCounted<PlatformVideoWindow> {
>         PlatformVideoWindow();
>         ~PlatformVideoWindow();
> 
>+#if PLATFORM(GTK)
>         PlatformWindowType* window() const { return m_window; }
>+#endif

>         gulong videoWindowId() const { return m_videoWindowId; }
> 
>     private:
>         gulong m_videoWindowId;
>+#if PLATFORM(GTK)
>         PlatformWindowType* m_videoWindow;
>+#endif

This is wrong. You need to add a typedef for your platform so PlatformWindowType is valid.

[Gyuyoung Kim]

(In reply to comment #3)
> (From update of attachment 65250 [details])
> 
> >+#if PLATFORM(EFL)
> >+#if ENABLE(GLIB_SUPPORT)
> >+#include <glib.h>
> >+#endif
> >+#endif
> >+
> 
> I think the guards can be removed because glib.h is required for the use of gulong below anyway.
> The only reason why it works currently is that gtk.h includes glib.h
> 
> > namespace WebCore {
> > 
> > class PlatformVideoWindow : public RefCounted<PlatformVideoWindow> {
> >@@ -39,12 +45,16 @@ class PlatformVideoWindow : public RefCounted<PlatformVideoWindow> {
> >         PlatformVideoWindow();
> >         ~PlatformVideoWindow();
> > 
> >+#if PLATFORM(GTK)
> >         PlatformWindowType* window() const { return m_window; }
> >+#endif
> 
> >         gulong videoWindowId() const { return m_videoWindowId; }
> > 
> >     private:
> >         gulong m_videoWindowId;
> >+#if PLATFORM(GTK)
> >         PlatformWindowType* m_videoWindow;
> >+#endif
> 
> This is wrong. You need to add a typedef for your platform so PlatformWindowType is valid.

I agree with your opinion. I am going to consider your guidance.

Thank you.

[Philippe Normand]

BTW fullscreen video display shouldn't be blocker for an initial HTML5 video support in WebKit EFL.

You just need to avoid painting the fullscreen button in your media controls and make the webkitSupportsFullscreen method of the gst player return false in your case.

[Gyuyoung Kim]

(In reply to comment #3)
> (From update of attachment 65250 [details])
> 
> >+#if PLATFORM(EFL)
> >+#if ENABLE(GLIB_SUPPORT)
> >+#include <glib.h>
> >+#endif
> >+#endif
> >+
> 
> I think the guards can be removed because glib.h is required for the use of gulong below anyway.
> The only reason why it works currently is that gtk.h includes glib.h

WebKit EFL doesn't use gtk. So, I include glib.h for EFL. Is this wrong ?

[Philippe Normand]

(In reply to comment #6)
> (In reply to comment #3)
> > (From update of attachment 65250 [details] [details])
> > 
> > >+#if PLATFORM(EFL)
> > >+#if ENABLE(GLIB_SUPPORT)
> > >+#include <glib.h>
> > >+#endif
> > >+#endif
> > >+
> > 
> > I think the guards can be removed because glib.h is required for the use of gulong below anyway.
> > The only reason why it works currently is that gtk.h includes glib.h
> 
> WebKit EFL doesn't use gtk. So, I include glib.h for EFL. Is this wrong ?

It is totally right, I was just asking to remove the #ifs

[Gyuyoung Kim]

Created attachment 65381 [details]
Patch

I define PlatformWindowType for EFL.

Thank you.

[Philippe Normand]

Comment on attachment 65381 [details]
Patch

>--- a/WebCore/platform/graphics/gstreamer/PlatformVideoWindow.h
>+++ b/WebCore/platform/graphics/gstreamer/PlatformVideoWindow.h
>@@ -28,6 +28,15 @@
> #if PLATFORM(GTK)
> #include <gtk/gtk.h>
> typedef GtkWidget PlatformWindowType;
>+#elif PLATFORM(EFL)
>+#include "Widget.h"
>+typedef PlatformWidget PlatformWindowType;
>+#endif
>+

Oh nice I didn't know about Widget.h
So we duplicated efforts here. I think it'd make sense to remove PlatformWindowType and use PlatformWidget directly.

>+#if PLATFORM(EFL)
>+#if ENABLE(GLIB_SUPPORT)
>+#include <glib.h>
>+#endif
> #endif
> 

I said in previous comment that the #ifs are not necessary.

The rest of the patch looks ok to me!

[Gyuyoung Kim]

(In reply to comment #9)
> (From update of attachment 65381 [details])
> >--- a/WebCore/platform/graphics/gstreamer/PlatformVideoWindow.h
> >+++ b/WebCore/platform/graphics/gstreamer/PlatformVideoWindow.h
> >@@ -28,6 +28,15 @@
> > #if PLATFORM(GTK)
> > #include <gtk/gtk.h>
> > typedef GtkWidget PlatformWindowType;
> >+#elif PLATFORM(EFL)
> >+#include "Widget.h"
> >+typedef PlatformWidget PlatformWindowType;
> >+#endif
> >+
> 
> Oh nice I didn't know about Widget.h
> So we duplicated efforts here. I think it'd make sense to remove PlatformWindowType and use PlatformWidget directly.
> 

I wonder how to use PlatformWidget directly. In order to use the PlatformformWidget directly, I think we need to modify code as below,

 48 #if PLATFORM(GTK)
 49         PlatformWindowType* window() const { return m_window; }
 50 #elif PLATFORM(EFL)
 51         PlatformWidget* window() const { return m_window; }
 52 #endif


> >+#if PLATFORM(EFL)
> >+#if ENABLE(GLIB_SUPPORT)
> >+#include <glib.h>
> >+#endif
> > #endif
> > 
> 
> I said in previous comment that the #ifs are not necessary.
> 
> The rest of the patch looks ok to me!

I modify above code as below,

 31 #elif PLATFORM(EFL)
 32 #if ENABLE(GLIB_SUPPORT)
 33 #include <glib.h>
 34 #endif
 35 #include "Widget.h"
 36 typedef PlatformWidget PlatformWindowType;
 37 #endif

Is this ok ?

[Philippe Normand]

(In reply to comment #10)

> I wonder how to use PlatformWidget directly. In order to use the PlatformformWidget directly, I think we need to modify code as below,
> 
>  48 #if PLATFORM(GTK)
>  49         PlatformWindowType* window() const { return m_window; }
>  50 #elif PLATFORM(EFL)
>  51         PlatformWidget* window() const { return m_window; }
>  52 #endif
> 
> 

No, it would be best to make the GTK port use PlatformWidget as well, it will all be more coherent and readable.

> I modify above code as below,
> 
>  31 #elif PLATFORM(EFL)
>  32 #if ENABLE(GLIB_SUPPORT)
>  33 #include <glib.h>
>  34 #endif
>  35 #include "Widget.h"
>  36 typedef PlatformWidget PlatformWindowType;
>  37 #endif
> 
> Is this ok ?

No. I thought I was clear, but it seems not :)
glib.h can safely be included *outside* any #ifs like this:

#include <glib.h>

If you make use of PlatformWidget no #if at all should be needed in this file.

[Philippe Normand]

(In reply to comment #11)
> 
> > I modify above code as below,
> > 
> >  31 #elif PLATFORM(EFL)
> >  32 #if ENABLE(GLIB_SUPPORT)
> >  33 #include <glib.h>
> >  34 #endif
> >  35 #include "Widget.h"
> >  36 typedef PlatformWidget PlatformWindowType;
> >  37 #endif
> > 
> > Is this ok ?
> 

Well I understand your port doesn't have a hard dependency on GLib, so yeah you need guards, maybe like:

#if PLATFORM(GTK) || ENABLE(GLIB_SUPPORT)
#include <glib.h>
#endif

?

[Philippe Normand]

Or even:

#if !PLATFORM(EFL) || ENABLE(GLIB_SUPPORT)
#include <glib.h>
#endif

[Gyuyoung Kim]

Yes, WebKit EFL doesn't have hard dependency with glib. WebKit EFL should enable ENABLE_GLIB_SUPPORT macro to use glib. If you are ok, I will upload new patch again.

+#include "Widget.h"
+
 #if PLATFORM(GTK)
 #include <gtk/gtk.h>
 typedef GtkWidget PlatformWindowType;
+#elif PLATFORM(EFL)
+#include "Widget.h"
+typedef PlatformWidget PlatformWindowType;
+#endif
+
+#if !PLATFORM(EFL) || ENABLE(GLIB_SUPPORT)
+#include <glib.h>
 #endif

[Gyuyoung Kim]

(In reply to comment #14)
> Yes, WebKit EFL doesn't have hard dependency with glib. WebKit EFL should enable ENABLE_GLIB_SUPPORT macro to use glib. If you are ok, I will upload new patch again.
> 
> +#include "Widget.h"
> +
>  #if PLATFORM(GTK)
>  #include <gtk/gtk.h>
>  typedef GtkWidget PlatformWindowType;
> +#elif PLATFORM(EFL)
> +#include "Widget.h"
> +typedef PlatformWidget PlatformWindowType;
> +#endif
> +
> +#if !PLATFORM(EFL) || ENABLE(GLIB_SUPPORT)
> +#include <glib.h>
>  #endif

Sorry, please remove "#include "Widget.h"

> +#include "Widget.h"
> +

[Philippe Normand]

(In reply to comment #15)
> >  #if PLATFORM(GTK)
> >  #include <gtk/gtk.h>
> >  typedef GtkWidget PlatformWindowType;
> > +#elif PLATFORM(EFL)
> > +#include "Widget.h"
> > +typedef PlatformWidget PlatformWindowType;
> > +#endif
> > +
> > +#if !PLATFORM(EFL) || ENABLE(GLIB_SUPPORT)
> > +#include <glib.h>
> >  #endif
> 

Ok much better, what about PlatformWidget use for the GTK port too? I would be willing to test such patch :)

Also PlatformWidget is a pointer already. So you need to update the declarations of ::window(), m_videoWindow and m_window accordingly.

[Gyuyoung Kim]

Created attachment 65388 [details]
Patch

I modify this patch according to your guidance. 

I built WebKit GTK based on this patch as well. There was no build breaks.

[Philippe Normand]

Comment on attachment 65388 [details]
Patch

Nice, thanks :)
One last thing (from my side, at least):

> #if PLATFORM(GTK)
> #include <gtk/gtk.h>
>-typedef GtkWidget PlatformWindowType;
>+#endif

That #if can be removed all together. We no longer require to include gtk.h there but then we should move it to PlatformVideoWindowGtk.cpp I think. But as this patch is EFL-specific I can make that change myself in a separate patch.

[Gyuyoung Kim]

(In reply to comment #18)
> (From update of attachment 65388 [details])
> Nice, thanks :)
> One last thing (from my side, at least):
> 
> > #if PLATFORM(GTK)
> > #include <gtk/gtk.h>
> >-typedef GtkWidget PlatformWindowType;
> >+#endif
> 
> That #if can be removed all together. We no longer require to include gtk.h there but then we should move it to PlatformVideoWindowGtk.cpp I think. But as this patch is EFL-specific I can make that change myself in a separate patch.

I couldn't remove "#include <gtk/gtk.h>" because of build error in PlatformVideoWindowGtk.cpp. If you want to move "#include <gtk/gtk.h>" to PlatformVideoWindowGtk.cpp, I can do it new patch. Do you want ?

[Philippe Normand]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 65388 [details] [details])
> > Nice, thanks :)
> > One last thing (from my side, at least):
> > 
> > > #if PLATFORM(GTK)
> > > #include <gtk/gtk.h>
> > >-typedef GtkWidget PlatformWindowType;
> > >+#endif
> > 
> > That #if can be removed all together. We no longer require to include gtk.h there but then we should move it to PlatformVideoWindowGtk.cpp I think. But as this patch is EFL-specific I can make that change myself in a separate patch.
> 
> I couldn't remove "#include <gtk/gtk.h>" because of build error in PlatformVideoWindowGtk.cpp. If you want to move "#include <gtk/gtk.h>" to PlatformVideoWindowGtk.cpp, I can do it new patch. Do you want ?

In comment 19 I was thinking about yes. Sure, if you want file a new bug and patch after this one lands :)

[Gyuyoung Kim]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (From update of attachment 65388 [details] [details] [details])
> > > Nice, thanks :)
> > > One last thing (from my side, at least):
> > > 
> > > > #if PLATFORM(GTK)
> > > > #include <gtk/gtk.h>
> > > >-typedef GtkWidget PlatformWindowType;
> > > >+#endif
> > > 
> > > That #if can be removed all together. We no longer require to include gtk.h there but then we should move it to PlatformVideoWindowGtk.cpp I think. But as this patch is EFL-specific I can make that change myself in a separate patch.
> > 
> > I couldn't remove "#include <gtk/gtk.h>" because of build error in PlatformVideoWindowGtk.cpp. If you want to move "#include <gtk/gtk.h>" to PlatformVideoWindowGtk.cpp, I can do it new patch. Do you want ?
> 
> In comment 19 I was thinking about yes. Sure, if you want file a new bug and patch after this one lands :)

Sure, I will do that after landing this patch. :) 

Thanks.

[Rafael Antognolli]

(In reply to comment #12)
> (In reply to comment #11)
> > 
> > > I modify above code as below,
> > > 
> > >  31 #elif PLATFORM(EFL)
> > >  32 #if ENABLE(GLIB_SUPPORT)
> > >  33 #include <glib.h>
> > >  34 #endif
> > >  35 #include "Widget.h"
> > >  36 typedef PlatformWidget PlatformWindowType;
> > >  37 #endif
> > > 
> > > Is this ok ?
> > 
> 
> Well I understand your port doesn't have a hard dependency on GLib, so yeah you need guards, maybe like:
> 
> #if PLATFORM(GTK) || ENABLE(GLIB_SUPPORT)
> #include <glib.h>
> #endif
> 
> ?

Actually, no guards are needed since Gyuyoung's patch is for gstreamer video backend, and gstreamer needs glib anyway. We even force ENABLE_GLIB_SUPPORT if ENABLE_VIDEO_SUPPORT is true.

Maybe if later another video implementation that doesn't use gstreamer is available, the yes, we could make this optional. And I think the following line (unless changed to something else with ifdefs too) wouldn't compile without #include <glib.h>:
         gulong videoWindowId() const { return m_videoWindowId; }

[Gyuyoung Kim]

Hello  Philippe Normand,

Who can review this patch ?

[Philippe Normand]

(In reply to comment #23)
> Hello  Philippe Normand,
> 
> Who can review this patch ?

Well Rafael is right about including glib.h without guards.

Then either Xan, Martin or Gustavo could review it maybe

[Martin Robinson]

Comment on attachment 65388 [details]
Patch


> +#include "Widget.h"
> +
>  #include <wtf/PassRefPtr.h>

Please remove this extra line. There is no need to separate the include styles.

>  #if PLATFORM(GTK)
>  #include <gtk/gtk.h>
> -typedef GtkWidget PlatformWindowType;
> +#endif
> +
> +#if !PLATFORM(EFL) || ENABLE(GLIB_SUPPORT)
> +#include <glib.h>
>  #endif

Is the only reason for these #ifdefs and #includes gulong? Having
GLib and/or GTK+ includes in a header files can increase the compilation
time unecessarily. If possible, please just use unsigned long instead of
gulong and remove all the #ifdefs and #includes. Thanks!
  
> diff --git a/WebCore/platform/graphics/gstreamer/PlatformVideoWindowEfl.cpp b/WebCore/platform/graphics/gstreamer/PlatformVideoWindowEfl.cpp

Should this also be added to the CMake build scripts?

[Gyuyoung Kim]

Created attachment 65682 [details]
Patch

Thanks, I fix this patch according to your comment.
I move the "#include <gtk/gtk.h>" to PlatformVideoWindowGtk.cpp.

BTW, I add the PlatformVideoWindowEfl.cpp to CMakeListEfl.txt in Bug 44098.
(Bug 44098 is blocked by this bug.)
Because, I want to add this file together with other files regarding gstreamer. So, the PlatformVideoWindowEfl.cpp will be added after landing this patch.

[Gyuyoung Kim]

Hello Martin Robinson,

Please review again. :)

[Gyuyoung Kim]

Hello Martin Robinson,

I am looking forward to land this patch in mainline.
I'd like to get your opinion regarding your patch.

[Gyuyoung Kim]

wrong expression : "your patch" => "my patch"

[Martin Robinson]

Comment on attachment 65682 [details]
Patch

LGTM.

[WebKit Commit Bot]

Comment on attachment 65682 [details]
Patch

Clearing flags on attachment: 65682

Committed r66578: <http://trac.webkit.org/changeset/66578>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.