GNOME Bugzilla – Bug 522009
cheese just crashes if some gstreamer plugins arent available
Last modified: 2008-08-11 15:44:05 UTC
if a user is missing the oggmux plugin e.g. cheese crashes. we should add a proper error handling and exit smoothly
*** Bug 524566 has been marked as a duplicate of this bug. ***
*** Bug 524229 has been marked as a duplicate of this bug. ***
*** Bug 522340 has been marked as a duplicate of this bug. ***
*** Bug 518349 has been marked as a duplicate of this bug. ***
I will implement the error handling if you describe what should happen upon the detection of the problem. I can't guarantee that my patches will be very good (this will be my first attempt at working on such a project), but it sounds manageable.
Created attachment 112226 [details] [review] Patch to clean up crash from missing plugins. Not sure if this is completely how the crash should be handled, but the patch will at least notify the user via a dialog and then call gtk_main_quit.
Todd, thanks for looking into this, from a design point of view we should not add GUI code in cheese-webcam.c, because this makes it less reusable. In my opinion a better approach is to use the GError system [1] to pass the error up to the code calling cheese-webcam functions. In this case the cheese-webcam functions are used in cheese-window. Overthere would be the right place to show the dialog. Jaap [1] http://library.gnome.org/devel/glib/stable/glib-Error-Reporting.html
Ah, thank you very much. I'll work on getting that set up.
Created attachment 112246 [details] [review] Updated patch I hope this one is a bit better.
*** Bug 530441 has been marked as a duplicate of this bug. ***
Todd, You're getting there :-). I suggest you just make a few changes. 1. Your patch changes formatting of existing code, which breaks the code style we use. E.g. @@ -1325,14 +1325,14 @@ menu = gtk_menu_new(); menuitem = gtk_menu_item_new_with_label(_("Quit")); g_signal_connect_swapped(menuitem,"activate", - GTK_SIGNAL_FUNC(cheese_window_cmd_close), - NULL); + GTK_SIGNAL_FUNC(cheese_window_cmd_close), + NULL); Please remove these changes from the patch. In general just put stuff in the patch that are real changes. This makes them easier to review. If there are code style formatting errors please submit a separate patch. 2. Please change CHEESE_WEBCAM_ERROR_NOELT to CHEESE_WEBCAM_ERROR_ELEMENT_NOT_FOUND 3. Rename cheese_element_create_failed (const char *factoryname, GError **error) to cheese_webcam_set_error_element_not_found (GError **error, const char* element_name) Thanks for working on this
Created attachment 112361 [details] [review] Updated patch Fixed the problems mentioned.
Thanks it's in SVN now 2008-06-08 Jaap Haitsma <jaap@haitsma.org> * src/cheese-webcam.c, src/cheese-webcam.h, src/cheese-window.c: Instead of crashing report when certain gstreamer are not found. Fixes bug #522009. Patch by Todd Eisenberger
Reopening: I now have a nice critical error window (I'm not sure "Critical Error" is the best window title, btw). And when I close it, I get a segmentation fault.
Could you provide a backtrace and the errors and warnings that are given before the segfault?
(gdb) run Starting program: /home/vuntz/work/gnome/releases/usr/bin/cheese [Thread debugging using libthread_db enabled] [New Thread 0xb52aeb90 (LWP 19744)] [New Thread 0xb4aadb90 (LWP 19745)] (cheese:19739): Gdk-CRITICAL **: gdk_window_set_geometry_hints: assertion `GDK_IS_WINDOW (window)' failed (cheese:19739): GStreamer-CRITICAL **: gst_element_set_state: assertion `GST_IS_ELEMENT (element)' failed (cheese:19739): GStreamer-CRITICAL **: gst_object_unref: assertion `object != NULL' failed (cheese:19739): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed (cheese:19739): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed (cheese:19739): GLib-GObject-WARNING **: instance with invalid (NULL) class pointer (cheese:19739): GLib-GObject-CRITICAL **: g_signal_connect_data: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed (cheese:19739): GLib-GObject-WARNING **: instance with invalid (NULL) class pointer (cheese:19739): GLib-GObject-CRITICAL **: g_signal_connect_data: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed Program received signal SIGSEGV, Segmentation fault.
+ Trace 204739
Thread 3039488912 (LWP 19744)
Created attachment 116004 [details] [review] Fix segfault It appears that the problem was caused by a missing return statement. When I wrote the previous patch, I had thought that gtk_main_quit causes the program to terminate immediately. Since it does not, we had the wonderful problem of a dangling pointer. The return should allow for the main loop to regain control and terminate safely. As for changing the title from Critical Error, if you have any suggestions, I would be glad to accept them. I've never been very good at error messages.
vincent, does this fix the problem for you?
Yes. There are still a few critical warnings, though.
Created attachment 116016 [details] [review] Correct critical warnings This patch fixes most of the critical warnings. I believe the other warnings that I am getting are unrelated to this bug, as I get them whether the plugins or missing or not. These are (cheese:15019): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed (cheese:15019): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed and appear to come from cheese-window.c:287. The critical warnings that were fixed were caused by the detection of a missing plugin before the creation of the pipeline, leading to the usage of an uninitialized pointer.
thanks, todd. i fixed that useless warning in svn. lets see if vincent still sees that error
Confirming the second patch is okay.
comitted to trunk, thanks todd!
Created attachment 116034 [details] [review] hig compliant alert Just a little step further: replace the "critical error" with a more hig compliant alert (modal, destroy with parent and better message). Vincent, like it more? any suggestion? I think we're not done yet with this bug. The alert dialog should give a more detailed message about missing elements. At the moment we return at the first missing element and report just that in the error message. I think we shouln't return and we should build an error message listing all the missing elements so that the user can solve his issue with less pain.
Reopening, see previous comment.
committed last patch (fixed the "One of more" typo too).
Created attachment 116079 [details] [review] Create error list/Fix segfault This patch should handle reporting a full list, as well as fix a segfault that occurs if the v4l2 plugin is missing.
Hi Todd, thank you for your patch but it's not ready yet! - setting an error message like "The element '%s' could not be found." for each missing element could lead to a huge error string. (Try to chmod -r libgstcoreelements.so to see it). - there are a couple of elements that we check twice or even more (e.g. queue, ffmpegcolorspace). If they cannot be found we have several duplicated error messages. So either we have to not check them more than once or remove duplicates from the error string. So I'm more for a single error string like "The following elements could not be found: oggmux, queue, etc." where each missing element appears only once. Todd would you like to work on this? Please let us know so we can avoid duplating the efforts working on the same thing ;) Ok for the segfault fix, keep up the good work!
ehm, duplicating :P I should check what I wrote before to post
I'd gladly take this one over, but it's likely that I wouldn't be able to provide the next patch until Aug 12, I'm about to take a vacation with no computer access.
Created attachment 116178 [details] [review] Create Error list v2 Found a little time before I left to work on it.
Comment on attachment 116178 [details] [review] Create Error list v2 >Index: cheese-window.c >=================================================================== >--- cheese-window.c (revision 873) >+++ cheese-window.c (working copy) >@@ -1614,7 +1614,7 @@ > gchar *primary, *secondary; > > primary = g_strdup (_("Check your gstreamer installation")); >- secondary = g_strdup_printf (_("One or more needed gstreamer elements are missing:\n" >+ secondary = g_strdup_printf (_("The following elements could not be found: \n" > "<i>%s</i>"), error->message); > > gdk_threads_enter (); >Index: cheese-webcam.c >=================================================================== >--- cheese-webcam.c (revision 873) >+++ cheese-webcam.c (working copy) >@@ -731,7 +731,28 @@ > static void > cheese_webcam_set_error_element_not_found (GError **error, const char *factoryname) > { >- g_set_error (error, CHEESE_WEBCAM_ERROR, CHEESE_WEBCAM_ERROR_ELEMENT_NOT_FOUND, "The element '%s' could not be found.", factoryname); >+ if (error == NULL) >+ return; >+ >+ if (*error == NULL) >+ { >+ g_set_error (error, CHEESE_WEBCAM_ERROR, CHEESE_WEBCAM_ERROR_ELEMENT_NOT_FOUND, "%s.", factoryname); >+ } >+ else >+ { >+ /* Ensure that what is found is not a substring of an element; all strings >+ * should have a ' ' or nothing to the left and a '.' or ',' to the right */ >+ char *found = strstr((*error)->message, factoryname); >+ char *prev = found - 1; >+ char *next = found + strlen(factoryname); >+ >+ if (found == NULL || >+ ((found == (*error)->message || *prev == ' ') && /* Prefix check */ >+ (*next == ',' || *next == '.'))) /* Postfix check */ >+ { >+ g_prefix_error (error, "%s, ", factoryname); >+ } >+ } > } > > static gboolean >@@ -750,47 +771,44 @@ > if ((priv->effect_filter = gst_element_factory_make ("identity", "effect")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "identity"); >- return FALSE; > } > if ((priv->csp_post_effect = gst_element_factory_make ("ffmpegcolorspace", "csp_post_effect")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "ffmpegcolorspace"); >- return FALSE; > } > > if ((tee = gst_element_factory_make ("tee", "tee")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "tee"); >- return FALSE; > } > > if ((save_queue = gst_element_factory_make ("queue", "save_queue")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "queue"); >- return FALSE; > } > > if ((video_display_queue = gst_element_factory_make ("queue", "video_display_queue")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "queue"); >- return FALSE; > } > > if ((video_scale = gst_element_factory_make ("videoscale", "video_scale")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "videoscale"); >- return FALSE; > } >+ else >+ { >+ /* Use bilinear scaling */ >+ g_object_set (video_scale, "method", 1, NULL); >+ } > >- /* Use bilinear scaling */ >- g_object_set (video_scale, "method", 1, NULL); >- > if ((video_sink = gst_element_factory_make ("gconfvideosink", "video_sink")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "gconfvideosink"); >- return FALSE; > } > >+ if (error != NULL && *error != NULL) >+ return FALSE; > > gst_bin_add_many (GST_BIN (priv->video_display_bin), priv->webcam_source_bin, > priv->effect_filter, priv->csp_post_effect, tee, save_queue, >@@ -829,14 +847,15 @@ > if ((csp_photo_save_bin = gst_element_factory_make ("ffmpegcolorspace", "csp_photo_save_bin")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "ffmpegcolorspace"); >- return FALSE; > } > if ((priv->photo_sink = gst_element_factory_make ("fakesink", "photo_sink")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "fakesink"); >- return FALSE; > } > >+ if (error != NULL && *error != NULL) >+ return FALSE; >+ > gst_bin_add_many (GST_BIN (priv->photo_save_bin), csp_photo_save_bin, > priv->photo_sink, NULL); > >@@ -876,59 +895,59 @@ > if ((priv->audio_source = gst_element_factory_make ("gconfaudiosrc", "audio_source")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "gconfaudiosrc"); >- return FALSE; > } > if ((audio_queue = gst_element_factory_make ("queue", "audio_queue")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "queue"); >- return FALSE; > } > if ((audio_convert = gst_element_factory_make ("audioconvert", "audio_convert")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "audioconvert"); >- return FALSE; > } > if ((audio_enc = gst_element_factory_make ("vorbisenc", "audio_enc")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "vorbisenc"); >- return FALSE; > } > > if ((video_save_csp = gst_element_factory_make ("ffmpegcolorspace", "video_save_csp")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "ffmpegcolorspace"); >- return FALSE; > } > if ((video_enc = gst_element_factory_make ("theoraenc", "video_enc")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "theoraenc"); >- return FALSE; > } >- g_object_set (video_enc, "keyframe-force", 1, NULL); >+ else >+ { >+ g_object_set (video_enc, "keyframe-force", 1, NULL); >+ } >+ > if ((video_save_rate = gst_element_factory_make ("videorate", "video_save_rate")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "videorate"); >- return FALSE; > } > if ((video_save_scale = gst_element_factory_make ("videoscale", "video_save_scale")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "videoscale"); >- return FALSE; > } >- /* Use bilinear scaling */ >- g_object_set (video_save_scale, "method", 1, NULL); >+ else >+ { >+ /* Use bilinear scaling */ >+ g_object_set (video_save_scale, "method", 1, NULL); >+ } > > if ((mux = gst_element_factory_make ("oggmux", "mux")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "oggmux"); >- return FALSE; > } > if ((priv->video_file_sink = gst_element_factory_make ("filesink", "video_file_sink")) == NULL) > { > cheese_webcam_set_error_element_not_found (error, "filesink"); >- return FALSE; > } > >+ if (error != NULL && *error != NULL) >+ return FALSE; >+ > gst_bin_add_many (GST_BIN (priv->video_save_bin), priv->audio_source, audio_queue, > audio_convert, audio_enc, video_save_csp, video_save_rate, video_save_scale, video_enc, > mux, priv->video_file_sink, NULL); >@@ -1355,18 +1374,8 @@ > priv->pipeline = gst_pipeline_new ("pipeline"); > > cheese_webcam_create_video_display_bin (webcam, &tmp_error); >- if (tmp_error != NULL) >- { >- g_propagate_error (error, tmp_error); >- return; >- } > > cheese_webcam_create_photo_save_bin (webcam, &tmp_error); >- if (tmp_error != NULL) >- { >- g_propagate_error (error, tmp_error); >- return; >- } > > cheese_webcam_create_video_save_bin (webcam, &tmp_error); > if (tmp_error != NULL)
Comment on attachment 116178 [details] [review] Create Error list v2 >+ /* Ensure that what is found is not a substring of an element; all strings >+ * should have a ' ' or nothing to the left and a '.' or ',' to the right */ >+ char *found = strstr((*error)->message, factoryname); >+ char *prev = found - 1; >+ char *next = found + strlen(factoryname); >+ >+ if (found == NULL || >+ ((found == (*error)->message || *prev == ' ') && /* Prefix check */ >+ (*next == ',' || *next == '.'))) /* Postfix check */ >+ { >+ g_prefix_error (error, "%s, ", factoryname); >+ } This doesn't work ;)! If the found string is a substring of an element it has to be prefixed not the opposite. Hence the second condition (prefix and postfix check) has to be negated to work as expected. I'd suggest a change like: + gchar *found = g_strrstr((*error)->message, factoryname); + gchar prev = 0; + gchar next = 0; + if (found != NULL) { + prev = *(found - 1); + next = *(found + strlen(factoryname)); + } + + if (found == NULL || + ((found != (*error)->message && prev != ' ') && + (next != ',' && next != '.'))) + { + g_prefix_error (error, "%s, ", factoryname); + } Another thing, the error message should have a meaning itself, with no relation with the alert dialog in the UI side. So I'd leave the alert secondary text as it is and use error->message as an explanatory detail, not just as a list of missing elements. Sorry if I've been less clear previously. error->message should be something like "Unable to create the following elements: element1, element2, ..." or something better if you have any idea.
Created attachment 116307 [details] [review] Create Error list v3 Sorry about that last one, looks like I was in too much of a hurry check my logic. Thanks Filippo for pointing out that one. Hope this one is a bit closer to what we need.
Thanks Todd! I just committed your patch, slightly modified to avoid redundancy in the alert dialog. Closing.