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

[James Almer]

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.

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