diff mbox series

[FFmpeg-devel,2/3] lavc/ffv1: move damage handling code to decode_slice()

Message ID 20240722094322.28916-2-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel,1/3] lavc/ffv1dec: drop code handling AV_PIX_FMT_FLAG_PAL | expand

Checks

Context Check Description
andriy/configure_x86 warning Failed to apply patch

Commit Message

Anton Khirnov July 22, 2024, 9:43 a.m. UTC
There is no reason to delay it and this is a more natural place for
this code.
---
 libavcodec/ffv1dec.c | 53 ++++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 26 deletions(-)

Comments

Michael Niedermayer July 22, 2024, 9:14 p.m. UTC | #1
On Mon, Jul 22, 2024 at 11:43:21AM +0200, Anton Khirnov wrote:
> There is no reason to delay it and this is a more natural place for
> this code.

There is a reason.
By doing it later the surrounding pixels are available and one could
compute motion vectors from these surroundings and use all kinds of stuff
from motion compensation and optical flow and all that.

someone, i dont remember exactly who, (maybe you remember?)
said something about premature optimization is the root of all evil.
Here that actually applies. Moving the code up thwarts write better
error concealment. (and frankly we already have most of that EC code
it would just need a cleanup to free it from the 16x16 limitation)

thx

[...]
Anton Khirnov July 23, 2024, 6:52 a.m. UTC | #2
Quoting Michael Niedermayer (2024-07-22 23:14:04)
> On Mon, Jul 22, 2024 at 11:43:21AM +0200, Anton Khirnov wrote:
> > There is no reason to delay it and this is a more natural place for
> > this code.
> 
> There is a reason.
> By doing it later the surrounding pixels are available and one could
> compute motion vectors from these surroundings and use all kinds of stuff
> from motion compensation and optical flow and all that.
> 
> someone, i dont remember exactly who, (maybe you remember?)
> said something about premature optimization is the root of all evil.
> Here that actually applies. Moving the code up thwarts write better
> error concealment. (and frankly we already have most of that EC code
> it would just need a cleanup to free it from the 16x16 limitation)

This is not an optimization though.

But okay, I am dropping both original patch 28 and these 3 new patches.
Are you ok with the rest of the series?
Michael Niedermayer July 23, 2024, 8:14 p.m. UTC | #3
On Tue, Jul 23, 2024 at 08:52:58AM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2024-07-22 23:14:04)
> > On Mon, Jul 22, 2024 at 11:43:21AM +0200, Anton Khirnov wrote:
> > > There is no reason to delay it and this is a more natural place for
> > > this code.
> > 
> > There is a reason.
> > By doing it later the surrounding pixels are available and one could
> > compute motion vectors from these surroundings and use all kinds of stuff
> > from motion compensation and optical flow and all that.
> > 
> > someone, i dont remember exactly who, (maybe you remember?)
> > said something about premature optimization is the root of all evil.
> > Here that actually applies. Moving the code up thwarts write better
> > error concealment. (and frankly we already have most of that EC code
> > it would just need a cleanup to free it from the 16x16 limitation)
> 
> This is not an optimization though.
> 
> But okay, I am dropping both original patch 28 and these 3 new patches.
> Are you ok with the rest of the series?

The series looked mostly ok, is there somewhere i can take a quick look
at the current set ?

also i definitly want all bugfixes and speedups you did :)

thx

[...]
Anton Khirnov July 23, 2024, 9:02 p.m. UTC | #4
Quoting Michael Niedermayer (2024-07-23 22:14:19)
> On Tue, Jul 23, 2024 at 08:52:58AM +0200, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2024-07-22 23:14:04)
> > > On Mon, Jul 22, 2024 at 11:43:21AM +0200, Anton Khirnov wrote:
> > > > There is no reason to delay it and this is a more natural place for
> > > > this code.
> > > 
> > > There is a reason.
> > > By doing it later the surrounding pixels are available and one could
> > > compute motion vectors from these surroundings and use all kinds of stuff
> > > from motion compensation and optical flow and all that.
> > > 
> > > someone, i dont remember exactly who, (maybe you remember?)
> > > said something about premature optimization is the root of all evil.
> > > Here that actually applies. Moving the code up thwarts write better
> > > error concealment. (and frankly we already have most of that EC code
> > > it would just need a cleanup to free it from the 16x16 limitation)
> > 
> > This is not an optimization though.
> > 
> > But okay, I am dropping both original patch 28 and these 3 new patches.
> > Are you ok with the rest of the series?
> 
> The series looked mostly ok, is there somewhere i can take a quick look
> at the current set ?

