GNOME Bugzilla – Bug 623063
[jpegdec] add "max-errors" property
Last modified: 2010-12-11 20:40:43 UTC
Created attachment 164834 [details] [review] Adds error-after property to jpegdec I've added an "error-after" property that emits GST_FLOW_ERROR only after a certain number of errors are found. We have cameras that occasionally emit a bad jpeg frame once every few days and this allows us to continue streaming despite the error. It's done on a slightly older version of jpegdec, but I hope it's still applicable to the latest in the repo. Sorry, I don't have git on my machine (I'm on Windows) to submit a patch in the desired format.
Comment on attachment 164834 [details] [review] Adds error-after property to jpegdec I think the property should be called "max-errors" or somesuch, not "error-after" ("error-after" means something else). Also, I wonder if one can do something to make these if/else constructions that repeat the error message/debug stuff prettier. (A macro maybe?) Furthermore, your patch contains tabs. If you can't be bothered to install gnu indent and gst-indent to make sure your code and patches adhere to the coding guidelines, please at least attempt to mimic the indentation style (for *.c files only, not header files), which is: indentation with spaces, no tabs. This also makes the patch easier to read and review. Thanks!
You're exactly right -- I submitted this a while back before I'd done a lot of serious gst coding. Do you require the macro before the patch is accepted or just the tab/space fix?
Created attachment 167466 [details] [review] Max-error property
Review of attachment 167466 [details] [review]: ::: ext/jpeg/gstjpegdec.h @@ +117,3 @@ + + /* number of errors we're currently at before reporting GST_FLOW_ERROR */ + gint max_error_count; Should be error_count IMHO. ::: ext/jpeg/gstjpegdec.c @@ +54,2 @@ #define JPEG_DEFAULT_IDCT_METHOD JDCT_FASTEST +#define JPEG_DEFAULT_MAX_ERRORS -1 What does -1 mean here? (vs. 0) @@ +195,3 @@ + + g_object_class_install_property (gobject_class, PROP_MAX_ERRORS, + g_param_spec_int ("error-after", "Error After", "Error after N buffers", You forgot to changes this to "max-errors" here. (Description needs fixing too then btw.) @@ +196,3 @@ + g_object_class_install_property (gobject_class, PROP_MAX_ERRORS, + g_param_spec_int ("error-after", "Error After", "Error after N buffers", + G_MININT, G_MAXINT, JPEG_DEFAULT_MAX_ERRORS, Surely the minimum is 0? Or -1 for 'default' (which could be mode-dependent, ie. 0 for still image and N for MJPEG). @@ +1681,3 @@ break; + case PROP_MAX_ERRORS: + dec->max_errors = g_value_get_int (value); Technically this needs locking and/or g_atomic_int_{get|set}(), but then the method property dosn't do that either.. @@ +1722,3 @@ switch (transition) { case GST_STATE_CHANGE_READY_TO_PAUSED: + dec->max_error_count = dec->max_errors; Why this? Shouldn't it be reset to 0?
Review of attachment 167466 [details] [review]: ::: ext/jpeg/gstjpegdec.h @@ +117,3 @@ + + /* number of errors we're currently at before reporting GST_FLOW_ERROR */ + gint max_error_count; Not a problem. ::: ext/jpeg/gstjpegdec.c @@ +54,2 @@ #define JPEG_DEFAULT_IDCT_METHOD JDCT_FASTEST +#define JPEG_DEFAULT_MAX_ERRORS -1 -1 means to always do the default behavior. In the future it could be expanded to mean different things for different modes. My goal here was to reduce impact to the maximum extent possible. @@ +195,3 @@ + + g_object_class_install_property (gobject_class, PROP_MAX_ERRORS, + g_param_spec_int ("error-after", "Error After", "Error after N buffers", Good catch. @@ +196,3 @@ + g_object_class_install_property (gobject_class, PROP_MAX_ERRORS, + g_param_spec_int ("error-after", "Error After", "Error after N buffers", + G_MININT, G_MAXINT, JPEG_DEFAULT_MAX_ERRORS, Good catch. @@ +1681,3 @@ break; + case PROP_MAX_ERRORS: + dec->max_errors = g_value_get_int (value); What do you suggest be done? Leave it as is since it's mirroring other properties? @@ +1722,3 @@ switch (transition) { case GST_STATE_CHANGE_READY_TO_PAUSED: + dec->max_error_count = dec->max_errors; Elsewhere we're setting the error count to zero when good frames come in. This just ensures that the error count is capped at every ready -> paused transition (in case you had more than max_errors before the pipeline was able to shutdown). I don't think it would hurt to reset it to 0.
*** Bug 626857 has been marked as a duplicate of this bug. ***
I was curious on what the feasibility would be of using GST_ELEMENT_WARNING instead of GST_ELEMENT_INFO? I'm not trying to split hairs here, but to me a random bad decode could be indicative of something possibly being bad (hence the warning).
> -1 means to always do the default behavior. In the future it could be expanded > to mean different things for different modes. > My goal here was to reduce impact to the maximum extent possible. Ok, not a problem, should just be documented in the property description, e.g.: "Error out after this many consecutive decoding errors (-1 = automatic)" (feel free to rephrase this) > + case PROP_MAX_ERRORS: > + dec->max_errors = g_value_get_int (value); > > What do you suggest be done? Leave it as is since it's mirroring other > properties? I'm fine with a /* FIXME: not thread-safe */, but fixing the code to use g_atomic_int_{set|get} for this structure member is also not particularly hard if you want to take a stab. > case GST_STATE_CHANGE_READY_TO_PAUSED: > + dec->max_error_count = dec->max_errors; > > Elsewhere we're setting the error count to zero when good frames come in. This > just ensures that the error count is capped at every ready -> paused transition > (in case you had more than max_errors before the pipeline was able to > shutdown). I'm not sure I understand this. The question for me is basically: does this work if the very first frame is broken, but all consecutive frames are fine? (I think not?) (In reply to comment #7) > I was curious on what the feasibility would be of using GST_ELEMENT_WARNING > instead of GST_ELEMENT_INFO? I'm not trying to split hairs here, but to me a > random bad decode could be indicative of something possibly being bad (hence > the warning). Yes, making this GST_ELEMENT_WARNING is probably more appropriate.
> > case GST_STATE_CHANGE_READY_TO_PAUSED: > > + dec->max_error_count = dec->max_errors; > > > > Elsewhere we're setting the error count to zero when good frames come in. This > > just ensures that the error count is capped at every ready -> paused transition > > (in case you had more than max_errors before the pipeline was able to > > shutdown). > > I'm not sure I understand this. The question for me is basically: does this > work if the very first frame is broken, but all consecutive frames are fine? (I > think not?) It should work just fine b/c the counter is reset (e.g. on line 946) before it checks this value.
Created attachment 168019 [details] [review] max-errors property Here's a new patch with the requested improvements for your review.
Is this patch a candidate for the next release?
you can find a test video, with a broken jpeg, at the following link: http://77.43.75.110/temp/2010-11-20T11-56-35-817596.avi
Not particularly pretty either, but as I mentioned on IRC I'm not particularly keen on all this bloat in the error goto labels in the decoding code paths, so did it slightly differently: commit addbc3c4ca29a6869fe21ea5b1228a496ac0f26a Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Sat Dec 11 17:39:20 2010 +0000 jpegdec: add "max-errors" property to ignore decoding errors Add property to ignore decoding errors. Default is to ignore a few decoding errors if the input is packetized, but error out immediately if the input is not packetized. Ignoring errors for packetized input most likely doesn't work properly yet, so don't do that for now. https://bugzilla.gnome.org/show_bug.cgi?id=623063 Please test and let me know if something doesn't work as expected. Thanks!