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

[Leo Izen]

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. 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

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 planes go where in which places of the code.

- Leo Izen