[Webkit-unassigned] [Bug 39474] [GStreamer] GTK XOverlay support in GStreamerGWorld

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39474





--- Comment #26 from Philippe Normand <pnormand at igalia.com>  2010-07-20 00:32:31 PST ---
(In reply to comment #25)
> (From update of attachment 61777 [details])
>  286     gtk_widget_show_all(m_window);
>  287     gtk_widget_realize(m_window);
> 
> No point in showing, then realizing. Showing triggers realizing. You seem to only need realize at this point, but it seems you want it shown anyway, so just show it.

I just need it realized to access the window, just below. Removed the show_all call.

> 
>  290     GdkCursor* cursor = gdk_cursor_new(GDK_BLANK_CURSOR);
>  291     m_cursor = gdk_window_get_cursor(window);
>  292     gdk_window_set_cursor(window, cursor);
> 
> I think you need to ref this cursor. Since the cursor is owned by the window, the pointer you got will not necessarily (and more likely not) exist after you set the blank cursor on it.
> 

I guess you're right although I didn't observe what you states ;) I now use GRefPtr for the cursor, just to be safe.

>  298     gtk_window_fullscreen(GTK_WINDOW(m_window));
>  299     gtk_widget_show_all(m_hudWindow);
>  300     showHud(true);
> 
> You are already showing the m_hudWindow inside showHud, why do you need this call to gtk_widget_show_all() here?
> 

Remove show_all call ;)

>  319     // Scale the hud to the full width of the screen.
>  320     gtk_window_resize(GTK_WINDOW(m_hudWindow), fullscreenRectangle.width, hudHeight);
> 
> Scale -> resize. Scale gives the wrong idea.
> 

ok

> Some general comments: we have a number of callbacks taking the video controller object as the data. This has been, I think, one of the top three causes of crashes in webkitgtk code as far as I remember. mrobinson figured out a way to make this kind of code more robust, using GClosures. Check this change out, I'd recommend doing something similar for these: http://trac.webkit.org/changeset/57901. 

Sorry but I don't understand this. Where exactly should I use GClosures? g_signal_connect already internally uses GClosures. Would you please explain a bit more?

> Also, you don't seem to have many checks in place for the case where enterFullScreen fails, and returns 0.
> 

Made it return a bool and adapted the controller's enterFullscreen accordingly.

> There are two things I really don't like: the code uses the media element directly for many things, and seems to duplicate some code, I think that might be something we'll have to accept, though, I haven't been able to assess if you could use the player instead of messing with the element. 

Using the private player you mean? or the MediaPlayer instance?
It'd be nice if I could use the MediaPlayer instance but using the media element here is better (I think) because it's the highest-level API. So by using it we are sure our controller is using the high level APIs as defined in the spec.

> I really dislike how enterFullscreen returns a gulong, though. That feels really dirty heh. I would prefer having another method, platformWindow(), say, that would return a typedef'ed type, having each implementation return its window type, with no need for ugly reinterpret_cast'ing, and stuff. enterFullScreen could return a boolean to let the caller know things went through or not. So you'd have, instead of:
> 
>  266     m_window = reinterpret_cast<GtkWidget*>(m_mediaElement->platformMedia().media.gstreamerGWorld->enterFullscreen());
> 
> this:
> 
>     if (!m_mediaElement->platformMedia().media.gstreamerGWorld->enterFullscreen())
>         return;
> 
>     m_window = m_mediaElement->platformMedia().media.gstreamerGWorld->platformWindow();
> 

Done that, I think :)
A PlatformVideoWindow class with a Gtk implementation.

> I don't really like these huge chains, too, btw, can we just hold a pointer to gstreamerGWorld?

Sure!

> Finally, I think this design is a good enough one for a first iteration, but I think we should consider factoring out the "interfacing with the media element/player" into a MediaPlayerClient so that we could provide API for browsers to know about/control the media elements in a way that is higher level than using the DOM bindings. I'll say r- for now so we can get these comments worked on, thanks for working on this, again =).

Yes that'd be really great. The MediaPlayer would need some changes to accept multiple clients instead of only one though.

Will attach the updated patch once I tested it in my debug build

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.