[FFmpeg-devel] [PATCH 07/18] avcodec/vp8: Pass mb_y explicitly

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Avoids atomic stores and loads and is a prerequisite for
removing all atomic synchronizations for VP7.
Notice that removing the explicit atomic_store() in
vp78_decode_mb_row_sliced() does not negatively affect
parallelism during slice-threading, because no check_thread_pos()
ever waits for an (mb_x, mb_y) pair with mb_x == 0
(which this atomic store signalled).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Btw: The code in update_pos looks fishy to me; namely
the part that tries to avoid the broadcast. Consider the scenario
in which the other threads (prev and next, A and B) are not waiting
when the current thread C checks their wait_mb_pos. Then
one of the other threads reads C's thread_mb_pos and notices that
it needs to wait for an update from C. It therefore locks C's mutex,
stores its wait_mb_pos, checks C's thread_mb_pos again (still reading
the old value in this scenario) and waits via pthread_cond_wait().

Then C updates its thread_mb_pos, but because C uses outdated values
for A and B's wait_mb_pos, it never signals a broadcast. Who will
then wake up the waiting thread?

This should be fixable by moving the loads after C's update
of thread_mb_pos: In case C's read of A's wait_mb_pos value happens
before A updates it, then C's update of its thread_mb_pos
happens before A updates its wait_mb_pos and A will therefore read
C's updated value of thread_mb_pos its atomic_load while holding
C's lock (and will therefore never call pthread_cond_wait()).
In case C's read of A's wait_mb_pos value happens after A updates it,
C will emit its broadcast, waking A which reads the updated value
and stops.

 libavcodec/vp8.c | 30 +++++++++++++++---------------
 libavcodec/vp8.h |  4 ++--
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/libavcodec/vp8.c b/libavcodec/vp8.c
index c259f3588c..5ecb9b07e5 100644
--- a/libavcodec/vp8.c
+++ b/libavcodec/vp8.c
@@ -2389,11 +2389,11 @@ static int vp8_decode_mv_mb_modes(AVCodecContext *avctx, VP8Frame *cur_frame,
 #endif
 
 static av_always_inline int decode_mb_row_no_filter(AVCodecContext *avctx, void *tdata,
-                                        int jobnr, int threadnr, int is_vp7)
+                                                    int jobnr, int threadnr, int mb_y,
+                                                    int is_vp7)
 {
     VP8Context *s = avctx->priv_data;
     VP8ThreadData *prev_td, *next_td, *td = &s->thread_data[threadnr];
-    int mb_y = atomic_load(&td->thread_mb_pos) >> 16;
     int mb_x, mb_xy = mb_y * s->mb_width;
     int num_jobs = s->num_jobs;
     const VP8Frame *prev_frame = s->prev_frame;
@@ -2518,23 +2518,24 @@ static av_always_inline int decode_mb_row_no_filter(AVCodecContext *avctx, void
 }
 
 static int vp7_decode_mb_row_no_filter(AVCodecContext *avctx, void *tdata,
-                                        int jobnr, int threadnr)
+                                       int jobnr, int threadnr, int mb_y)
 {
-    return decode_mb_row_no_filter(avctx, tdata, jobnr, threadnr, 1);
+    return decode_mb_row_no_filter(avctx, tdata, jobnr, threadnr, mb_y, 1);
 }
 
 static int vp8_decode_mb_row_no_filter(AVCodecContext *avctx, void *tdata,
-                                        int jobnr, int threadnr)
+                                       int jobnr, int threadnr, int mb_y)
 {
-    return decode_mb_row_no_filter(avctx, tdata, jobnr, threadnr, 0);
+    return decode_mb_row_no_filter(avctx, tdata, jobnr, threadnr, mb_y, 0);
 }
 
 static av_always_inline void filter_mb_row(AVCodecContext *avctx, void *tdata,
-                              int jobnr, int threadnr, int is_vp7)
+                                           int jobnr, int threadnr, int mb_y,
+                                           int is_vp7)
 {
     VP8Context *s = avctx->priv_data;
     VP8ThreadData *td = &s->thread_data[threadnr];
-    int mb_x, mb_y = atomic_load(&td->thread_mb_pos) >> 16, num_jobs = s->num_jobs;
+    int mb_x, num_jobs = s->num_jobs;
     AVFrame *curframe = s->curframe->tf.f;
     VP8Macroblock *mb;
     VP8ThreadData *prev_td, *next_td;
@@ -2589,15 +2590,15 @@ static av_always_inline void filter_mb_row(AVCodecContext *avctx, void *tdata,
 }
 
 static void vp7_filter_mb_row(AVCodecContext *avctx, void *tdata,
-                              int jobnr, int threadnr)
+                              int jobnr, int threadnr, int mb_y)
 {
-    filter_mb_row(avctx, tdata, jobnr, threadnr, 1);
+    filter_mb_row(avctx, tdata, jobnr, threadnr, mb_y, 1);
 }
 
 static void vp8_filter_mb_row(AVCodecContext *avctx, void *tdata,
-                              int jobnr, int threadnr)
+                              int jobnr, int threadnr, int mb_y)
 {
-    filter_mb_row(avctx, tdata, jobnr, threadnr, 0);
+    filter_mb_row(avctx, tdata, jobnr, threadnr, mb_y, 0);
 }
 
 static av_always_inline
@@ -2615,14 +2616,13 @@ int vp78_decode_mb_row_sliced(AVCodecContext *avctx, void *tdata, int jobnr,
     td->mv_bounds.mv_min.y   = -MARGIN - 64 * threadnr;
     td->mv_bounds.mv_max.y   = ((s->mb_height - 1) << 6) + MARGIN - 64 * threadnr;
     for (mb_y = jobnr; mb_y < s->mb_height; mb_y += num_jobs) {
-        atomic_store(&td->thread_mb_pos, mb_y << 16);
-        ret = s->decode_mb_row_no_filter(avctx, tdata, jobnr, threadnr);
+        ret = s->decode_mb_row_no_filter(avctx, tdata, jobnr, threadnr, mb_y);
         if (ret < 0) {
             update_pos(td, s->mb_height, INT_MAX & 0xFFFF);
             return ret;
         }
         if (s->deblock_filter)
-            s->filter_mb_row(avctx, tdata, jobnr, threadnr);
+            s->filter_mb_row(avctx, tdata, jobnr, threadnr, mb_y);
         update_pos(td, mb_y, INT_MAX & 0xFFFF);
 
         td->mv_bounds.mv_min.y -= 64 * num_jobs;
diff --git a/libavcodec/vp8.h b/libavcodec/vp8.h
index 30aeb4cb06..ed79bc79c1 100644
--- a/libavcodec/vp8.h
+++ b/libavcodec/vp8.h
@@ -330,8 +330,8 @@ typedef struct VP8Context {
      */
     int mb_layout;
 
-    int (*decode_mb_row_no_filter)(AVCodecContext *avctx, void *tdata, int jobnr, int threadnr);
-    void (*filter_mb_row)(AVCodecContext *avctx, void *tdata, int jobnr, int threadnr);
+    int (*decode_mb_row_no_filter)(AVCodecContext *avctx, void *tdata, int jobnr, int threadnr, int mb_y);
+    void (*filter_mb_row)(AVCodecContext *avctx, void *tdata, int jobnr, int threadnr, int mb_y);
 
     int vp7;
 
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".