GNOME Bugzilla – Bug 752441
gtk: Create a base class to remove code duplication
Last modified: 2016-03-08 05:35:41 UTC
So far the GTK sink elements have presented an important amount of defects. These fixes had to be copy pasted over and over, which lead to errors. This is becoming more of an issue now that we start having to redesigned things like the approximate resize code. I propose to move all the common code to a base class, so we stop this copy paste dance. As all this remains private, we don't have to worry much about API stability.
Created attachment 307587 [details] [review] gtk: Add GtkGstBaseWidget This is a "pseudo" base class. Basically it's a shared instance and class structure and a shared set of function between the two widget. It cannot have it's own type like normal base class since the one instance will implement GtkGLArea while the other implements GtkDrawingAreay. To workaround this, the parent instance and class is a union of both.
Created attachment 307588 [details] [review] gtksink: Port to GtkGstBaseWidget
Created attachment 307589 [details] [review] gtkglsink: Port to GtkGstBaseWidget
Created attachment 307590 [details] [review] gtkbasesink: Create a base class This contains all the common code between the gtkglsink and gtksink.
Created attachment 307591 [details] [review] gtksink: Port to GstGtkBaseSink
Created attachment 307592 [details] [review] gtkglsink: Port to GstGtkBaseSink base class
Review of attachment 307587 [details] [review]: Copyrights :)
(In reply to Mathieu Duponchelle from comment #7) > Review of attachment 307587 [details] [review] [review]: > > Copyrights :) Well, it's 99% Matthew's code, which I moved into a common base class. I don't think it would be correct to put my copyright on that.
Review of attachment 307587 [details] [review]: ::: ext/gtk/gtkgstbasewidget.c @@ +280,3 @@ + + if (widget->reset) + widget->reset (widget); You don't seems to ever set a value to widget->reset() @@ +290,3 @@ + if (!widget->resize_id) { + widget->resize_id = g_idle_add_full (G_PRIORITY_DEFAULT, + (GSourceFunc) _queue_resize, widget, NULL); You don't need _full if you set DEFAULT priority and no destroynotify. @@ +312,3 @@ + if (!widget->draw_id) { + widget->draw_id = g_idle_add_full (G_PRIORITY_DEFAULT, + (GSourceFunc) _queue_draw, widget, NULL); You don't need _full if you set DEFAULT priority and no destroynotify.
Review of attachment 307590 [details] [review]: ::: ext/gtk/gstgtkbasesink.c @@ +150,3 @@ +{ + if (gtk_sink->widget != NULL) + return gtk_sink->widget; I would add here an assert that we are in the main thread. Something like: g_assert (g_main_context_is_owner(NULL)); But the default main context could not yet be acquired, if we are called from main() before we first call g_main_loop_run(), I think. Maybe rather: g_assert (g_main_context_acquire(NULL)); But then you have to release it somewhere... Maybe just add a comment saying it must happen in GTK thread. And at least tell it in the property doc. @@ +249,3 @@ + window = gtk_window_new (GTK_WINDOW_TOPLEVEL); + gtk_window_set_default_size (GTK_WINDOW (window), 640, 480); + gtk_window_set_title (GTK_WINDOW (window), "Gtk+ Cairo renderer"); The title must come from subclass.
(In reply to Xavier Claessens from comment #9) > Review of attachment 307587 [details] [review] [review]: > > ::: ext/gtk/gtkgstbasewidget.c > @@ +280,3 @@ > + > + if (widget->reset) > + widget->reset (widget); > > You don't seems to ever set a value to widget->reset() It's NULL as per GObject initialization. It is only set for GL. > > @@ +290,3 @@ > + if (!widget->resize_id) { > + widget->resize_id = g_idle_add_full (G_PRIORITY_DEFAULT, > + (GSourceFunc) _queue_resize, widget, NULL); > > You don't need _full if you set DEFAULT priority and no destroynotify. IDLE_DEFAULT (200) is not DEFAULT (0). (In reply to Xavier Claessens from comment #10) > Review of attachment 307590 [details] [review] [review]: > > ::: ext/gtk/gstgtkbasesink.c > @@ +150,3 @@ > +{ > + if (gtk_sink->widget != NULL) > + return gtk_sink->widget; > > I would add here an assert that we are in the main thread. Something like: > g_assert (g_main_context_is_owner(NULL)); > But the default main context could not yet be acquired, if we are called > from main() before we first call g_main_loop_run(), I think. > > Maybe rather: > g_assert (g_main_context_acquire(NULL)); > But then you have to release it somewhere... I don't like this, as this would executing code in a debugging statement. > > Maybe just add a comment saying it must happen in GTK thread. And at least > tell it in the property doc. I'll do. > > @@ +249,3 @@ > + window = gtk_window_new (GTK_WINDOW_TOPLEVEL); > + gtk_window_set_default_size (GTK_WINDOW (window), 640, 480); > + gtk_window_set_title (GTK_WINDOW (window), "Gtk+ Cairo renderer"); > > The title must come from subclass. Yes, thanks.
Created attachment 307629 [details] [review] gtk: Add GtkGstBaseWidget This is a "pseudo" base class. Basically it's a shared instance and class structure and a shared set of function between the two widget. It cannot have it's own type like normal base class since the one instance will implement GtkGLArea while the other implements GtkDrawingAreay. To workaround this, the parent instance and class is a union of both.
Created attachment 307630 [details] [review] gtksink: Port to GtkGstBaseWidget
Created attachment 307631 [details] [review] gtkglsink: Port to GtkGstBaseWidget
Created attachment 307632 [details] [review] gtkbasesink: Create a base class This contains all the common code between the gtkglsink and gtksink.
Created attachment 307633 [details] [review] gtksink: Port to GstGtkBaseSink
Created attachment 307634 [details] [review] gtkglsink: Port to GstGtkBaseSink base class
Created attachment 307635 [details] [review] gtkgstbasewidget: Pass already parsed VideoInfo As the base sink already parse the caps into VideoInfo it makes sense to pass in VideoInfo to the widget instead.
Created attachment 307636 [details] [review] gtkgstbasewidget: Fix black frame on resize This is solved by only applying the new format when the next buffer is to be rendered and on the GTK thread.
Created attachment 307637 [details] [review] gtkglsink: Don't leak vertex array and buffers This is now possible since reset is always called from the main thread.
Created attachment 307638 [details] [review] gtkgstbasewidget: Pass already parsed VideoInfo As the base sink already parse the caps into VideoInfo it makes sense to pass in VideoInfo to the widget instead.
Created attachment 307639 [details] [review] gtkgstbasewidget: Fix black frame on resize This is solved by only applying the new format when the next buffer is to be rendered and on the GTK thread.
Created attachment 307640 [details] [review] gtkglsink: Don't leak vertex array and buffers This is now possible since reset is always called from the main thread.
Attachment 307629 [details] pushed as ad45bcd - gtk: Add GtkGstBaseWidget Attachment 307630 [details] pushed as b53f859 - gtksink: Port to GtkGstBaseWidget Attachment 307631 [details] pushed as 13ae5cb - gtkglsink: Port to GtkGstBaseWidget Attachment 307632 [details] pushed as 0fc6765 - gtkbasesink: Create a base class Attachment 307633 [details] pushed as 84bc5ad - gtksink: Port to GstGtkBaseSink Attachment 307634 [details] pushed as 785b7bd - gtkglsink: Port to GstGtkBaseSink base class Attachment 307638 [details] pushed as d0fd6a0 - gtkgstbasewidget: Pass already parsed VideoInfo Attachment 307639 [details] pushed as 410ffd5 - gtkgstbasewidget: Fix black frame on resize Attachment 307640 [details] pushed as 5e87b9f - gtkglsink: Don't leak vertex array and buffers