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