It seems to me that WebKit EFL also needs to have PlatformVideoWindowEfl.cpp in order to use gstreamer as it's video framework.
Created attachment 65250 [details] Patch I modify PlatformVideoWindow.h and create PlatformVideoWindowEfl.cpp.
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.
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.
(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.
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.
(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 ?
(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
Created attachment 65381 [details] Patch I define PlatformWindowType for EFL. Thank you.
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!
(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 ?
(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.
(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 ?
Or even: #if !PLATFORM(EFL) || ENABLE(GLIB_SUPPORT) #include <glib.h> #endif
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
(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" > +
(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.
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.
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.
(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 ?
(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 :)
(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.
(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; }
Hello Philippe Normand, Who can review this patch ?
(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
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?
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.
Hello Martin Robinson, Please review again. :)
Hello Martin Robinson, I am looking forward to land this patch in mainline. I'd like to get your opinion regarding your patch.
wrong expression : "your patch" => "my patch"
Comment on attachment 65682 [details] Patch LGTM.
Comment on attachment 65682 [details] Patch Clearing flags on attachment: 65682 Committed r66578: <http://trac.webkit.org/changeset/66578>
All reviewed patches have been landed. Closing bug.