From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id AC91745F72 for ; Thu, 20 Apr 2023 09:50:39 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 7C18968BCFB; Thu, 20 Apr 2023 12:50:35 +0300 (EEST) Received: from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net [217.70.183.195]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id A82A668B2D8 for ; Thu, 20 Apr 2023 12:50:28 +0300 (EEST) Received: (Authenticated sender: michael@niedermayer.cc) by mail.gandi.net (Postfix) with ESMTPSA id 57F2760005 for ; Thu, 20 Apr 2023 09:50:27 +0000 (UTC) Date: Thu, 20 Apr 2023 11:50:26 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20230420095026.GW275832@pb2> References: <20230419181126.38662-1-leo.izen@gmail.com> <20230419181126.38662-2-leo.izen@gmail.com> <20230419185838.GS275832@pb2> <1e1d5c0d-7b06-3215-706c-edfc65a72801@gmail.com> <20230419203732.GT275832@pb2> <76d79ef5-b059-240c-7a95-8385d728b2e2@gmail.com> MIME-Version: 1.0 In-Reply-To: <76d79ef5-b059-240c-7a95-8385d728b2e2@gmail.com> Subject: Re: [FFmpeg-devel] [PATCH v3 1/3] avcodec/mjpegdec: fix non-subsampled RGB JPEGs X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: multipart/mixed; boundary="===============4796789021546951960==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============4796789021546951960== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="glDgwcODtpHomn63" Content-Disposition: inline --glDgwcODtpHomn63 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Apr 19, 2023 at 05:15:00PM -0400, Leo Izen wrote: > On 4/19/23 16:37, Michael Niedermayer wrote: > > On Wed, Apr 19, 2023 at 03:23:41PM -0400, Leo Izen wrote: > > > On 4/19/23 14:58, Michael Niedermayer wrote: > > > > On Wed, Apr 19, 2023 at 02:11:24PM -0400, Leo Izen wrote: > > > > > The change introduced in b18a9c29713abc3a1b081de3f320ab53a47120c6 > > > > > created a regression for non-subsampled progressive RGB jpegs. Th= is > > > > > should fix that. > > > > > --- > > > > > libavcodec/mjpegdec.c | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > >=20 > > > > > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c > > > > > index 01537d4774..1e3ddb72fb 100644 > > > > > --- a/libavcodec/mjpegdec.c > > > > > +++ b/libavcodec/mjpegdec.c > > > > > @@ -1698,7 +1698,8 @@ int ff_mjpeg_decode_sos(MJpegDecodeContext = *s, const uint8_t *mb_bitmask, > > > > > s->h_scount[i] =3D s->h_count[index]; > > > > > s->v_scount[i] =3D s->v_count[index]; > > > > > - if(nb_components =3D=3D 3 && s->nb_components =3D=3D 3 &= & s->avctx->pix_fmt =3D=3D AV_PIX_FMT_GBRP) > > > > > + if((nb_components =3D=3D 3 || nb_components =3D=3D 1) &&= s->nb_components =3D=3D 3 > > > > > + && s->avctx->pix_fmt =3D=3D AV_PIX_FMT_GBRP && != s->progressive) > > > > > index =3D (index+2)%3; > > > >=20 > > > > Why is progressive/!progressive special cased in all the new RGB co= de ? > > > >=20 > > >=20 > > > With progressive, I decode RGB in RGB-order, and then pivot it into > > > GBR-order, whereas baseline is just decoded directly into GBR-order. = If you > > > decode progressive directly in GBR-order the buffers will be the wron= g size > > > and it will overrun the subsampled buffer when filling it with a > > > non-subsampled one. See the allocation block on line 766 of mjpegdec.= c. This > > > depends on h_count and v_count, which cannot be changed or pivoted as= if you > > > do so, progressive JPEGs will fail to decode at all (invalid VLC entr= ies, > > > etc.) > > >=20 > > > Ideally, you'd just alloc them the right size, but s->component_index= [i] > > > won't refer to the right index for many progressive files, depending = on > > > whether the SOS marker has 1 or 3 components. If you have SOS markers= with > > > one component it will not properly pivot the colors. > > >=20 > > > Initially, I didn't have the checks and just always decoded in RGB or= der and > > > then pivoted, but that broke some baseline files like the ones in Trac > > > #4045. I used some casework so I could handle all files I tested with= this. > > >=20 > > > If anyone has any suggestions on how to make the casework more elegan= t I'm > > > all ears but this is the solution I found to work with every sample I > > > tested. > >=20 > > First i would document which array is in PIXFMT vs. JPEG order > > when anything is in 2 different orders at different points or for > > different cases thats probably not a good idea. > > But even if such bad cases exist, it should be documented > >=20 > > progressive is complicated because one could argue that it needs > > to be possible to both add pieces into the image and also to > > make these pieces immedeatly available to the user so some > > application could present to the user the image as it is > > "progressing". Ok we maybe dont care for that feature but its > > still not a bad way to look at the problem. > > I presume all jpeg streams can be decoded without too much problems > > if everything is in jpeg order. > > at the same time to present it we need planes to be scaled and > > ordered into a standard RGB/GBR/YUV form. > > I think these 2 worlds JPEG vs presentation should be more clearly > > seperated, > >=20 > > am i seeing the issue correctly or am i missing the problems here ? > >=20 > > thx >=20 > I could try to see if I can decode *every* image in JPEG order and then > pivot the planes from RGB to GBR order at the end, but it might take me a > bit more time to figure it out. I'll take a look at it this week. It would > be a more elegant solution and wouldn't require us to document which plan= es > go where in which places of the code. You might run into problems with user provided buffers if teh ordering is entirely done at the end. because when decoding lets say red it should be in the red plane. The user app might not expect its planes swapped [...] thx --=20 Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I have often repented speaking, but never of holding my tongue. -- Xenocrates --glDgwcODtpHomn63 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEIAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZEEK1gAKCRBhHseHBAsP q6+OAJ4h4oDS2MxgGgpP6fSTiMmzIy0+twCZARX1y8JmWVG5SBlNfZTDnEb/dFs= =nIDX -----END PGP SIGNATURE----- --glDgwcODtpHomn63-- --===============4796789021546951960== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". --===============4796789021546951960==--