After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 752441 - gtk: Create a base class to remove code duplication
gtk: Create a base class to remove code duplication
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 752442
 
 
Reported: 2015-07-15 21:07 UTC by Nicolas Dufresne (ndufresne)
Modified: 2016-03-08 05:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gtk: Add GtkGstBaseWidget (13.29 KB, patch)
2015-07-16 21:12 UTC, Nicolas Dufresne (ndufresne)
reviewed Details | Review
gtksink: Port to GtkGstBaseWidget (17.72 KB, patch)
2015-07-16 21:12 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
gtkglsink: Port to GtkGstBaseWidget (33.38 KB, patch)
2015-07-16 21:12 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
gtkbasesink: Create a base class (15.54 KB, patch)
2015-07-16 21:12 UTC, Nicolas Dufresne (ndufresne)
reviewed Details | Review
gtksink: Port to GstGtkBaseSink (14.25 KB, patch)
2015-07-16 21:12 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
gtkglsink: Port to GstGtkBaseSink base class (19.56 KB, patch)
2015-07-16 21:12 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
gtk: Add GtkGstBaseWidget (13.29 KB, patch)
2015-07-17 19:23 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
gtksink: Port to GtkGstBaseWidget (17.72 KB, patch)
2015-07-17 19:23 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
gtkglsink: Port to GtkGstBaseWidget (33.48 KB, patch)
2015-07-17 19:23 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
gtkbasesink: Create a base class (15.74 KB, patch)
2015-07-17 19:23 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
gtksink: Port to GstGtkBaseSink (14.30 KB, patch)
2015-07-17 19:23 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
gtkglsink: Port to GstGtkBaseSink base class (19.62 KB, patch)
2015-07-17 19:23 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
gtkgstbasewidget: Pass already parsed VideoInfo (1.79 KB, patch)
2015-07-17 19:25 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
gtkgstbasewidget: Fix black frame on resize (6.72 KB, patch)
2015-07-17 19:25 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
gtkglsink: Don't leak vertex array and buffers (1.54 KB, patch)
2015-07-17 19:25 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
gtkgstbasewidget: Pass already parsed VideoInfo (3.23 KB, patch)
2015-07-17 19:42 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
gtkgstbasewidget: Fix black frame on resize (6.23 KB, patch)
2015-07-17 19:42 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review
gtkglsink: Don't leak vertex array and buffers (1.54 KB, patch)
2015-07-17 19:42 UTC, Nicolas Dufresne (ndufresne)
committed Details | Review

Description Nicolas Dufresne (ndufresne) 2015-07-15 21:07:17 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.
Comment 1 Nicolas Dufresne (ndufresne) 2015-07-16 21:12:34 UTC
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.
Comment 2 Nicolas Dufresne (ndufresne) 2015-07-16 21:12:38 UTC
Created attachment 307588 [details] [review]
gtksink: Port to GtkGstBaseWidget
Comment 3 Nicolas Dufresne (ndufresne) 2015-07-16 21:12:42 UTC
Created attachment 307589 [details] [review]
gtkglsink: Port to GtkGstBaseWidget
Comment 4 Nicolas Dufresne (ndufresne) 2015-07-16 21:12:46 UTC
Created attachment 307590 [details] [review]
gtkbasesink: Create a base class

This contains all the common code between the gtkglsink and
gtksink.
Comment 5 Nicolas Dufresne (ndufresne) 2015-07-16 21:12:50 UTC
Created attachment 307591 [details] [review]
gtksink: Port to GstGtkBaseSink
Comment 6 Nicolas Dufresne (ndufresne) 2015-07-16 21:12:54 UTC
Created attachment 307592 [details] [review]
gtkglsink: Port to GstGtkBaseSink base class
Comment 7 Mathieu Duponchelle 2015-07-16 23:07:23 UTC
Review of attachment 307587 [details] [review]:

Copyrights :)
Comment 8 Nicolas Dufresne (ndufresne) 2015-07-17 12:43:11 UTC
(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.
Comment 9 Xavier Claessens 2015-07-17 13:55:26 UTC
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.
Comment 10 Xavier Claessens 2015-07-17 14:10:25 UTC
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.
Comment 11 Nicolas Dufresne (ndufresne) 2015-07-17 14:44:35 UTC
(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.
Comment 12 Nicolas Dufresne (ndufresne) 2015-07-17 19:23:08 UTC
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.
Comment 13 Nicolas Dufresne (ndufresne) 2015-07-17 19:23:12 UTC
Created attachment 307630 [details] [review]
gtksink: Port to GtkGstBaseWidget
Comment 14 Nicolas Dufresne (ndufresne) 2015-07-17 19:23:16 UTC
Created attachment 307631 [details] [review]
gtkglsink: Port to GtkGstBaseWidget
Comment 15 Nicolas Dufresne (ndufresne) 2015-07-17 19:23:21 UTC
Created attachment 307632 [details] [review]
gtkbasesink: Create a base class

This contains all the common code between the gtkglsink and
gtksink.
Comment 16 Nicolas Dufresne (ndufresne) 2015-07-17 19:23:26 UTC
Created attachment 307633 [details] [review]
gtksink: Port to GstGtkBaseSink
Comment 17 Nicolas Dufresne (ndufresne) 2015-07-17 19:23:30 UTC
Created attachment 307634 [details] [review]
gtkglsink: Port to GstGtkBaseSink base class
Comment 18 Nicolas Dufresne (ndufresne) 2015-07-17 19:25:00 UTC
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.
Comment 19 Nicolas Dufresne (ndufresne) 2015-07-17 19:25:04 UTC
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.
Comment 20 Nicolas Dufresne (ndufresne) 2015-07-17 19:25:08 UTC
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.
Comment 21 Nicolas Dufresne (ndufresne) 2015-07-17 19:42:18 UTC
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.
Comment 22 Nicolas Dufresne (ndufresne) 2015-07-17 19:42:24 UTC
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.
Comment 23 Nicolas Dufresne (ndufresne) 2015-07-17 19:42:28 UTC
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.
Comment 24 Nicolas Dufresne (ndufresne) 2015-07-17 20:17:52 UTC
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