diff mbox series

[FFmpeg-devel] Make execute() and execute2() return FFMIN() of thread return codes

Message ID 7c2bdda6042d62e21fff91407393e77e7e7fedc2.camel@acc.umu.se
State New
Headers show
Series [FFmpeg-devel] Make execute() and execute2() return FFMIN() of thread return codes | expand

Checks

Context Check Description
andriy/commit_msg_x86 warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
yinshiyou/configure_loongarch64 warning Failed to apply patch
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/commit_msg_armv7_RPi4 warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
andriy/make_armv7_RPi4 success Make finished
andriy/make_fate_armv7_RPi4 success Make fate finished

Commit Message

Tomas Härdin June 30, 2022, 12:42 p.m. UTC 
Hi

Previous version of this patch failed fate-fic-avi with
THREAD_TYPE=slice THREADS=2 due to thread_execute() always returning 0.
Fixed in this version.

The fic sample appears to indeed be broken. Some packets are truncated,
including one zero-length packet.

/Tomas

Comments

Anton Khirnov July 2, 2022, 9:43 a.m. UTC | #1 
Quoting Tomas Härdin (2022-06-30 14:42:42)
> Hi
> 
> Previous version of this patch failed fate-fic-avi with
> THREAD_TYPE=slice THREADS=2 due to thread_execute() always returning 0.
> Fixed in this version.
> 
> The fic sample appears to indeed be broken. Some packets are truncated,
> including one zero-length packet.

maybe mention this fact for posterity above the relevant test in
tests/fate/scren.mak

