GNOME Bugzilla – Bug 749846
gloverlay crash on missing file
Last modified: 2015-05-26 20:59:34 UTC
Patch coming
Created attachment 303937 [details] [review] gloverlay: do not g_error() on missing file
Actually not crashing is a first good step, but it's not enough. With a missing file the whole video gets black and it prints 2 WARN for each frame: 0:00:02.054407476 13652 0x2368190 WARN gloverlay gstgloverlay.c:635:gst_gl_overlay_load_jpeg: unable to load /plop: couldn't open file 0:00:02.087447698 13652 0x2368190 WARN gloverlay gstgloverlay.c:701:gst_gl_overlay_load_png: unable to load /plop: file not found
Can be tested with: gst-launch-1.0 videotestsrc ! glupload ! glcolorscale ! gloverlay location=/plop ! glimagesink
Created attachment 303938 [details] [review] gloverlay: fix black frames if image file is not found If opening the location fails we shouldn't retry on every frame, and we should still render upstream's video frame.
Review of attachment 303937 [details] [review]: ::: ext/gl/gstgloverlay.c @@ +633,3 @@ fp = fopen (overlay->location, "rb"); if (!fp) { + LOAD_ERROR ("couldn't open file"); Sending a generic error is not nice. We have GST_RESOURCE_ERROR_NOT_FOUND for that. Also, this should result in GST_FLOW_ERROR being returned to stop the streaming thread.
Created attachment 303953 [details] [review] gloverlay: Post error on the bus when image can't be loaded
Review of attachment 303953 [details] [review]: Maybe I should have mentionned GST_ELEMENT_ERROR macro, which you are re-implementing.
Review of attachment 303953 [details] [review]: Looks mostly good. ::: ext/gl/gstgloverlay.c @@ +75,3 @@ +static gint gst_gl_overlay_load_png (GstGLFilter * filter, GError ** error); +static gint gst_gl_overlay_load_jpeg (GstGLFilter * filter, GError ** error); match these with the implementations below. s/gint/gboolean/ @@ +594,3 @@ + gst_element_post_message (GST_ELEMENT_CAST (filter), msg); + g_clear_error (&error); + g_free (debug); GST_ELEMENT_ERROR macro as Nicolas mentioned ::: gst-libs/gst/gl/gstglfilter.c @@ +922,3 @@ gst_gl_sync_meta_set_sync_point (out_sync_meta, context); + return ret ? GST_FLOW_OK : GST_FLOW_ERROR; separate this into a separate patch please
It's actually not that easy. I cannot use GST_ELEMENT_ERROR() in gst_gl_overlay_load_png() because errors are not fatal there, we fallback to try gst_gl_overlay_load_jpeg(). So I have to use the common pattern of returning FALSE with a GError. And GST_ELEMENT_ERROR() does not take a GError, so posting it manually seems the best way... I guess ideally gst_gl_overlay_filter_texture() should be responsibly of opening the file, read the magic number in the header to determine if it's PNG or JPEG, then pass the FILE* to gst_gl_overlay_load_png() or gst_gl_overlay_load_jpeg(). In that case error in those function could use GST_ELEMENT_ERROR(). But maybe that cleanup is work for another bug, if someone care.
I'd says, remove that code, and use a type finder. And catch the "NOT FOUND" using GLib file API maybe ?
Created attachment 304036 [details] [review] gl: propagate return value from filter and filter_texture
Created attachment 304037 [details] [review] gloverlay: remove unused type_file field
Created attachment 304038 [details] [review] gloverlay: properly handle errors while loading file Post an error on the bus if anything bad happens while reading and parsing the image file.
Review of attachment 304038 [details] [review]: Otherwise looks good ! ::: ext/gl/gstgloverlay.c @@ +611,3 @@ + structure = gst_caps_get_structure (caps, 0); + name = gst_structure_get_name (structure); + if (g_str_equal (name, "image/jpeg")) { -> gst_structure_has_name()
Review of attachment 304036 [details] [review]: .
Review of attachment 304037 [details] [review]: .
Created attachment 304040 [details] [review] gloverlay: properly handle errors while loading file Post an error on the bus if anything bad happens while reading and parsing the image file.
Review of attachment 304040 [details] [review]: ::: ext/gl/gstgloverlay.c @@ +808,2 @@ if (!gst_memory_map ((GstMemory *) overlay->image_memory, &map_info, GST_MAP_WRITE)) { Leak, png_destroy_read_struct (&png_ptr, &info_ptr, png_infopp_NULL);
This leak was there before, so a new patch is fine.
Created attachment 304041 [details] [review] gloverlay: fix a leak
Attachment 304036 [details] pushed as 7bfd270 - gl: propagate return value from filter and filter_texture Attachment 304037 [details] pushed as 02b2c2e - gloverlay: remove unused type_file field Attachment 304040 [details] pushed as 53cd9e3 - gloverlay: properly handle errors while loading file Attachment 304041 [details] pushed as 2bb01f9 - gloverlay: fix a leak