diff mbox series

[FFmpeg-devel,2/2] avcodec/av1dec: derive skip_mode_frame_idx only where it's used

Message ID 20201114134332.885-2-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/2] avcodec/av1dec: derive coded_lossless only where it's used | 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

James Almer Nov. 14, 2020, 1:43 p.m. UTC 
No hwaccel other than nvdec cares about this field

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/av1dec.c    | 78 ------------------------------------------
 libavcodec/av1dec.h    |  2 --
 libavcodec/nvdec_av1.c | 72 +++++++++++++++++++++++++++++++++++---
 3 files changed, 68 insertions(+), 84 deletions(-)

Comments

Timo Rothenpieler Nov. 14, 2020, 3:01 p.m. UTC | #1 
I still strongly dislike this whole thing.
The same logic is ran essentially twice.

But I also really don't like putting all this stuff into the hwaccel.
That's way more logic than a hwaccel integration should have to do.
James Almer Nov. 14, 2020, 3:08 p.m. UTC | #2 
On 11/14/2020 12:01 PM, Timo Rothenpieler wrote:
> I still strongly dislike this whole thing.
> The same logic is ran essentially twice.
> 
> But I also really don't like putting all this stuff into the hwaccel.
> That's way more logic than a hwaccel integration should have to do.

Atm, only nvdec cares about these fields, and since all reference 
AV1RawFrameHeader are now available in hwaccel->start_frame(), it seemed 
proper to have it there.

Eventually, a software implementation will be added to av1dec, and this 
code will have to be moved back. But if you are really against this set, 
then I'll withdraw it.
Timo Rothenpieler Nov. 14, 2020, 5:32 p.m. UTC | #3 
On 14.11.2020 16:08, James Almer wrote:
> On 11/14/2020 12:01 PM, Timo Rothenpieler wrote:
>> I still strongly dislike this whole thing.
>> The same logic is ran essentially twice.
>>
>> But I also really don't like putting all this stuff into the hwaccel.
>> That's way more logic than a hwaccel integration should have to do.
> 
> Atm, only nvdec cares about these fields, and since all reference 
> AV1RawFrameHeader are now available in hwaccel->start_frame(), it seemed 
> proper to have it there.
> 
> Eventually, a software implementation will be added to av1dec, and this 
> code will have to be moved back. But if you are really against this set, 
> then I'll withdraw it.

An ideal solution would would only run this logic once, in the AV1 
decoder/parser.
Which right now is in a bit of temporary not-really-a-decoder limbo, so 
of course it's not perfect.

So if this will likely be needed for the software decoder in the future, 
leaving it there in the first place seems better?
Or would it get in the way for decoder development?
I don't want to hold up development on that front.
James Almer Nov. 14, 2020, 6:09 p.m. UTC | #4 
On 11/14/2020 2:32 PM, Timo Rothenpieler wrote:
> On 14.11.2020 16:08, James Almer wrote:
>> On 11/14/2020 12:01 PM, Timo Rothenpieler wrote:
>>> I still strongly dislike this whole thing.
>>> The same logic is ran essentially twice.
>>>
>>> But I also really don't like putting all this stuff into the hwaccel.
>>> That's way more logic than a hwaccel integration should have to do.
>>
>> Atm, only nvdec cares about these fields, and since all reference 
>> AV1RawFrameHeader are now available in hwaccel->start_frame(), it 
>> seemed proper to have it there.
>>
>> Eventually, a software implementation will be added to av1dec, and 
>> this code will have to be moved back. But if you are really against 
>> this set, then I'll withdraw it.
> 
> An ideal solution would would only run this logic once, in the AV1 
> decoder/parser.
> Which right now is in a bit of temporary not-really-a-decoder limbo, so 
> of course it's not perfect.
> 
> So if this will likely be needed for the software decoder in the future, 
> leaving it there in the first place seems better?
> Or would it get in the way for decoder development?

Best case scenario, it's used. Worst case scenario, it's replaced. So 
yes, in the long run it may be better to leave it here.
My intention with moving it to the only hwaccel uses it was to ensure 
it's not run for those that don't.

Since you want to keep nvdec_av1.c as clean as possible, I'm withdrawing 
this set.

> I don't want to hold up development on that front.
> _______________________________________________
> 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".
Mark Thompson Nov. 14, 2020, 6:56 p.m. UTC | #5 
On 14/11/2020 17:32, Timo Rothenpieler wrote:
> On 14.11.2020 16:08, James Almer wrote:
>> On 11/14/2020 12:01 PM, Timo Rothenpieler wrote:
>>> I still strongly dislike this whole thing.
>>> The same logic is ran essentially twice.

While it's applying the same method it is not doing the same thing, because different inputs are being used.

> An ideal solution would would only run this logic once, in the AV1 decoder/parser.

No it wouldn't.

You can't do it in the parser, because the parser doesn't know which frames have been successfully decoded.  The parser still has to run the standard-dictated logic, though, because it needs to know whether the skip_mode_present syntax element is present.

The other hwaccel implementations have made the decision to push it into the driver, which definitely knows which frames have decoded successfully - I'm pretty sure that's the right answer for a correct result.

