diff mbox series

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

Message ID 2ab7238154c98289a19f655c87a20c339561b3e8.camel@acc.umu.se
State New
Headers show
Series [FFmpeg-devel,01/13] lavc/jpeg2000dec: Finer granularity threading | expand

Checks

Context Check Description
andriy/configure_x86 warning Failed to apply patch

Commit Message

Tomas Härdin June 14, 2022, 2:41 p.m. UTC

Comments

Michael Niedermayer June 14, 2022, 9:11 p.m. UTC | #1 
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@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

thx

[...]
Tomas Härdin June 15, 2022, 1:11 p.m. UTC | #2 
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@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..

/Tomas
Michael Niedermayer June 15, 2022, 9:05 p.m. UTC | #3 
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@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

thx


[...]
Tomas Härdin June 16, 2022, 9:28 a.m. UTC | #4 
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@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
diff mbox series

Patch

From 080ebdc9bad130098bff575f9ce690b8a522c9f7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Mon, 13 Jun 2022 15:09:17 +0200
Subject: [PATCH 05/13] lavc/jpeg2000dec: Thread init_tile()

---
 libavcodec/jpeg2000dec.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/libavcodec/jpeg2000dec.c b/libavcodec/jpeg2000dec.c
index 9344630c6f..ef5167c29e 100644
--- a/libavcodec/jpeg2000dec.c
+++ b/libavcodec/jpeg2000dec.c
@@ -1015,12 +1015,19 @@  static int get_ppt(Jpeg2000DecoderContext *s, int n)
     return 0;
 }
 
-static int init_tile(Jpeg2000DecoderContext *s, int tileno)
+static int init_tile(AVCodecContext *avctx, void *td,
+                     int jobnr, int threadnr)
 {
-    int compno;
-    int tilex = tileno % s->numXtiles;
-    int tiley = tileno / s->numXtiles;
-    Jpeg2000Tile *tile = s->tile + tileno;
+    Jpeg2000DecoderContext *s   = avctx->priv_data;
+    int tileno                  = jobnr / s->ncomponents;
+    int tilex                   = tileno % s->numXtiles;
+    int tiley                   = tileno / s->numXtiles;
+    int compno                  = jobnr % s->ncomponents;
+    Jpeg2000Tile *tile          = s->tile + tileno;
+    Jpeg2000Component *comp     = tile->comp + compno;
+    Jpeg2000CodingStyle *codsty = tile->codsty + compno;
+    Jpeg2000QuantStyle  *qntsty = tile->qntsty + compno;
+    int ret; // global bandno
 
     if (!tile->comp)
         return AVERROR(ENOMEM);
@@ -1030,12 +1037,6 @@  static int init_tile(Jpeg2000DecoderContext *s, int tileno)
     tile->coord[1][0] = av_clip(tiley       * (int64_t)s->tile_height + s->tile_offset_y, s->image_offset_y, s->height);
     tile->coord[1][1] = av_clip((tiley + 1) * (int64_t)s->tile_height + s->tile_offset_y, s->image_offset_y, s->height);
 
-    for (compno = 0; compno < s->ncomponents; compno++) {
-        Jpeg2000Component *comp = tile->comp + compno;
-        Jpeg2000CodingStyle *codsty = tile->codsty + compno;
-        Jpeg2000QuantStyle  *qntsty = tile->qntsty + compno;
-        int ret; // global bandno
-
         comp->coord_o[0][0] = tile->coord[0][0];
         comp->coord_o[0][1] = tile->coord[0][1];
         comp->coord_o[1][0] = tile->coord[1][0];
@@ -1059,7 +1060,7 @@  static int init_tile(Jpeg2000DecoderContext *s, int tileno)
                                              s->cbps[compno], s->cdx[compno],
                                              s->cdy[compno], s->avctx, s->slices))
             return ret;
-    }
+
     return 0;
 }
 
@@ -2367,9 +2368,6 @@  static int jpeg2000_read_bitstream_packets(Jpeg2000DecoderContext *s)
     for (tileno = 0; tileno < s->numXtiles * s->numYtiles; tileno++) {
         Jpeg2000Tile *tile = s->tile + tileno;
 
-        if ((ret = init_tile(s, tileno)) < 0)
-            return ret;
-
         if ((ret = jpeg2000_decode_packets(s, tile)) < 0)
             return ret;
     }
@@ -2656,6 +2654,8 @@  static int jpeg2000_decode_frame(AVCodecContext *avctx, AVFrame *picture,
     picture->key_frame = 1;
     s->slices = avctx->active_thread_type == FF_THREAD_SLICE ? avctx->thread_count : 1;
 
+    avctx->execute2(avctx, init_tile, NULL, NULL, s->numXtiles * s->numYtiles * s->ncomponents);
+
     if (ret = jpeg2000_read_bitstream_packets(s))
         goto end;
 
-- 
2.30.2