I just pushed it to branch 'receive_frame' in my tree.
diff mbox series

Patch

diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
index aa2c35880e..5821a4156a 100644
--- a/libavcodec/ffv1dec.c
+++ b/libavcodec/ffv1dec.c
@@ -290,7 +290,7 @@  static int decode_slice(AVCodecContext *c, void *arg)
     if ((p->flags & AV_FRAME_FLAG_KEY) || sc->slice_reset_contexts) {
         ff_ffv1_clear_slice_state(f, sc);
     } else if (sc->slice_damaged) {
-        return AVERROR_INVALIDDATA;
+        goto handle_damage;
     }
 
     width  = sc->slice_width;
@@ -347,6 +347,32 @@  static int decode_slice(AVCodecContext *c, void *arg)
         }
     }
 
+handle_damage:
+    if (sc->slice_damaged && f->last_picture.f) {
+        const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(c->pix_fmt);
+        const uint8_t *src[4];
+        uint8_t *dst[4];
+
+        ff_progress_frame_await(&f->last_picture, si);
+
+        for (int j = 0; j < desc->nb_components; j++) {
+            int pixshift = desc->comp[j].depth > 8;
+            int sh = (j == 1 || j == 2) ? f->chroma_h_shift : 0;
+            int sv = (j == 1 || j == 2) ? f->chroma_v_shift : 0;
+            dst[j] = p->data[j] + p->linesize[j] *
+                     (sc->slice_y >> sv) + ((sc->slice_x >> sh) << pixshift);
+            src[j] = f->last_picture.f->data[j] + f->last_picture.f->linesize[j] *
+                     (sc->slice_y >> sv) + ((sc->slice_x >> sh) << pixshift);
+
+        }
+
+        av_image_copy(dst, p->linesize, src,
+                      f->last_picture.f->linesize,
+                      c->pix_fmt,
+                      sc->slice_width,
+                      sc->slice_height);
+    }
+
     ff_progress_frame_report(&f->picture, si);
 
     return 0;
@@ -964,31 +990,6 @@  static int decode_frame(AVCodecContext *avctx, AVFrame *rframe,
                    f->slice_count,
                    sizeof(*f->slices));
 
-    for (int i = f->slice_count - 1; i >= 0; i--) {
-        FFV1SliceContext *sc = &f->slices[i];
-        if (sc->slice_damaged && f->last_picture.f) {
-            const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt);
-            const uint8_t *src[4];
-            uint8_t *dst[4];
-            ff_progress_frame_await(&f->last_picture, INT_MAX);
-            for (int j = 0; j < desc->nb_components; j++) {
-                int pixshift = desc->comp[j].depth > 8;
-                int sh = (j == 1 || j == 2) ? f->chroma_h_shift : 0;
-                int sv = (j == 1 || j == 2) ? f->chroma_v_shift : 0;
-                dst[j] = p->data[j] + p->linesize[j] *
-                         (sc->slice_y >> sv) + ((sc->slice_x >> sh) << pixshift);
-                src[j] = f->last_picture.f->data[j] + f->last_picture.f->linesize[j] *
-                         (sc->slice_y >> sv) + ((sc->slice_x >> sh) << pixshift);
-
-            }
-
-            av_image_copy(dst, p->linesize, src,
-                          f->last_picture.f->linesize,
-                          avctx->pix_fmt,
-                          sc->slice_width,
-                          sc->slice_height);
-        }
-    }
     ff_progress_frame_report(&f->picture, INT_MAX);
 
     ff_progress_frame_unref(&f->last_picture);