Nvdec forcing us to guess in the wrapper layer which doesn't have complete information is a problem, but we can at least make a better guess than the parser did.

- Mark
diff mbox series

Patch

diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
index c8f9152451..7999ff6692 100644
--- a/libavcodec/av1dec.c
+++ b/libavcodec/av1dec.c
@@ -145,78 +145,6 @@  static void global_motion_params(AV1DecContext *s)
     }
 }
 
-static int get_relative_dist(const AV1RawSequenceHeader *seq,
-                             unsigned int a, unsigned int b)
-{
-    unsigned int diff = a - b;
-    unsigned int m = 1 << seq->order_hint_bits_minus_1;
-    return (diff & (m - 1)) - (diff & m);
-}
-
-static void skip_mode_params(AV1DecContext *s)
-{
-    const AV1RawFrameHeader *header = s->raw_frame_header;
-    const AV1RawSequenceHeader *seq = s->raw_seq;
-
-    int forward_idx,  backward_idx;
-    int forward_hint, backward_hint;
-    int second_forward_idx, second_forward_hint;
-    int ref_hint, dist, i;
-
-    if (!header->skip_mode_present)
-        return;
-
-    forward_idx  = -1;
-    backward_idx = -1;
-    for (i = 0; i < AV1_REFS_PER_FRAME; i++) {
-        ref_hint = s->ref[header->ref_frame_idx[i]].raw_frame_header->order_hint;
-        dist = get_relative_dist(seq, ref_hint, header->order_hint);
-        if (dist < 0) {
-            if (forward_idx < 0 ||
-                get_relative_dist(seq, ref_hint, forward_hint) > 0) {
-                forward_idx  = i;
-                forward_hint = ref_hint;
-            }
-        } else if (dist > 0) {
-            if (backward_idx < 0 ||
-                get_relative_dist(seq, ref_hint, backward_hint) < 0) {
-                backward_idx  = i;
-                backward_hint = ref_hint;
-            }
-        }
-    }
-
-    if (forward_idx < 0) {
-        return;
-    } else if (backward_idx >= 0) {
-        s->cur_frame.skip_mode_frame_idx[0] =
-            AV1_REF_FRAME_LAST + FFMIN(forward_idx, backward_idx);
-        s->cur_frame.skip_mode_frame_idx[1] =
-            AV1_REF_FRAME_LAST + FFMAX(forward_idx, backward_idx);
-        return;
-    }
-
-    second_forward_idx = -1;
-    for (i = 0; i < AV1_REFS_PER_FRAME; i++) {
-        ref_hint = s->ref[header->ref_frame_idx[i]].raw_frame_header->order_hint;
-        if (get_relative_dist(seq, ref_hint, forward_hint) < 0) {
-            if (second_forward_idx < 0 ||
-                get_relative_dist(seq, ref_hint, second_forward_hint) > 0) {
-                second_forward_idx  = i;
-                second_forward_hint = ref_hint;
-            }
-        }
-    }
-
-    if (second_forward_idx < 0)
-        return;
-
-    s->cur_frame.skip_mode_frame_idx[0] =
-        AV1_REF_FRAME_LAST + FFMIN(forward_idx, second_forward_idx);
-    s->cur_frame.skip_mode_frame_idx[1] =
-        AV1_REF_FRAME_LAST + FFMAX(forward_idx, second_forward_idx);
-}
-
 static int init_tile_data(AV1DecContext *s)
 
 {
@@ -415,8 +343,6 @@  static void av1_frame_unref(AVCodecContext *avctx, AV1Frame *f)
     av_buffer_unref(&f->header_ref);
     f->raw_frame_header = NULL;
     f->spatial_id = f->temporal_id = 0;
-    memset(f->skip_mode_frame_idx, 0,
-           2 * sizeof(uint8_t));
 }
 
 static int av1_frame_ref(AVCodecContext *avctx, AV1Frame *dst, const AV1Frame *src)
@@ -448,9 +374,6 @@  static int av1_frame_ref(AVCodecContext *avctx, AV1Frame *dst, const AV1Frame *s
     memcpy(dst->gm_params,
            src->gm_params,
            AV1_NUM_REF_FRAMES * 6 * sizeof(int32_t));
-    memcpy(dst->skip_mode_frame_idx,
-           src->skip_mode_frame_idx,
-           2 * sizeof(uint8_t));
 
     return 0;
 
@@ -727,7 +650,6 @@  static int get_current_frame(AVCodecContext *avctx)
     }
 
     global_motion_params(s);
-    skip_mode_params(s);
 
     return ret;
 }
diff --git a/libavcodec/av1dec.h b/libavcodec/av1dec.h
index bc6ae68d1a..7f07aa9f7a 100644
--- a/libavcodec/av1dec.h
+++ b/libavcodec/av1dec.h
@@ -44,8 +44,6 @@  typedef struct AV1Frame {
 
     uint8_t gm_type[AV1_NUM_REF_FRAMES];
     int32_t gm_params[AV1_NUM_REF_FRAMES][6];
-
-    uint8_t skip_mode_frame_idx[2];
 } AV1Frame;
 
 typedef struct TileGroupInfo {
diff --git a/libavcodec/nvdec_av1.c b/libavcodec/nvdec_av1.c
index 1a553902bc..d1161dd258 100644
--- a/libavcodec/nvdec_av1.c
+++ b/libavcodec/nvdec_av1.c
@@ -64,6 +64,68 @@  static int get_coded_lossless_param(const AV1DecContext *s)
     return 1;
 }
 
