GNOME Bugzilla – Bug 599018
caca: Cleanups and fixes
Last modified: 2018-11-03 14:40:16 UTC
Created attachment 145832 [details] [review] 0001-Exit-properly-when-invalid-driver-has-been-selected.patch Currently, when specifying a driver that doesn't exist causes crash in cacasink.
Created attachment 145833 [details] [review] 0002-Minor-cleanups-for-header.patch Clean up the header file.
Created attachment 145834 [details] [review] 0003-Add-driver-selection-support-from-the-pipeline.patch Adds "driver" option for the pipeline. Please note that there's a FIXME in the code ;)
Created attachment 145856 [details] [review] 0003-Add-driver-selection-support-from-the-pipeline.patch
Review of attachment 145856 [details] [review]: ::: ext/libcaca/gstcacasink.c @@ +149,3 @@ + + list = caca_get_display_driver_list (); + Why is this code in the _get_type() function? You're basically rebuildilng the driver list on every type check, so possibly even multiple times for each buffer that arrives. Is that really what you want? But even if, this needs locking really, so that two different threads calling _get_type() don't trample over each other. If the driver list is assumed to be static during the runtime of the program, you could just put a _build_driver_list() call into the plugin_init function or so, or call it from class_init. @@ +161,3 @@ + driver[i].value_nick = g_strdup (list[2 * i]); + driver[i].value_name = g_strdup (list[2 * i + 1]); + } Since you're buildling an enum table here: will the drivers always be enumerated with the same integer/enum values, or only be addressed by string nick/name? If the latter, then maybe this should be a string property instead, since the assumption for enum properties is that you could theoretically hard-code numbers and still always get what you expect. @@ +358,3 @@ + case ARG_DRIVER:{ + cacasink->driver = g_value_get_enum (value); + } A break here woudl be nice, even if it's not stricly needed.
Priit: ping? Are you going to work on this some more? Do you know the answer to any of the questions any asked?
(In reply to comment #5) > Priit: ping? Are you going to work on this some more? Do you know the answer to > any of the questions any asked? I haven't had much time to look into this due to school and I probably won't have time until um.. second part of summer :(
Hm. But if you want to do it, you could assign this bug to yourself and get it done as soon as you come around to do it :-)
Folks, this bugs is lacking attention for quite some time now. And there is a patch bit rotting around. If nobody steps up, I feel it's appropriate to close as INCOMPLETE.
> If nobody steps up, I feel it's appropriate to close > as INCOMPLETE. I don't, please keep it open, thanks.
It would be nice if the enum values could be made consistent, but I don't see a way. On the other hand, using a string property leaves no way to discover which device backends are available - which an enum does, so the inconsistent enum is probably better.
Also this needs to be ported to 1.0 now
Created attachment 278676 [details] [review] cacasink: add driver selection support from the pipeline
I rebased the patch on top of 1.2 and tried fixing the review comments. Still not sure if we want an enum or string property though.
Review of attachment 278676 [details] [review]: ::: ext/libcaca/gstcacasink.c @@ +190,3 @@ + g_object_class_install_property (G_OBJECT_CLASS (klass), PROP_DRIVER, + g_param_spec_enum ("driver", "driver", "Output driver", + GST_TYPE_CACADRIVER, 0, G_PARAM_READWRITE)); | G_PARAM_STATIC_STRINGS @@ +407,3 @@ + cacasink->dp = + caca_create_display_with_driver (cacasink->cv, + driver_array[cacasink->driver]); You can avoid the driver_array. GEnumValue *ev = g_enum_get_value (enum_class, cacasink->driver); caca_create_display_with_driver (cacasink->cv, ev->value_nick)
Regarding the enums, can we assume that future libcaca versions would append new drivers? If they remove one the would indeed break the stability.
Created attachment 294365 [details] [review] cacasink: add driver selection support from the pipeline
Review of attachment 294365 [details] [review]: Just a small suggestions. Thanks! ::: ext/libcaca/gstcacasink.c @@ +404,3 @@ goto init_failed; + enum_class = g_type_class_ref (GST_TYPE_CACADRIVER); its probably better to use g_type_class_peek() here, the type is definitely loaded (as it is an enum on the class) - this way you don't need to unref
Created attachment 295796 [details] [review] cacasink: add driver selection support from the pipeline
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/17.