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 623063 - [jpegdec] add "max-errors" property
[jpegdec] add "max-errors" property
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
0.10.22
Other Windows
: Normal enhancement
: 0.10.27
Assigned To: Tim-Philipp Müller
GStreamer Maintainers
: 626857 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-06-28 19:33 UTC by David Hoyt
Modified: 2010-12-11 20:40 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Adds error-after property to jpegdec (5.72 KB, patch)
2010-06-28 19:33 UTC, David Hoyt
needs-work Details | Review
Max-error property (8.54 KB, patch)
2010-08-09 22:20 UTC, David Hoyt
needs-work Details | Review
max-errors property (8.81 KB, patch)
2010-08-16 23:25 UTC, David Hoyt
none Details | Review

Description David Hoyt 2010-06-28 19:33:27 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 1 Tim-Philipp Müller 2010-08-08 10:53:00 UTC
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!
Comment 2 David Hoyt 2010-08-09 16:46:24 UTC
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?
Comment 3 David Hoyt 2010-08-09 22:20:18 UTC
Created attachment 167466 [details] [review]
Max-error property
Comment 4 Tim-Philipp Müller 2010-08-09 22:35:14 UTC
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?
Comment 5 David Hoyt 2010-08-10 00:02:17 UTC
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.
Comment 6 Tim-Philipp Müller 2010-08-13 15:44:52 UTC
*** Bug 626857 has been marked as a duplicate of this bug. ***
Comment 7 American Dynamics 2010-08-13 16:57:11 UTC
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).
Comment 8 Tim-Philipp Müller 2010-08-14 13:38:36 UTC
> -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.
Comment 9 David Hoyt 2010-08-16 21:42:38 UTC
> >      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.
Comment 10 David Hoyt 2010-08-16 23:25:22 UTC
Created attachment 168019 [details] [review]
max-errors property

Here's a new patch with the requested improvements for your review.
Comment 11 American Dynamics 2010-10-08 21:35:01 UTC
Is this patch a candidate for the next release?
Comment 12 Nicola 2010-11-20 12:36:32 UTC
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
Comment 13 Tim-Philipp Müller 2010-12-11 20:40:15 UTC
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!