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

[Leo Izen]

On 4/20/23 05:50, Michael Niedermayer wrote:
> 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. 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.
> 
> 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
> 

As far as I'm aware, I should be able to just rotate the AVFrame->data 
pointers and AVFrame->linesize values, as we don't promise these are 
ordered in the same order as the AVBufferRefs.