> diff --git a/libavutil/slicethread.h b/libavutil/slicethread.h
> index f6f6f302c4..5c8f197932 100644
> --- a/libavutil/slicethread.h
> +++ b/libavutil/slicethread.h
> @@ -31,8 +31,8 @@ typedef struct AVSliceThread AVSliceThread;
>   * @return return number of threads or negative AVERROR on failure
>   */
>  int avpriv_slicethread_create(AVSliceThread **pctx, void *priv,
> -                              void (*worker_func)(void *priv, int jobnr, int threadnr, int nb_jobs, int nb_threads),
> -                              void (*main_func)(void *priv),
> +                              int (*worker_func)(void *priv, int jobnr, int threadnr, int nb_jobs, int nb_threads),
> +                              int (*main_func)(void *priv),

This is an ABI break.
Tomas Härdin July 4, 2022, 10:46 a.m. UTC | #2 
lör 2022-07-02 klockan 11:43 +0200 skrev Anton Khirnov:
> Quoting Tomas Härdin (2022-06-30 14:42:42)
> > Hi
> > 
> > Previous version of this patch failed fate-fic-avi with
> > THREAD_TYPE=slice THREADS=2 due to thread_execute() always
> > returning 0.
> > Fixed in this version.
> > 
> > The fic sample appears to indeed be broken. Some packets are
> > truncated,
> > including one zero-length packet.
> 
> maybe mention this fact for posterity above the relevant test in
> tests/fate/scren.mak

Sure

> 
> > diff --git a/libavutil/slicethread.h b/libavutil/slicethread.h
> > index f6f6f302c4..5c8f197932 100644
> > --- a/libavutil/slicethread.h
> > +++ b/libavutil/slicethread.h
> > @@ -31,8 +31,8 @@ typedef struct AVSliceThread AVSliceThread;
> >   * @return return number of threads or negative AVERROR on failure
> >   */
> >  int avpriv_slicethread_create(AVSliceThread **pctx, void *priv,
> > -                              void (*worker_func)(void *priv, int
> > jobnr, int threadnr, int nb_jobs, int nb_threads),
> > -                              void (*main_func)(void *priv),
> > +                              int (*worker_func)(void *priv, int
> > jobnr, int threadnr, int nb_jobs, int nb_threads),
> > +                              int (*main_func)(void *priv),
> 
> This is an ABI break.
> 

You're right. I was under the impression that avpriv functions could be
changed more freely but obviously not when they're shared between
libraries.

This could be worked around with a new function called say
avpriv_slicethread_create2() and a minor bump. Another approach is to
av_fast_malloc() an array for rets when none is given. The number of
allocations would thereby be quite limited.

I'd like to see all users of execute() and execute2() start checking
their return values, and making that easier to do is another step
toward more correctness of the code

/Tomas
Anton Khirnov July 5, 2022, 4:42 p.m. UTC | #3 
Quoting Tomas Härdin (2022-07-04 12:46:04)
> lör 2022-07-02 klockan 11:43 +0200 skrev Anton Khirnov:
> > Quoting Tomas Härdin (2022-06-30 14:42:42)
> > > Hi
> > > 
> > > Previous version of this patch failed fate-fic-avi with
> > > THREAD_TYPE=slice THREADS=2 due to thread_execute() always
> > > returning 0.
> > > Fixed in this version.
> > > 
> > > The fic sample appears to indeed be broken. Some packets are
> > > truncated,
> > > including one zero-length packet.
> > 
> > maybe mention this fact for posterity above the relevant test in
> > tests/fate/scren.mak
> 
> Sure
> 
> > 
> > > diff --git a/libavutil/slicethread.h b/libavutil/slicethread.h
> > > index f6f6f302c4..5c8f197932 100644
> > > --- a/libavutil/slicethread.h
> > > +++ b/libavutil/slicethread.h
> > > @@ -31,8 +31,8 @@ typedef struct AVSliceThread AVSliceThread;
> > >   * @return return number of threads or negative AVERROR on failure
> > >   */
> > >  int avpriv_slicethread_create(AVSliceThread **pctx, void *priv,
> > > -                              void (*worker_func)(void *priv, int
> > > jobnr, int threadnr, int nb_jobs, int nb_threads),
> > > -                              void (*main_func)(void *priv),
> > > +                              int (*worker_func)(void *priv, int
> > > jobnr, int threadnr, int nb_jobs, int nb_threads),
> > > +                              int (*main_func)(void *priv),
> > 
> > This is an ABI break.
> > 
> 
> You're right. I was under the impression that avpriv functions could be
> changed more freely but obviously not when they're shared between
> libraries.
> 
> This could be worked around with a new function called say
> avpriv_slicethread_create2() and a minor bump. Another approach is to
> av_fast_malloc() an array for rets when none is given. The number of
> allocations would thereby be quite limited.

Adding a new function is ok I suppose.

Though longer-term I'd like to see that API cleaned up and made properly
public.

> I'd like to see all users of execute() and execute2() start checking
> their return values, and making that easier to do is another step
> toward more correctness of the code

I'm all in favor of that.
diff mbox series

Patch

From 872354ff70f6e880d77e8bddd28bfa7bc6582ab2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Thu, 16 Jun 2022 12:16:44 +0200
Subject: [PATCH] Make execute() and execute2() return FFMIN() of thread return
 codes

At the moment only fic.c actually checks return code of execute() hence the change to its FATE reference
---
 libavcodec/avcodec.c          | 10 ++++++----
 libavcodec/pthread_slice.c    | 12 ++++++------
 libavfilter/pthread.c         |  3 ++-
 libavutil/slicethread.c       | 37 ++++++++++++++++++++++-------------
 libavutil/slicethread.h       |  6 +++---
 libswscale/swscale.c          |  5 +++--
 libswscale/swscale_internal.h |  4 ++--
 tests/ref/fate/fic-avi        | 30 ++++++++++++----------------
 8 files changed, 58 insertions(+), 49 deletions(-)

diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
index 5f6e71a39e..49f0fd06fb 100644
--- a/libavcodec/avcodec.c
+++ b/libavcodec/avcodec.c
@@ -44,28 +44,30 @@ 
 
 int avcodec_default_execute(AVCodecContext *c, int (*func)(AVCodecContext *c2, void *arg2), void *arg, int *ret, int count, int size)
 {
-    int i;
+    int i, rr = 0;
 
     for (i = 0; i < count; i++) {
         int r = func(c, (char *)arg + i * size);
+        rr = FFMIN(rr, r);
         if (ret)
             ret[i] = r;
     }
     emms_c();
-    return 0;
+    return rr;
 }
 
 int avcodec_default_execute2(AVCodecContext *c, int (*func)(AVCodecContext *c2, void *arg2, int jobnr, int threadnr), void *arg, int *ret, int count)
 {
-    int i;
+    int i, rr = 0;
 
     for (i = 0; i < count; i++) {
         int r = func(c, arg, i, 0);
+        rr = FFMIN(rr, r);
         if (ret)
             ret[i] = r;
     }
     emms_c();
-    return 0;
+    return rr;
 }
 
 static AVMutex codec_mutex = AV_MUTEX_INITIALIZER;
diff --git a/libavcodec/pthread_slice.c b/libavcodec/pthread_slice.c
index 0ad1965a22..e2903846eb 100644
--- a/libavcodec/pthread_slice.c
+++ b/libavcodec/pthread_slice.c
@@ -57,13 +57,13 @@  typedef struct SliceThreadContext {
     pthread_mutex_t *progress_mutex;
 } SliceThreadContext;
 
-static void main_function(void *priv) {
+static int main_function(void *priv) {
     AVCodecContext *avctx = priv;
     SliceThreadContext *c = avctx->internal->thread_ctx;
-    c->mainfunc(avctx);
+    return c->mainfunc(avctx);
 }
 
-static void worker_func(void *priv, int jobnr, int threadnr, int nb_jobs, int nb_threads)
+static int worker_func(void *priv, int jobnr, int threadnr, int nb_jobs, int nb_threads)
 {
     AVCodecContext *avctx = priv;
     SliceThreadContext *c = avctx->internal->thread_ctx;
@@ -73,6 +73,7 @@  static void worker_func(void *priv, int jobnr, int threadnr, int nb_jobs, int nb
                   : c->func2(avctx, c->args, jobnr, threadnr);
     if (c->rets)
         c->rets[jobnr] = ret;
+    return ret;
 }
 
 void ff_slice_thread_free(AVCodecContext *avctx)
@@ -108,8 +109,7 @@  static int thread_execute(AVCodecContext *avctx, action_func* func, void *arg, i
     c->func = func;
     c->rets = ret;
 
-    avpriv_slicethread_execute(c->thread, job_count, !!c->mainfunc  );
-    return 0;
+    return avpriv_slicethread_execute(c->thread, job_count, !!c->mainfunc  );
 }
 
 static int thread_execute2(AVCodecContext *avctx, action_func2* func2, void *arg, int *ret, int job_count)
@@ -131,7 +131,7 @@  int ff_slice_thread_init(AVCodecContext *avctx)
 {
     SliceThreadContext *c;
     int thread_count = avctx->thread_count;
-    void (*mainfunc)(void *);
+    int (*mainfunc)(void *);
 
     // We cannot do this in the encoder init as the threads are created before
     if (av_codec_is_encoder(avctx->codec) &&
diff --git a/libavfilter/pthread.c b/libavfilter/pthread.c
index 1a063d3cc0..8cec278be0 100644
--- a/libavfilter/pthread.c
+++ b/libavfilter/pthread.c
@@ -43,12 +43,13 @@  typedef struct ThreadContext {
     int   *rets;
 } ThreadContext;
 
-static void worker_func(void *priv, int jobnr, int threadnr, int nb_jobs, int nb_threads)
+static int worker_func(void *priv, int jobnr, int threadnr, int nb_jobs, int nb_threads)
 {
     ThreadContext *c = priv;
     int ret = c->func(c->ctx, c->arg, jobnr, nb_jobs);
     if (c->rets)
         c->rets[jobnr] = ret;
+    return ret;
 }
 
 static void slice_thread_uninit(ThreadContext *c)
diff --git a/libavutil/slicethread.c b/libavutil/slicethread.c
index ea1c9c8311..2e78d32ab8 100644
--- a/libavutil/slicethread.c
+++ b/libavutil/slicethread.c
@@ -32,6 +32,7 @@  typedef struct WorkerContext {
     pthread_cond_t  cond;
     pthread_t       thread;
     int             done;
+    int             ret;
 } WorkerContext;
 
 struct AVSliceThread {
@@ -48,11 +49,11 @@  struct AVSliceThread {
     int             finished;
 
     void            *priv;
-    void            (*worker_func)(void *priv, int jobnr, int threadnr, int nb_jobs, int nb_threads);
-    void            (*main_func)(void *priv);
+    int             (*worker_func)(void *priv, int jobnr, int threadnr, int nb_jobs, int nb_threads);
+    int             (*main_func)(void *priv);
 };
 
-static int run_jobs(AVSliceThread *ctx)
+static int run_jobs(AVSliceThread *ctx, int *ret_out)
 {
     unsigned nb_jobs    = ctx->nb_jobs;
     unsigned nb_active_threads = ctx->nb_active_threads;
@@ -60,7 +61,8 @@  static int run_jobs(AVSliceThread *ctx)
     unsigned current_job  = first_job;
 
     do {
-        ctx->worker_func(ctx->priv, current_job, first_job, nb_jobs, nb_active_threads);
+        int ret = ctx->worker_func(ctx->priv, current_job, first_job, nb_jobs, nb_active_threads);
+        *ret_out = FFMIN(*ret_out, ret);
     } while ((current_job = atomic_fetch_add_explicit(&ctx->current_job, 1, memory_order_acq_rel)) < nb_jobs);
 
     return current_job == nb_jobs + nb_active_threads - 1;
@@ -84,7 +86,7 @@  static void *attribute_align_arg thread_worker(void *v)
             return NULL;
         }
 
-        if (run_jobs(ctx)) {
+        if (run_jobs(ctx, &w->ret)) {
             pthread_mutex_lock(&ctx->done_mutex);
             ctx->done = 1;
             pthread_cond_signal(&ctx->done_cond);
@@ -94,8 +96,8 @@  static void *attribute_align_arg thread_worker(void *v)
 }
 
 int avpriv_slicethread_create(AVSliceThread **pctx, void *priv,
-                              void (*worker_func)(void *priv, int jobnr, int threadnr, int nb_jobs, int nb_threads),
-                              void (*main_func)(void *priv),
+                              int (*worker_func)(void *priv, int jobnr, int threadnr, int nb_jobs, int nb_threads),
+                              int (*main_func)(void *priv),
                               int nb_threads)
 {
     AVSliceThread *ctx;
@@ -163,9 +165,9 @@  int avpriv_slicethread_create(AVSliceThread **pctx, void *priv,
     return nb_threads;
 }
 
-void avpriv_slicethread_execute(AVSliceThread *ctx, int nb_jobs, int execute_main)
+int avpriv_slicethread_execute(AVSliceThread *ctx, int nb_jobs, int execute_main)
 {
-    int nb_workers, i, is_last = 0;
+    int nb_workers, i, is_last = 0, ret = 0;
 
     av_assert0(nb_jobs > 0);
     ctx->nb_jobs           = nb_jobs;
@@ -180,14 +182,15 @@  void avpriv_slicethread_execute(AVSliceThread *ctx, int nb_jobs, int execute_mai
         WorkerContext *w = &ctx->workers[i];
         pthread_mutex_lock(&w->mutex);
         w->done = 0;
+        w->ret = 0;
         pthread_cond_signal(&w->cond);
         pthread_mutex_unlock(&w->mutex);
     }
 
     if (ctx->main_func && execute_main)
-        ctx->main_func(ctx->priv);
+        ret = ctx->main_func(ctx->priv);
     else
-        is_last = run_jobs(ctx);
+        is_last = run_jobs(ctx, &ret);
 
     if (!is_last) {
         pthread_mutex_lock(&ctx->done_mutex);
@@ -196,6 +199,11 @@  void avpriv_slicethread_execute(AVSliceThread *ctx, int nb_jobs, int execute_mai
         ctx->done = 0;
         pthread_mutex_unlock(&ctx->done_mutex);
     }
+
+    for (i = 0; i < nb_workers; i++)
+        ret = FFMIN(ret, ctx->workers[i].ret);
+
+    return ret;
 }
 
 void avpriv_slicethread_free(AVSliceThread **pctx)
@@ -236,17 +244,18 @@  void avpriv_slicethread_free(AVSliceThread **pctx)
 #else /* HAVE_PTHREADS || HAVE_W32THREADS || HAVE_OS32THREADS */
 
 int avpriv_slicethread_create(AVSliceThread **pctx, void *priv,
-                              void (*worker_func)(void *priv, int jobnr, int threadnr, int nb_jobs, int nb_threads),
-                              void (*main_func)(void *priv),
+                              int (*worker_func)(void *priv, int jobnr, int threadnr, int nb_jobs, int nb_threads),
+                              int (*main_func)(void *priv),
                               int nb_threads)
 {
     *pctx = NULL;
     return AVERROR(ENOSYS);
 }
 
-void avpriv_slicethread_execute(AVSliceThread *ctx, int nb_jobs, int execute_main)
+int avpriv_slicethread_execute(AVSliceThread *ctx, int nb_jobs, int execute_main)
 {
     av_assert0(0);
+    return AVERROR(ENOSYS);
 }
 
 void avpriv_slicethread_free(AVSliceThread **pctx)
diff --git a/libavutil/slicethread.h b/libavutil/slicethread.h
index f6f6f302c4..5c8f197932 100644
--- a/libavutil/slicethread.h
+++ b/libavutil/slicethread.h
@@ -31,8 +31,8 @@  typedef struct AVSliceThread AVSliceThread;
  * @return return number of threads or negative AVERROR on failure
  */
 int avpriv_slicethread_create(AVSliceThread **pctx, void *priv,
-                              void (*worker_func)(void *priv, int jobnr, int threadnr, int nb_jobs, int nb_threads),
-                              void (*main_func)(void *priv),
+                              int (*worker_func)(void *priv, int jobnr, int threadnr, int nb_jobs, int nb_threads),
+                              int (*main_func)(void *priv),
                               int nb_threads);
 
 /**
@@ -41,7 +41,7 @@  int avpriv_slicethread_create(AVSliceThread **pctx, void *priv,
  * @param nb_jobs number of jobs, must be > 0
  * @param execute_main also execute main_func
  */
-void avpriv_slicethread_execute(AVSliceThread *ctx, int nb_jobs, int execute_main);
+int avpriv_slicethread_execute(AVSliceThread *ctx, int nb_jobs, int execute_main);
 
 /**
  * Destroy slice threading context.
diff --git a/libswscale/swscale.c b/libswscale/swscale.c
index 7b40f49da4..2f9a0b5a7c 100644
--- a/libswscale/swscale.c
+++ b/libswscale/swscale.c
@@ -1211,8 +1211,8 @@  int attribute_align_arg sws_scale(struct SwsContext *c,
                           dst, dstStride, 0, c->dstH);
 }
 
-void ff_sws_slice_worker(void *priv, int jobnr, int threadnr,
-                         int nb_jobs, int nb_threads)
+int ff_sws_slice_worker(void *priv, int jobnr, int threadnr,
+                        int nb_jobs, int nb_threads)
 {
     SwsContext *parent = priv;
     SwsContext      *c = parent->slice_ctx[threadnr];
@@ -1241,4 +1241,5 @@  void ff_sws_slice_worker(void *priv, int jobnr, int threadnr,
     }
 
     parent->slice_err[threadnr] = err;
+    return err;
 }
diff --git a/libswscale/swscale_internal.h b/libswscale/swscale_internal.h
index e118b54457..eab3e26331 100644
--- a/libswscale/swscale_internal.h
+++ b/libswscale/swscale_internal.h
@@ -1135,8 +1135,8 @@  void ff_init_vscale_pfn(SwsContext *c, yuv2planar1_fn yuv2plane1, yuv2planarX_fn
     yuv2interleavedX_fn yuv2nv12cX, yuv2packed1_fn yuv2packed1, yuv2packed2_fn yuv2packed2,
     yuv2packedX_fn yuv2packedX, yuv2anyX_fn yuv2anyX, int use_mmx);
 
-void ff_sws_slice_worker(void *priv, int jobnr, int threadnr,
-                         int nb_jobs, int nb_threads);
+int ff_sws_slice_worker(void *priv, int jobnr, int threadnr,
+                        int nb_jobs, int nb_threads);
 
 //number of extra lines to process
 #define MAX_LINES_AHEAD 4
diff --git a/tests/ref/fate/fic-avi b/tests/ref/fate/fic-avi
index df55789d54..4546f230b1 100644
--- a/tests/ref/fate/fic-avi
+++ b/tests/ref/fate/fic-avi
@@ -76,19 +76,18 @@ 
 0,         70,         70,        1,  1566720, 0x40f7d39a
 0,         71,         71,        1,  1566720, 0x40f7d39a
 0,         72,         72,        1,  1566720, 0x40f7d39a
-0,         73,         73,        1,  1566720, 0xa7d6e25f
-0,         74,         74,        1,  1566720, 0xa7d6e25f
-0,         75,         75,        1,  1566720, 0xa7d6e25f
-0,         76,         76,        1,  1566720, 0xa7d6e25f
-0,         77,         77,        1,  1566720, 0xa7d6e25f
-0,         78,         78,        1,  1566720, 0xa7d6e25f
-0,         79,         79,        1,  1566720, 0xa7d6e25f
-0,         80,         80,        1,  1566720, 0xa7d6e25f
-0,         81,         81,        1,  1566720, 0xa7d6e25f
-0,         82,         82,        1,  1566720, 0xa7d6e25f
-0,         83,         83,        1,  1566720, 0xa7d6e25f
-0,         84,         84,        1,  1566720, 0xa7d6e25f
-0,         85,         85,        1,  1566720, 0xa7d6e25f
+0,         74,         74,        1,  1566720, 0x40f7d39a
+0,         75,         75,        1,  1566720, 0x40f7d39a
+0,         76,         76,        1,  1566720, 0x40f7d39a
+0,         77,         77,        1,  1566720, 0x40f7d39a
+0,         78,         78,        1,  1566720, 0x40f7d39a
+0,         79,         79,        1,  1566720, 0x40f7d39a
+0,         80,         80,        1,  1566720, 0x40f7d39a
+0,         81,         81,        1,  1566720, 0x40f7d39a
+0,         82,         82,        1,  1566720, 0x40f7d39a
+0,         83,         83,        1,  1566720, 0x40f7d39a
+0,         84,         84,        1,  1566720, 0x40f7d39a
+0,         85,         85,        1,  1566720, 0x40f7d39a
 0,         86,         86,        1,  1566720, 0xa7d6e25f
 0,         87,         87,        1,  1566720, 0xa7d6e25f
 0,         88,         88,        1,  1566720, 0xa7d6e25f
@@ -104,7 +103,6 @@ 
 0,         98,         98,        1,  1566720, 0xa7d6e25f
 0,         99,         99,        1,  1566720, 0xa7d6e25f
 0,        100,        100,        1,  1566720, 0xeaf8d207
-0,        101,        101,        1,  1566720, 0x6724983e
 0,        102,        102,        1,  1566720, 0x0e95d209
 0,        103,        103,        1,  1566720, 0x0e95d209
 0,        104,        104,        1,  1566720, 0x0e95d209
@@ -121,6 +119,4 @@ 
 0,        115,        115,        1,  1566720, 0xfe83b964
 0,        116,        116,        1,  1566720, 0xfe83b964
 0,        117,        117,        1,  1566720, 0xfe83b964
-0,        118,        118,        1,  1566720, 0x25dc30a6
-0,        119,        119,        1,  1566720, 0x25dc30a6
-0,        120,        120,        1,  1566720, 0x25dc30a6
+0,        119,        119,        1,  1566720, 0xfe83b964
-- 
2.30.2