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

[James Almer]

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.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>