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@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
