GNOME Bugzilla – Bug 791988
jpeg: Fixup frames without an EOI marker
Last modified: 2017-12-27 18:42:07 UTC
Created attachment 366021 [details] [review] jpeg: Fixup frames without an EOI marker Quoting the patch attached: Some cameras fail to send an end-of-image marker (EOI) and can't be properly decoded by either JPEG or libjpeg. This commit parses the frame, making sure it has an EOI. If there isn't one, the EOI gets added to the buffer. A similar fixup is done in the rtpjpegdepay element, and it makes sense to do it in jpegdec as well. An equivalent version of this change is used in production. We came across some cameras that send frames without EOI, thus breaking one of our gstreamer pipelines; the attached patch fixes the problem. This patch is tested via a crafted corrupted JPEGs, removing EOI markers: # create a three-frame stream $ gst-launch-1.0 videotestsrc num-buffers=3 ! jpegenc ! filesink location=j.jpeg # search and remove EOI $ hexedit # we good? $ GST_DEBUG=jpegdec:9 gst-launch-1.0 filesrc location=j.jpeg ! jpegdec ! fakesink
Review of attachment 366021 [details] [review]: Makes sense to me, two trivial remarks. It is annoying that we have to add the EOI at all, but if you say underlying libraries require one then we don't have much of a choice :) ::: ext/jpeg/gstjpegdec.c @@ -1009,3 @@ G_GNUC_UNUSED GstFlowReturn ret; guint r_h, r_v, hdr_ok; - No need for that new line :) @@ +1204,3 @@ + has_eoi = ((data[nbytes - 2] != 0xff) || (data[nbytes - 1] != 0xd9)); + + if (!has_eoi) { Can you add a brief comment here explaining why we have to do that?
Review of attachment 366021 [details] [review]: Changing status
Review of attachment 366021 [details] [review]: ::: ext/jpeg/gstjpegdec.c @@ -1009,3 @@ G_GNUC_UNUSED GstFlowReturn ret; guint r_h, r_v, hdr_ok; - No need to remove that new line*
(In reply to Mathieu Duponchelle from comment #3) > Review of attachment 366021 [details] [review] [review]: > > ::: ext/jpeg/gstjpegdec.c > @@ -1009,3 @@ > G_GNUC_UNUSED GstFlowReturn ret; > guint r_h, r_v, hdr_ok; > - > > No need to remove that new line* Oops. Let me fix that.
Created attachment 366023 [details] [review] v2 - jpeg: Fixup frames without an EOI marke Removes spurious change in v1.
Review of attachment 366023 [details] [review]: Still missing a simple comment about why we have to add EOI :) I think it's a bit more important than comments stating what the code does or will do ;)
Created attachment 366024 [details] [review] v3 - jpeg: Fixup frames without an EOI marker Add a comment explaining why we may need to fixup the frame, adding the EOI.
Thanks!