[FFmpeg-devel] [PATCH 05/13] lavc/jpeg2000dec: Thread init_tile()

[Tomas Härdin]

ons 2022-06-15 klockan 23:05 +0200 skrev Michael Niedermayer:
> On Wed, Jun 15, 2022 at 03:11:34PM +0200, Tomas Härdin wrote:
> > tis 2022-06-14 klockan 23:11 +0200 skrev Michael Niedermayer:
> > > On Tue, Jun 14, 2022 at 04:41:14PM +0200, Tomas Härdin wrote:
> > > > 
> > > 
> > > >  jpeg2000dec.c |   30 +++++++++++++++---------------
> > > >  1 file changed, 15 insertions(+), 15 deletions(-)
> > > > 6fa2fbf99afee36ee73459863df0527a72663f43  0005-lavc-
> > > > jpeg2000dec-
> > > > Thread-init_tile.patch
> > > > From 080ebdc9bad130098bff575f9ce690b8a522c9f7 Mon Sep 17
> > > > 00:00:00
> > > > 2001
> > > > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git at haerdin.se>
> > > > Date: Mon, 13 Jun 2022 15:09:17 +0200
> > > > Subject: [PATCH 05/13] lavc/jpeg2000dec: Thread init_tile()
> > > 
> > > Causes segfaults
> > > 
> > > [jpeg2000 @ 0x2cf53380] End mismatch 149
> > > [jpeg2000 @ 0x2cf53380] ==1439== Thread 6:
> > > ==1439== Invalid read of size 4
> > > ==1439==    at 0x9771F0: jpeg2000_mct_write_frame (in ffmpeg_g)
> > > ==1439==    by 0x78BA6F: avcodec_default_execute2 (in ffmpeg_g)
> > > ==1439==    by 0x97C0BB: jpeg2000_decode_frame (in ffmpeg_g)
> > > ==1439==    by 0xA90F72: frame_worker_thread (in ffmpeg_g)
> > > ==1439==    by 0x54046DA: start_thread (pthread_create.c:463)
> > > ==1439==    by 0xF8F261E: clone (clone.S:95)
> > > 
> > > i will send you the sample privatly
> > 
> > This is because init_tile() fails. I had assumed errors were
> > handled in
> > some way like longjmp since the function already called execute2()
> > but
> > it seems the threading doesn't do any kind of magic for this.
> > 
> > Can we have execute2() return some kind of error code when one or
> > more
> > jobs fail? Either say FFMIN() of all errors or negative jobnr that
> > failed? This would save on having to allocate an array for errors
> > when
> > we don't really care which exact jobs failed..
> 
> one could return a struct with error code, index and number of failed
> ones or something. But then maybe just atomically setting some error
> flag
> and leaving the API would be fine too.
> Iam fine with either

It currently has the ability to return error codes in an array, but
only hevcdec.c makes use of that and it does so poorly. It just adds up
all return codes. What I propose is returning FFMIN() of all return
codes seen. This allows bailing out on error without having to bother
allocating an array for returns that no one is handling in any sensible
manner anyway.

The API states that execute() and execute2() may return non-zero and
that users should act on that so it seems safe to make it actually do
so in case of error. I think we could also av_log() errors + jobnr

I'm working on something for this at the moment. Will post a separate
patchset for that.

/Tomas