jpegdec: Outputting garbled colors
Submitted by Sebastian Dröge @slomo
Link to original bug (#783267)
Description
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=863663
The AVI file with JPEG in https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=863663#15 looks wrong when decoding with jpegdec, but is correct with avdec_mjpeg.
gst-launch-1.0 filesrc location=wave_anim.avi ! avidemux ! jpegdec ! glimagesink
gst-launch-1.0 filesrc location=wave_anim.avi ! avidemux ! jpegdec ! glimagesink
See also the attached screenshots to get an idea how it looks like.
Version: 1.12.0
Activity
@ndufresnesaid:I have the impression that CbCr is shifted. For those seeking for the screenshots (buried into the rather confusing debian log style bug):
The Good (mpv):
https://bugs.debian.org/cgi-bin/bugreport.cgi?att=2;bug=863663;filename=wave_anim_mpv_screenshot.png;msg=15The Ugly (jpegdec):
https://bugs.debian.org/cgi-bin/bugreport.cgi?att=3;bug=863663;filename=wave_anim_gst_screenshot.png;msg=15The Source:
https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=863663;filename=waveplot3d.tar.xz;msg=15@slomosaid:Take a look at the jpegdec code and it will be obvious to you. It currently e.g. always converts YUV formats to I420, with funny (i.e. wrong) chroma resampling internally. It could use the chroma resample from libgstvideo for odd formats, and otherwise output directly and let videoconvert worry about it.
It's quite clear what has to be done here, just needs someone to have the time for that.
@ao2uploaded an attachment:I got curious and I tried to understand what was going on in more details,
As pointed out the sub-sampling strategy of jpegdec makes it behave correctly only for 420-sampled input images.
However something more is going on with this specific test case: the problem is not only that the image is 422, it is also in a weird variant of it which looks especially broken because the sampling layout is not handled right by jpegdec when setting the output frame data pointers.
In particular the problematic data has frames encoded with a 422-sampling scheme of 2x2,1x2,1x2 while a normal 422 scheme would be 2x1,1x1,1x1.
It looks to me like jpegdec does not handle very well the cases where v_samp_factor[i] != h_samp_factor[i], not even in gst_jpeg_dec_decode_direct().
About using the original subsampling (e.g. Y42B, Y444) and gst_jpeg_dec_decode_direct(), would that work when the dimensions are not multiple of DCTSIZE?
After some hints from https://github.com/libjpeg-turbo/libjpeg-turbo/issues/92#issuecomment-236642411 I wrote something to reproduce the problem with a synthetic test: https://git.ao2.it/experiments/gstreamer.git/blob/HEAD:/shell/gst-test-jpegdec.sh
The script can also be run on the attached wave_anim_frame_000.jpg image which shows the artifacts more clearly, for instance compare: decoded_wave_anim_frame_000_NORMAL_422_jpegdec.png and decoded_wave_anim_frame_000_WEIRD_422_jpegdec.png, IIUC the first one would be the "expected" output of a 422 image represented as I420, even if it's a little funny looking.
wave_anim_frame_000.jpg is the first frame of the video from the Debian bug report, extracted without decoding it.
Ciao,
AntonioAttachment 366050, "The first frame of the video from the Debian bug report":
@ao2said:I took another look and come up with some changes which improves things in some cases.
One thing that I noticed is that the current code considers r_v = dec->cinfo.comp_info[0].v_samp_factor; but I found this not general enough when dealing with the problematic variant of 4:2:2 so I changed it to be an actual ratio between the sampling factors.
The proposed changes sacrifice one case (4:4:0 images in one of the two variant) in the name of better support for most used formats (4:2:2 and 4:4:4).
More info in the commit messages of the attached patches.
Ah, the patches are against the 1.12 branch, so they don't apply cleanly to master because of the interlaced MJPEG changes.
I don't think I'll be able to do some more work on this myself anytime soon, but I wanted to post something to get things moving.
Ciao,
Antonio
@ao2submitted a patch:r_v and r_h have the same value of the comp_info[0] sampling factor, so
the code should not check against this again but against comp_info[1]
and comp_info[2] instead.Patch 366499, "jpegdec: fix validating sampling factors":
jpegdec-fix-validating-sampling-factors.patch@ao2submitted a patch:When decoding jpeg images try to use native formats when they match the
image subsampling scheme, this allows to keep the chroma info intact
and pass it downstream, where it can be surely handled in better ways
than the current crude chroma subsampling approach.Get also rig of the horizontal resampling function which is not needed
anymore.The changes sacrifice one case which was working before but won't work
after, 4:4:0 images in one of the variants.The following feature matrix shows the situation before and after the changes for the most common sampling schemes:
4:1:0
Before: wrong chroma rendering
After: unsupported (lack of native format in GStreamer)4:1:1
Before: wrong chroma rendering
After: OK, colors a little smudged, but this is to be expected4:2:0
Before: OK
After: OK4:2:2
Before: chroma crudely subsampled, with off colors depending on the variant
After: OK, with full sampling for all variants4:4:0
Before: OK, for the most common variant, chroma wrong for the other variant
After: unsupported (lack of native format in GStreamer)4:4:4
Before: chroma crudely subsampled
After: OK, with full sampling)Patch 366500, "jpegdec: improve visual results for 4:2:2 and 4:4:4 sampled images":
jpegdec-improve-visual-results-for-422-and-444-sam.patchmentioned in issue #592 (closed)
@ao2 can you create a MR for this patch?
@slomo, the patch only applies cleanly and works on 1.12.x
There were some changes in 1.14 about interlacing which may break some assumptions, and I haven't found the time to look into it and update the patch.
I am still not sure when I will be able to do so.
#592 (closed) is another instance of this problem
mentioned in merge request !201 (merged)
