Possibly because for historical reasons jpegdec downsamples everything to I420 in the crudest possible way.

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=15

The Ugly (jpegdec):
https://bugs.debian.org/cgi-bin/bugreport.cgi?att=3;bug=863663;filename=wave_anim_gst_screenshot.png;msg=15

The Source:
https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=863663;filename=waveplot3d.tar.xz;msg=15

FWIW, this definitely goes through our funny chroma resampling implementation

(In reply to Sebastian Dröge (slomo) from comment #3)
> FWIW, this definitely goes through our funny chroma resampling implementation

Do you have some more context ? Isn't the output format something videoconvert can handle ?

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.

Created attachment 366050 [details]
The first frame of the video from the Debian bug report

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,
   Antonio

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

Created attachment 366499 [details] [review]
jpegdec: fix validating sampling factors

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.

Created attachment 366500 [details] [review]
jpegdec: improve visual results for 4:2:2 and 4:4:4 sampled images

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 expected

4:2:0
  Before: OK
  After: OK

4:2:2
  Before: chroma crudely subsampled, with off colors depending on the variant
  After: OK, with full sampling for all variants

4: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)

-- 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-good/issues/376.