diff mbox series

[FFmpeg-devel,1/2] lavc/cbs_av1: fill in ref_frame_sign_bias

Message ID Npbk3Pz--3-9@lynne.ee
State New
Headers show
Series [FFmpeg-devel,1/2] lavc/cbs_av1: fill in ref_frame_sign_bias | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Lynne Feb. 2, 2024, 2:57 a.m. UTC 
Needed for AV1.

Patch attached

Comments

Mark Thompson Feb. 4, 2024, 4:06 p.m. UTC | #1 
On 02/02/2024 02:57, Lynne wrote:
> Needed for AV1.
> 
 > From 81be215060a718fdc3d043847e8155ba56fcb431 Mon Sep 17 00:00:00 2001
 > From: Lynne <dev@lynne.ee>
 > Date: Fri, 2 Feb 2024 03:54:06 +0100
 > Subject: [PATCH 1/2] lavc/cbs_av1: fill in ref_frame_sign_bias
 >
 > Needed for AV1.
 > ---
 >  libavcodec/cbs_av1.h                 |  1 +
 >  libavcodec/cbs_av1_syntax_template.c | 10 ++++++++++
 >  2 files changed, 11 insertions(+)
 >
 > diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
 > index a5402f069d..cbb43ac810 100644
 > --- a/libavcodec/cbs_av1.h
 > +++ b/libavcodec/cbs_av1.h
 > @@ -198,6 +198,7 @@ typedef struct AV1RawFrameHeader {
 >      uint8_t refresh_frame_flags;
 >      uint8_t allow_intrabc;
 >      uint8_t ref_order_hint[AV1_NUM_REF_FRAMES];
 > +    uint8_t ref_frame_sign_bias[AV1_NUM_REF_FRAMES];

This isn't a syntax element so it shouldn't go in the syntax structure.  Put it in the context structure with other derived fields (the pointer is already available as priv where you want it).

 >      uint8_t frame_refs_short_signaling;
 >      uint8_t last_frame_idx;
 >      uint8_t golden_frame_idx;
 > diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
 > index 3be1f2d30f..00e9a6d030 100644
 > --- a/libavcodec/cbs_av1_syntax_template.c
 > +++ b/libavcodec/cbs_av1_syntax_template.c
 > @@ -1572,6 +1572,16 @@ static int FUNC(uncompressed_header)(CodedBitstreamContext *ctx, RWContext *rw,
 >      }
 >
 >      if (!frame_is_intra) {
 > +        for (i = 0; i < AV1_REFS_PER_FRAME; i++) {
 > +            if (seq->enable_order_hint) {
 > +                int idx = current->ref_frame_idx[i];
 > +                int hint = current->ref_order_hint[idx];
 > +                current->ref_frame_sign_bias[i] = cbs_av1_get_relative_dist(seq, hint,
 > +                                                                            priv->order_hint) > 0;
 > +            } else {
 > +                infer(ref_frame_sign_bias[i], 0);
 > +            }
 > +        }
 >          // Derive reference frame sign biases.
 >      }
 >
 > --
 > 2.43.0.381.gb435a96ce8

Please exactly follow the layout of the specification as far as possible, since that makes it much easier to assess whether it is correct.  It looks like the indexing is wrong?  (Note that LAST_FRAME == 1.)

Thanks,

- Mark
Lynne Feb. 6, 2024, 12:24 a.m. UTC | #2 
Feb 4, 2024, 17:06 by sw@jkqxz.net:

> On 02/02/2024 02:57, Lynne wrote:
>
>> Needed for AV1.
>>
>> From 81be215060a718fdc3d043847e8155ba56fcb431 Mon Sep 17 00:00:00 2001
>> From: Lynne <dev@lynne.ee>
>> Date: Fri, 2 Feb 2024 03:54:06 +0100
>> Subject: [PATCH 1/2] lavc/cbs_av1: fill in ref_frame_sign_bias
>>
>> Needed for AV1.
>> ---
>>  libavcodec/cbs_av1.h                 |  1 +
>>  libavcodec/cbs_av1_syntax_template.c | 10 ++++++++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
>> index a5402f069d..cbb43ac810 100644
>> --- a/libavcodec/cbs_av1.h
>> +++ b/libavcodec/cbs_av1.h
>> @@ -198,6 +198,7 @@ typedef struct AV1RawFrameHeader {
>>  uint8_t refresh_frame_flags;
>>  uint8_t allow_intrabc;
>>  uint8_t ref_order_hint[AV1_NUM_REF_FRAMES];
>> +    uint8_t ref_frame_sign_bias[AV1_NUM_REF_FRAMES];
>>
>
> This isn't a syntax element so it shouldn't go in the syntax structure.  Put it in the context structure with other derived fields (the pointer is already available as priv where you want it).
>
>> uint8_t frame_refs_short_signaling;
>>  uint8_t last_frame_idx;
>>  uint8_t golden_frame_idx;
>> diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
>> index 3be1f2d30f..00e9a6d030 100644
>> --- a/libavcodec/cbs_av1_syntax_template.c
>> +++ b/libavcodec/cbs_av1_syntax_template.c
>> @@ -1572,6 +1572,16 @@ static int FUNC(uncompressed_header)(CodedBitstreamContext *ctx, RWContext *rw,
>>  }
>>
>>  if (!frame_is_intra) {
>> +        for (i = 0; i < AV1_REFS_PER_FRAME; i++) {
>> +            if (seq->enable_order_hint) {
>> +                int idx = current->ref_frame_idx[i];
>> +                int hint = current->ref_order_hint[idx];
>> +                current->ref_frame_sign_bias[i] = cbs_av1_get_relative_dist(seq, hint,
>> +                                                                            priv->order_hint) > 0;
>> +            } else {
>> +                infer(ref_frame_sign_bias[i], 0);
>> +            }
>> +        }
>>  // Derive reference frame sign biases.
>>  }
>>
>> --
>> 2.43.0.381.gb435a96ce8
>>
>
> Please exactly follow the layout of the specification as far as possible, since that makes it much easier to assess whether it is correct.  It looks like the indexing is wrong?  (Note that LAST_FRAME == 1.)
>

It is the layout of the specifications. I just skip filling in the reorder hints
since they're not really needed (it's just a lookup after all).
diff mbox series

Patch

From 81be215060a718fdc3d043847e8155ba56fcb431 Mon Sep 17 00:00:00 2001
From: Lynne <dev@lynne.ee>
Date: Fri, 2 Feb 2024 03:54:06 +0100
Subject: [PATCH 1/2] lavc/cbs_av1: fill in ref_frame_sign_bias

Needed for AV1.
---
 libavcodec/cbs_av1.h                 |  1 +
 libavcodec/cbs_av1_syntax_template.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
index a5402f069d..cbb43ac810 100644
--- a/libavcodec/cbs_av1.h
+++ b/libavcodec/cbs_av1.h
@@ -198,6 +198,7 @@  typedef struct AV1RawFrameHeader {
     uint8_t refresh_frame_flags;
     uint8_t allow_intrabc;
     uint8_t ref_order_hint[AV1_NUM_REF_FRAMES];
+    uint8_t ref_frame_sign_bias[AV1_NUM_REF_FRAMES];
     uint8_t frame_refs_short_signaling;
     uint8_t last_frame_idx;
     uint8_t golden_frame_idx;
diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
index 3be1f2d30f..00e9a6d030 100644
--- a/libavcodec/cbs_av1_syntax_template.c
+++ b/libavcodec/cbs_av1_syntax_template.c
@@ -1572,6 +1572,16 @@  static int FUNC(uncompressed_header)(CodedBitstreamContext *ctx, RWContext *rw,
     }
 
     if (!frame_is_intra) {
+        for (i = 0; i < AV1_REFS_PER_FRAME; i++) {
+            if (seq->enable_order_hint) {
+                int idx = current->ref_frame_idx[i];
+                int hint = current->ref_order_hint[idx];
+                current->ref_frame_sign_bias[i] = cbs_av1_get_relative_dist(seq, hint,
+                                                                            priv->order_hint) > 0;
+            } else {
+                infer(ref_frame_sign_bias[i], 0);
+            }
+        }
         // Derive reference frame sign biases.
     }
 
-- 
2.43.0.381.gb435a96ce8