diff mbox series

[FFmpeg-devel,07/14] avcodec/ffv1dec: Don't copy fields unnecessarily

Message ID HE1PR0301MB2154B3951FF3C5CC89A13BF28F449@HE1PR0301MB2154.eurprd03.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,01/14] avcodec/ffv1dec: Remove redundant writes, fix races | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt April 24, 2021, 11:14 a.m. UTC 
Copy slice-only fields only for the slicecontexts and not for the main
contexts in update_thread_context(). The converse is true for e.g.
key_frame_ok which is only used with the main context; when not doing
frame-threaded decoding it is actually only ever set for the main
context, so not setting it for the slice contexts when doing frame
threading is more consistent.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
I wonder whether one should use different structures for the main context
and the slice contexts.

 libavcodec/ffv1dec.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

Comments

Michael Niedermayer April 25, 2021, 12:27 p.m. UTC | #1 
On Sat, Apr 24, 2021 at 01:14:39PM +0200, Andreas Rheinhardt wrote:
> Copy slice-only fields only for the slicecontexts and not for the main
> contexts in update_thread_context(). The converse is true for e.g.
> key_frame_ok which is only used with the main context; when not doing
> frame-threaded decoding it is actually only ever set for the main
> context, so not setting it for the slice contexts when doing frame
> threading is more consistent.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---

> I wonder whether one should use different structures for the main context
> and the slice contexts.

i think either we should have seperate structs or we should make sure
all fields for which it makes sense are sensibly set. droping all code
for "unused" fields but leaving the fields there is a bit feeling like
placing mines one could step on.

That said, spliting the struct seems a reasonable choice assuming this
ends up looking nice and clean

thx

[...]
diff mbox series

Patch

diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
index ef908a24b7..45bfe21be5 100644
--- a/libavcodec/ffv1dec.c
+++ b/libavcodec/ffv1dec.c
@@ -993,8 +993,7 @@  static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame, AVPac
     return buf_size;
 }
 
-static void copy_fields(FFV1Context *fsdst, const FFV1Context *fssrc,
-                        const FFV1Context *fsrc)
+static void copy_fields(FFV1Context *fsdst, const FFV1Context *fsrc)
 {
     fsdst->version             = fsrc->version;
     fsdst->micro_version       = fsrc->micro_version;
@@ -1007,18 +1006,8 @@  static void copy_fields(FFV1Context *fsdst, const FFV1Context *fssrc,
     fsdst->colorspace          = fsrc->colorspace;
 
     fsdst->ec                  = fsrc->ec;
-    fsdst->intra               = fsrc->intra;
-    fsdst->slice_damaged       = fssrc->slice_damaged;
-    fsdst->key_frame_ok        = fsrc->key_frame_ok;
 
     fsdst->packed_at_lsb       = fsrc->packed_at_lsb;
-    fsdst->slice_count         = fsrc->slice_count;
-    if (fsrc->version<3){
-        fsdst->slice_x             = fssrc->slice_x;
-        fsdst->slice_y             = fssrc->slice_y;
-        fsdst->slice_width         = fssrc->slice_width;
-        fsdst->slice_height        = fssrc->slice_height;
-    }
 }
 
 #if HAVE_THREADS
@@ -1031,8 +1020,11 @@  static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src)
     if (dst == src)
         return 0;
 
-    copy_fields(fdst, fsrc, fsrc);
+    copy_fields(fdst, fsrc);
     fdst->use32bit     = fsrc->use32bit;
+    fdst->intra        = fsrc->intra;
+    fdst->key_frame_ok = fsrc->key_frame_ok;
+    fdst->slice_count  = fsrc->slice_count;
     memcpy(fdst->state_transition, fsrc->state_transition,
            sizeof(fdst->state_transition));
     memcpy(fdst->quant_table, fsrc->quant_table, sizeof(fsrc->quant_table));
@@ -1040,7 +1032,14 @@  static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src)
     for (i = 0; i < fdst->num_h_slices * fdst->num_v_slices; i++) {
         FFV1Context *fssrc = fsrc->slice_context[i];
         FFV1Context *fsdst = fdst->slice_context[i];
-        copy_fields(fsdst, fssrc, fsrc);
+        copy_fields(fsdst, fsrc);
+        fsdst->slice_damaged = fssrc->slice_damaged;
+        if (fsrc->version < 3) {
+            fsdst->slice_x      = fssrc->slice_x;
+            fsdst->slice_y      = fssrc->slice_y;
+            fsdst->slice_width  = fssrc->slice_width;
+            fsdst->slice_height = fssrc->slice_height;
+        }
     }
     av_assert0(!fdst->plane[0].state);
     av_assert0(!fdst->sample_buffer);