[FFmpeg-devel] [PATCH 2/2 v2] avcodec/mjpegdec: postpone calling ff_get_buffer() until the SOS marker

[James Almer]

On 5/1/2021 12:06 PM, James Almer wrote:
> On 5/1/2021 11:30 AM, James Almer wrote:
>> On 5/1/2021 11:17 AM, Michael Niedermayer wrote:
>>> On Sat, May 01, 2021 at 09:41:55AM -0300, James Almer wrote:
>>>> On 5/1/2021 4:01 AM, Michael Niedermayer wrote:
>>>>> On Thu, Apr 22, 2021 at 02:02:33PM -0300, James Almer wrote:
>>>>>> With JPEG-LS PAL8 samples, the JPEG-LS extension parameters 
>>>>>> signaled with
>>>>>> the LSE marker show up after SOF but before SOS. For those, the 
>>>>>> pixel format
>>>>>> chosen by get_format() in SOF is GRAY8, and then replaced by PAL8 
>>>>>> in LSE.
>>>>>> This has not been an issue given both pixel formats allocate the 
>>>>>> second data
>>>>>> plane for the palette, but after the upcoming soname bump, GRAY8 
>>>>>> will no longer
>>>>>> do that. This will result in segfauls when ff_jpegls_decode_lse() 
>>>>>> attempts to
>>>>>> write the palette on a buffer originally allocated as a GRAY8 one.
>>>>>>
>>>>>> Work around this by calling ff_get_buffer() after the actual pixel 
>>>>>> format is
>>>>>> known.
>>>>>
>>>>> What if the LSE occurs after the SOS ?
>>>>> What if there are 2 LSE ?
>>>>> It seems allowed by the specification
>>>>>
>>>>> "The LSE marker segment may be present where tables or 
>>>>> miscellaneous marker segments may appear. If tables specified
>>>>>    in this marker segment for a given table ID appear more than 
>>>>> once, each specification shall replace the previous
>>>>>    specification."
>>>>>
>>>>> Maybe iam missing something but a implemenattion of this would seem to
>>>>> require scanning through the image, finding all LSE and checking if 
>>>>> they all
>>>>> are compatible with the PAL8 format before allocating the image in SOF
>>>>>
>>>>> The implemenattion here which delays allocation slightly seems to make
>>>>> the code much more unstable allowing the frame configuration or 
>>>>> allocation
>>>>> to be partly done, double done, redone, without the matching partner.
>>>>> Also it does not seem to implement the related specification 
>>>>> completely.
>>>>
>>>> Well, it was a hack to replace a previous hack (allocate a gray 
>>>> buffer then
>>>> silently treat it as PAL8 once an LSE showed up). It's no wonder it 
>>>> may not
>>>> perfectly follow the spec and consider all potential cases.
>>>
>>> yes and i wouldnt mind but the new hack is leading to crashes ...
>>> and the fixes to the crashes all in all start looking a bit ugly and 
>>> iam not
>>> sure if any more remain. So id like to take a bit a step back here 
>>> and see
>>> if there isnt a easier/cleaner solution.
>>>
>>>
>>>>
>>>>>
>>>>> A complete implemenattion may reqire RGB24 or 48 to be selected in 
>>>>> some cases
>>>>> like when each scan uses its own palette, which unless iam missing 
>>>>> something
>>>>> seems allowed.
>>>>> But then maybe iam missing something, in which case please correct me!
>>>>
>>>> You're probably not wrong, since unlike me you actually know this 
>>>> code and
>>>> format. I tried to workaround an issue and it seemed to work and not 
>>>> break
>>>> any file anyone here could test.
>>>>
>>>> If you think it's not good enough then just revert it and maybe 
>>>> allocate the
>>>> data[1] pointer for the palette with av_buffer_alloc(), 
>>>> get_buffer2() custom
>>>> implementations be damned. But i don't think the previous code even 
>>>> did half
>>>> the things you listed above.
>>>
>>> how can i reproduce the issue you where trying to fix with this patch ?
>>
>> It's the crash you reported in 
>> https://lists.ffmpeg.org/pipermail/ffmpeg-devel/2021-April/279350.html
>>
>> The explanation of why it started crashing is in c8197f73e6. 
>> Basically, since GRAY8 no longer unnecessarily allocates a fixed 
>> palette, the old hack of changing pix_fmt from GRAY8 to PAL8 after 
>> having called ff_get_buffer() with the former, editing the palette 
>> plane buffer, and have things magically work is not possible anymore, 
>> because no palette buffer was ever allocated. ff_get_buffer() is meant 
>> to be called once the actual pix_fmt is known to correctly allocate 
>> all plane buffers.
> 
> Another possible solution instead of my suggestion above (revert and 
> then manually allocate a palette plane buffer, which can potentially 
> piss off custom get_buffer2() implementations), is to revert and then 
> initially set the pix_fmt in ff_mjpeg_decode_sof() to PAL8 instead of 
> GRAY8 for jpegls, and replace it with the latter if no LSE is ever 
> parsed (or if one is parsed and it doesn't contain a palette).
> It's also hacky, basically the inverse of what it used to be, but at 
> least it ensures the palette plane is allocated by get_buffer2(), which 
> will ultimately be ignored and freed.

Which is to say, revert fb5e2d7112 c8197f73e6 and try the something like 
  the following:

> $ git diff
> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> index 7c66ff8637..70e674758d 100644
> --- a/libavcodec/mjpegdec.c
> +++ b/libavcodec/mjpegdec.c
> @@ -681,10 +681,8 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
>              } else if (s->nb_components != 1) {
>                  av_log(s->avctx, AV_LOG_ERROR, "Unsupported number of components %d\n", s->nb_components);
>                  return AVERROR_PATCHWELCOME;
> -            } else if (s->palette_index && s->bits <= 8)
> +            } else if (s->bits <= 8)
>                  s->avctx->pix_fmt = AV_PIX_FMT_PAL8;
> -            else if (s->bits <= 8)
> -                s->avctx->pix_fmt = AV_PIX_FMT_GRAY8;
>              else
>                  s->avctx->pix_fmt = AV_PIX_FMT_GRAY16;
>          }
> @@ -733,6 +731,13 @@ int ff_mjpeg_decode_sof(MJpegDecodeContext *s)
>          for (i = 0; i < 4; i++)
>              s->linesize[i] = s->picture_ptr->linesize[i] << s->interlaced;
> 
> +        if (s->ls && !s->palette_index && s->bits <= 8) {
> +            // The LSE marker will change this back to PAL8 if there's a palette.
> +            // By calling ff_get_buffer() with PAL8 pix_fmt we ensured a plane for
> +            // it was allocated.
> +            s->avctx->pix_fmt = s->picture_ptr->format = AV_PIX_FMT_GRAY8;
> +            avpriv_set_systematic_pal2((uint32_t *)s->picture_ptr->data[1], s->picture_ptr->format);
> +        }
>          ff_dlog(s->avctx, "%d %d %d %d %d %d\n",
>                  s->width, s->height, s->linesize[0], s->linesize[1],
>                  s->interlaced, s->avctx->height);

fate-jpegls passes, so even if also hacky, it may be better than 
postponing the call to ff_get_buffer().