[FFmpeg-devel] [PATCH v3 1/3] avcodec/mjpegdec: fix non-subsampled RGB JPEGs

[Michael Niedermayer]

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. This
> > > should fix that.
> > > ---
> > >   libavcodec/mjpegdec.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > 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]  = s->h_count[index];
> > >           s->v_scount[i]  = s->v_count[index];
> > > -        if(nb_components == 3 && s->nb_components == 3 && s->avctx->pix_fmt == AV_PIX_FMT_GBRP)
> > > +        if((nb_components == 3 || nb_components == 1) && s->nb_components == 3
> > > +                && s->avctx->pix_fmt == AV_PIX_FMT_GBRP && !s->progressive)
> > >               index = (index+2)%3;
> > 
> > Why is progressive/!progressive special cased in all the new RGB code ?
> > 
> 
> 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 wrong 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 entries,
> etc.)
> 
> 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.
> 
> Initially, I didn't have the checks and just always decoded in RGB order 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.
> 
> If anyone has any suggestions on how to make the casework more elegant I'm
> all ears but this is the solution I found to work with every sample I
> tested.

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

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,

am i seeing the issue correctly or am i missing the problems here ?

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20230419/57523d3b/attachment.sig>