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

[James Almer]

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.

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

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