GNOME Bugzilla – Bug 766010
jifmux fails to find EOI if it's not within the last 5 bytes
Last modified: 2018-11-03 13:50:46 UTC
The attached image makes jifmux barf with the following pipeline: GST_DEBUG='*:2' gst-launch-1.0 filesrc num-buffers=1 blocksize=4299656 location=1462395350.jpg ! 'image/jpeg' ! jifmux ! filesink location=foo.jpg Setting pipeline to PAUSED ... Pipeline is PREROLLING ... 0:00:00.027655752 10721 0x872c290 WARN jifmux gstjifmux.c:341:gst_jif_mux_parse_image:<jifmux0> Couldn't find an EOI marker 0:00:00.027717421 10721 0x872c290 WARN jifmux gstjifmux.c:371:gst_jif_mux_parse_image:<jifmux0> Error parsing image header (need more that 0 bytes available) Pipeline is PREROLLED ... Setting pipeline to PLAYING ... It seems EOI marker is not present within the last 5 bytes thus jifmux fails to find it.
Created attachment 327324 [details] [review] proposed patch Attaching a proposed patch. The patch makes the error go away and the produced image seems to be valid. The patch is against 1.4.5 but I think it should still apply. If it does not then I can rebase the patch.
The jpeg image exceeds the maximum size allowed so I uploaded it here http://foolab.org/files/1462395350.jpg
Created attachment 327338 [details] [review] patch V2 I forgot a g_print() so just removed it
Out of curiosity, was this file handcrafted or created by some device? I see it has 00 bytes padding at the end. Need to check if this is still considered a valid jpeg. How does this bug in jifmux affects you, other than the warning? I guess after your patch the resulting file should have the padding removed?
(In reply to Thiago Sousa Santos from comment #4) > Out of curiosity, was this file handcrafted or created by some device? I see > it has 00 bytes padding at the end. Need to check if this is still > considered a valid jpeg. It was created by a device (Oneplus phone but not sure which one). It was captured via android camera. > How does this bug in jifmux affects you, other than the warning? I guess > after your patch the resulting file should have the padding removed? We use jifmux as part of our image capture pipeline for Sailfish OS. We get jpeg images from android HAL and we need to "mux" additional exif tags. When jifmux fails to parse the image, it also ignores any tags set by the application. With the proposed patch the tags are muxed correctly.
Sounds fair to me. Assuming that the scan data should be larger than the padding, wouldn't it be better to scan backwards from the end of the buffer?
Do we know what kind of data is present if we scan backwards? It is in theory more efficient but I have no idea what kind of byte values I should be watching for. Another idea would be to scan the last 5 bytes as we currently do but if we fail to find EOI then we scan all the encoded image data. What do you think?
(In reply to Mohammed Sameer from comment #7) > Do we know what kind of data is present if we scan backwards? It is in > theory more efficient but I have no idea what kind of byte values I should > be watching for. What do you mean? jifmux was made to deal with jpeg buffers, one jpeg frame per buffer. What other type of data could be present? Let's take a look at this bug from another angle, did you consider using jpegparse before the jifmux? It should be able to strip that extra padding data and seems like a more appropriate element to do it instead of having it in jifmux. > > Another idea would be to scan the last 5 bytes as we currently do but if we > fail to find EOI then we scan all the encoded image data. What do you think?
(In reply to Thiago Sousa Santos from comment #8) > (In reply to Mohammed Sameer from comment #7) > > Do we know what kind of data is present if we scan backwards? It is in > > theory more efficient but I have no idea what kind of byte values I should > > be watching for. > > What do you mean? jifmux was made to deal with jpeg buffers, one jpeg frame > per buffer. What other type of data could be present? jifmux is as good as its code is :) If I write the code to scan the stream backwards, I will have to make sure that I do it correctly. I don't know what markers or bytes will follow so the code I write might cause issues. This is what I meant. > Let's take a look at this bug from another angle, did you consider using > jpegparse before the jifmux? It should be able to strip that extra padding > data and seems like a more appropriate element to do it instead of having it > in jifmux. camerabin picks the needed elements. I don't know how to force jpegparse :-(
(In reply to Mohammed Sameer from comment #9) > (In reply to Thiago Sousa Santos from comment #8) > > (In reply to Mohammed Sameer from comment #7) > > > Do we know what kind of data is present if we scan backwards? It is in > > > theory more efficient but I have no idea what kind of byte values I should > > > be watching for. > > > > What do you mean? jifmux was made to deal with jpeg buffers, one jpeg frame > > per buffer. What other type of data could be present? > > jifmux is as good as its code is :) > > If I write the code to scan the stream backwards, I will have to make sure > that I do it correctly. I don't know what markers or bytes will follow so > the code I write might cause issues. This is what I meant. I'm no jpeg expert but you should be looking for a sequence of 0xFFDX, where X > 9 IIRC. Your code already does something similar. > > > Let's take a look at this bug from another angle, did you consider using > > jpegparse before the jifmux? It should be able to strip that extra padding > > data and seems like a more appropriate element to do it instead of having it > > in jifmux. > > camerabin picks the needed elements. I don't know how to force jpegparse :-( I guess it makes sense to modify camerabin to plug a jpegparse, then. Making sure that those extra padding stuff is removed.
(In reply to Thiago Sousa Santos from comment #10) > (In reply to Mohammed Sameer from comment #9) > > (In reply to Thiago Sousa Santos from comment #8) > > > (In reply to Mohammed Sameer from comment #7) > > > > Do we know what kind of data is present if we scan backwards? It is in > > > > theory more efficient but I have no idea what kind of byte values I should > > > > be watching for. > > > > > > What do you mean? jifmux was made to deal with jpeg buffers, one jpeg frame > > > per buffer. What other type of data could be present? > > > > jifmux is as good as its code is :) > > > > If I write the code to scan the stream backwards, I will have to make sure > > that I do it correctly. I don't know what markers or bytes will follow so > > the code I write might cause issues. This is what I meant. > > I'm no jpeg expert but you should be looking for a sequence of 0xFFDX, where > X > 9 IIRC. Your code already does something similar. I will try doing that then :) > > > > > Let's take a look at this bug from another angle, did you consider using > > > jpegparse before the jifmux? It should be able to strip that extra padding > > > data and seems like a more appropriate element to do it instead of having it > > > in jifmux. > > > > camerabin picks the needed elements. I don't know how to force jpegparse :-( > > I guess it makes sense to modify camerabin to plug a jpegparse, then. Making > sure that those extra padding stuff is removed. I tried jpegparse but it does not work: 0:00:00.023888024 30678 0x8b28180 WARN baseparse gstbaseparse.c:3569:gst_base_parse_loop:<jpegparse0> error: No valid frames found before end of stream ERROR: from element /GstPipeline:pipeline0/GstJpegParse:jpegparse0: No valid frames found before end of stream and the output file is empty.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/386.