+static int get_relative_dist(const AV1RawSequenceHeader *seq,
+                             unsigned int a, unsigned int b)
+{
+    unsigned int diff = a - b;
+    unsigned int m = 1 << seq->order_hint_bits_minus_1;
+    return (diff & (m - 1)) - (diff & m);
+}
+
+static void skip_mode_params(const AV1DecContext *s, uint8_t *skip_mode_frame_idx)
+{
+    const AV1RawFrameHeader *frame_header = s->raw_frame_header;
+    const AV1RawSequenceHeader *seq = s->raw_seq;
+
+    int forward_idx = -1, backward_idx = -1;
+    int forward_hint, backward_hint;
+    int second_forward_idx, second_forward_hint;
+
+    for (int i = 0; i < AV1_REFS_PER_FRAME; i++) {
+        int ref_hint = s->ref[frame_header->ref_frame_idx[i]].raw_frame_header->order_hint;
+        int dist = get_relative_dist(seq, ref_hint, frame_header->order_hint);
+        if (dist < 0) {
+            if (forward_idx < 0 ||
+                get_relative_dist(seq, ref_hint, forward_hint) > 0) {
+                forward_idx  = i;
+                forward_hint = ref_hint;
+            }
+        } else if (dist > 0) {
+            if (backward_idx < 0 ||
+                get_relative_dist(seq, ref_hint, backward_hint) < 0) {
+                backward_idx  = i;
+                backward_hint = ref_hint;
+            }
+        }
+    }
+
+    if (forward_idx < 0) {
+        return;
+    } else if (backward_idx >= 0) {
+        skip_mode_frame_idx[0] = AV1_REF_FRAME_LAST + FFMIN(forward_idx, backward_idx);
+        skip_mode_frame_idx[1] = AV1_REF_FRAME_LAST + FFMAX(forward_idx, backward_idx);
+        return;
+    }
+
+    second_forward_idx = -1;
+    for (int i = 0; i < AV1_REFS_PER_FRAME; i++) {
+        int ref_hint = s->ref[frame_header->ref_frame_idx[i]].raw_frame_header->order_hint;
+        if (get_relative_dist(seq, ref_hint, forward_hint) < 0) {
+            if (second_forward_idx < 0 ||
+                get_relative_dist(seq, ref_hint, second_forward_hint) > 0) {
+                second_forward_idx  = i;
+                second_forward_hint = ref_hint;
+            }
+        }
+    }
+
+    if (second_forward_idx < 0)
+        return;
+
+    skip_mode_frame_idx[0] = AV1_REF_FRAME_LAST + FFMIN(forward_idx, second_forward_idx);
+    skip_mode_frame_idx[1] = AV1_REF_FRAME_LAST + FFMAX(forward_idx, second_forward_idx);
+}
+
 static int nvdec_av1_start_frame(AVCodecContext *avctx, const uint8_t *buffer, uint32_t size)
 {
     const AV1DecContext *s = avctx->priv_data;
@@ -78,6 +140,7 @@  static int nvdec_av1_start_frame(AVCodecContext *avctx, const uint8_t *buffer, u
     AVFrame *cur_frame = s->cur_frame.tf.f;
 
     unsigned char remap_lr_type[4] = { AV1_RESTORE_NONE, AV1_RESTORE_SWITCHABLE, AV1_RESTORE_WIENER, AV1_RESTORE_SGRPROJ };
+    uint8_t skip_mode_frame_idx[2] = { 0 };
 
     int ret, i, j;
 
@@ -90,6 +153,9 @@  static int nvdec_av1_start_frame(AVCodecContext *avctx, const uint8_t *buffer, u
     else
         fg_header = frame_header;
 
+    if (frame_header->skip_mode_present)
+        skip_mode_params(s, skip_mode_frame_idx);
+
     fdd = (FrameDecodeData*)cur_frame->private_ref->data;
     cf  = (NVDECFrame*)fdd->hwaccel_priv;
 
@@ -164,10 +230,8 @@  static int nvdec_av1_start_frame(AVCodecContext *avctx, const uint8_t *buffer, u
             .cdef_bits            = frame_header->cdef_bits,
 
             /* SkipModeFrames */
-            .SkipModeFrame0 = frame_header->skip_mode_present ?
-                              s->cur_frame.skip_mode_frame_idx[0] : 0,
-            .SkipModeFrame1 = frame_header->skip_mode_present ?
-                              s->cur_frame.skip_mode_frame_idx[1] : 0,
+            .SkipModeFrame0 = skip_mode_frame_idx[0],
+            .SkipModeFrame1 = skip_mode_frame_idx[1],
 
             /* QP Information */
             .base_qindex     = frame_header->base_q_idx,