diff mbox series

[FFmpeg-devel,1/3] avcodec/cbs_av1: infer loop filter delta parameters from reference frames

Message ID 20201021001126.14044-1-jamrial@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel,1/3] avcodec/cbs_av1: infer loop filter delta parameters from reference frames
Related show

Checks

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

Commit Message

James Almer Oct. 21, 2020, 12:11 a.m. UTC
Partially implements of setup_past_independence() and load_previous().
These ensures they are always set, even if the values were not coded
in the input bitstream and will not be coded in the output bitstream.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/cbs_av1.h                 |  3 +++
 libavcodec/cbs_av1_syntax_template.c | 40 +++++++++++++++++++++++++---
 2 files changed, 39 insertions(+), 4 deletions(-)

Comments

Mark Thompson Oct. 27, 2020, 8:38 p.m. UTC | #1
On 21/10/2020 01:11, James Almer wrote:
> Partially implements of setup_past_independence() and load_previous().
> These ensures they are always set, even if the values were not coded
> in the input bitstream and will not be coded in the output bitstream.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>   libavcodec/cbs_av1.h                 |  3 +++
>   libavcodec/cbs_av1_syntax_template.c | 40 +++++++++++++++++++++++++---
>   2 files changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
> index 7a0c08c596..97aeee9795 100644
> --- a/libavcodec/cbs_av1.h
> +++ b/libavcodec/cbs_av1.h
> @@ -413,6 +413,9 @@ typedef struct AV1ReferenceFrameState {
>       int subsampling_y;  // RefSubsamplingY
>       int bit_depth;      // RefBitDepth
>       int order_hint;     // RefOrderHint
> +
> +    int8_t  loop_filter_ref_deltas[AV1_TOTAL_REFS_PER_FRAME];
> +    int8_t  loop_filter_mode_deltas[2];
>   } AV1ReferenceFrameState;
>   
>   typedef struct CodedBitstreamAV1Context {
> diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
> index bcaa334134..4edf4fd47c 100644
> --- a/libavcodec/cbs_av1_syntax_template.c
> +++ b/libavcodec/cbs_av1_syntax_template.c
> @@ -837,6 +837,9 @@ static int FUNC(loop_filter_params)(CodedBitstreamContext *ctx, RWContext *rw,
>                                       AV1RawFrameHeader *current)
>   {
>       CodedBitstreamAV1Context *priv = ctx->priv_data;
> +    static const int8_t default_loop_filter_ref_deltas[AV1_TOTAL_REFS_PER_FRAME] =
> +        { 1, 0, 0, 0, -1, 0, -1, -1 };
> +    static const int8_t default_loop_filter_mode_deltas[2] = { 0 };

I realise it's the same, but the single zero there looks like an error so I think put two of them.

>       int i, err;
>   
>       if (priv->coded_lossless || current->allow_intrabc) {
> @@ -870,19 +873,44 @@ static int FUNC(loop_filter_params)(CodedBitstreamContext *ctx, RWContext *rw,
>   
>       flag(loop_filter_delta_enabled);
>       if (current->loop_filter_delta_enabled) {
> +        const int8_t *loop_filter_ref_deltas, *loop_filter_mode_deltas;

Maybe call these ref_* to make the below a little clearer?  (foo[n] is inferred from ref_foo[n].)

> +
> +        if (current->primary_ref_frame == AV1_PRIMARY_REF_NONE) {
> +            loop_filter_ref_deltas = default_loop_filter_ref_deltas;
> +            loop_filter_mode_deltas = default_loop_filter_mode_deltas;
> +        } else {
> +            loop_filter_ref_deltas =
> +                priv->ref[current->ref_frame_idx[current->primary_ref_frame]].loop_filter_ref_deltas;
> +            loop_filter_mode_deltas =
> +                priv->ref[current->ref_frame_idx[current->primary_ref_frame]].loop_filter_mode_deltas;
> +        }
> +
>           flag(loop_filter_delta_update);
> -        if (current->loop_filter_delta_update) {
>               for (i = 0; i < AV1_TOTAL_REFS_PER_FRAME; i++) {
> -                flags(update_ref_delta[i], 1, i);
> +                if (current->loop_filter_delta_update)
> +                    flags(update_ref_delta[i], 1, i);
> +                else
> +                    infer(update_ref_delta[i], 0);
>                   if (current->update_ref_delta[i])
>                       sus(1 + 6, loop_filter_ref_deltas[i], 1, i);
> +                else
> +                    infer(loop_filter_ref_deltas[i], loop_filter_ref_deltas[i]);
>               }
>               for (i = 0; i < 2; i++) {
> -                flags(update_mode_delta[i], 1, i);
> +                if (current->loop_filter_delta_update)
> +                    flags(update_mode_delta[i], 1, i);
> +                else
> +                    infer(update_mode_delta[i], 0);
>                   if (current->update_mode_delta[i])
>                       sus(1 + 6, loop_filter_mode_deltas[i], 1, i);
> +                else
> +                    infer(loop_filter_mode_deltas[i], loop_filter_mode_deltas[i]);
>               }
> -        }
> +    } else {
> +        for (i = 0; i < AV1_TOTAL_REFS_PER_FRAME; i++)
> +            infer(loop_filter_ref_deltas[i], default_loop_filter_ref_deltas[i]);
> +        for (i = 0; i < 2; i++)
> +            infer(loop_filter_mode_deltas[i], default_loop_filter_mode_deltas[i]);
>       }
>   
>       return 0;
> @@ -1613,6 +1641,10 @@ update_refs:
>                   .bit_depth      = priv->bit_depth,
>                   .order_hint     = priv->order_hint,
>               };
> +            memcpy(priv->ref[i].loop_filter_ref_deltas, current->loop_filter_ref_deltas,
> +                   sizeof(current->loop_filter_ref_deltas));
> +            memcpy(priv->ref[i].loop_filter_mode_deltas, current->loop_filter_mode_deltas,
> +                   sizeof(current->loop_filter_mode_deltas));
>           }
>       }
>   
> 

Looks sensible.

Thanks,

- Mark
James Almer Oct. 27, 2020, 8:53 p.m. UTC | #2
On 10/27/2020 5:38 PM, Mark Thompson wrote:
> On 21/10/2020 01:11, James Almer wrote:
>> Partially implements of setup_past_independence() and load_previous().
>> These ensures they are always set, even if the values were not coded
>> in the input bitstream and will not be coded in the output bitstream.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>   libavcodec/cbs_av1.h                 |  3 +++
>>   libavcodec/cbs_av1_syntax_template.c | 40 +++++++++++++++++++++++++---
>>   2 files changed, 39 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
>> index 7a0c08c596..97aeee9795 100644
>> --- a/libavcodec/cbs_av1.h
>> +++ b/libavcodec/cbs_av1.h
>> @@ -413,6 +413,9 @@ typedef struct AV1ReferenceFrameState {
>>       int subsampling_y;  // RefSubsamplingY
>>       int bit_depth;      // RefBitDepth
>>       int order_hint;     // RefOrderHint
>> +
>> +    int8_t  loop_filter_ref_deltas[AV1_TOTAL_REFS_PER_FRAME];
>> +    int8_t  loop_filter_mode_deltas[2];
>>   } AV1ReferenceFrameState;
>>     typedef struct CodedBitstreamAV1Context {
>> diff --git a/libavcodec/cbs_av1_syntax_template.c
>> b/libavcodec/cbs_av1_syntax_template.c
>> index bcaa334134..4edf4fd47c 100644
>> --- a/libavcodec/cbs_av1_syntax_template.c
>> +++ b/libavcodec/cbs_av1_syntax_template.c
>> @@ -837,6 +837,9 @@ static int
>> FUNC(loop_filter_params)(CodedBitstreamContext *ctx, RWContext *rw,
>>                                       AV1RawFrameHeader *current)
>>   {
>>       CodedBitstreamAV1Context *priv = ctx->priv_data;
>> +    static const int8_t
>> default_loop_filter_ref_deltas[AV1_TOTAL_REFS_PER_FRAME] =
>> +        { 1, 0, 0, 0, -1, 0, -1, -1 };
>> +    static const int8_t default_loop_filter_mode_deltas[2] = { 0 };
> 
> I realise it's the same, but the single zero there looks like an error
> so I think put two of them.
> 
>>       int i, err;
>>         if (priv->coded_lossless || current->allow_intrabc) {
>> @@ -870,19 +873,44 @@ static int
>> FUNC(loop_filter_params)(CodedBitstreamContext *ctx, RWContext *rw,
>>         flag(loop_filter_delta_enabled);
>>       if (current->loop_filter_delta_enabled) {
>> +        const int8_t *loop_filter_ref_deltas, *loop_filter_mode_deltas;
> 
> Maybe call these ref_* to make the below a little clearer?  (foo[n] is
> inferred from ref_foo[n].)
> 
>> +
>> +        if (current->primary_ref_frame == AV1_PRIMARY_REF_NONE) {
>> +            loop_filter_ref_deltas = default_loop_filter_ref_deltas;
>> +            loop_filter_mode_deltas = default_loop_filter_mode_deltas;
>> +        } else {
>> +            loop_filter_ref_deltas =
>> +               
>> priv->ref[current->ref_frame_idx[current->primary_ref_frame]].loop_filter_ref_deltas;
>>
>> +            loop_filter_mode_deltas =
>> +               
>> priv->ref[current->ref_frame_idx[current->primary_ref_frame]].loop_filter_mode_deltas;
>>
>> +        }
>> +
>>           flag(loop_filter_delta_update);
>> -        if (current->loop_filter_delta_update) {
>>               for (i = 0; i < AV1_TOTAL_REFS_PER_FRAME; i++) {
>> -                flags(update_ref_delta[i], 1, i);
>> +                if (current->loop_filter_delta_update)
>> +                    flags(update_ref_delta[i], 1, i);
>> +                else
>> +                    infer(update_ref_delta[i], 0);
>>                   if (current->update_ref_delta[i])
>>                       sus(1 + 6, loop_filter_ref_deltas[i], 1, i);
>> +                else
>> +                    infer(loop_filter_ref_deltas[i],
>> loop_filter_ref_deltas[i]);
>>               }
>>               for (i = 0; i < 2; i++) {
>> -                flags(update_mode_delta[i], 1, i);
>> +                if (current->loop_filter_delta_update)
>> +                    flags(update_mode_delta[i], 1, i);
>> +                else
>> +                    infer(update_mode_delta[i], 0);
>>                   if (current->update_mode_delta[i])
>>                       sus(1 + 6, loop_filter_mode_deltas[i], 1, i);
>> +                else
>> +                    infer(loop_filter_mode_deltas[i],
>> loop_filter_mode_deltas[i]);
>>               }
>> -        }
>> +    } else {
>> +        for (i = 0; i < AV1_TOTAL_REFS_PER_FRAME; i++)
>> +            infer(loop_filter_ref_deltas[i],
>> default_loop_filter_ref_deltas[i]);
>> +        for (i = 0; i < 2; i++)
>> +            infer(loop_filter_mode_deltas[i],
>> default_loop_filter_mode_deltas[i]);
>>       }
>>         return 0;
>> @@ -1613,6 +1641,10 @@ update_refs:
>>                   .bit_depth      = priv->bit_depth,
>>                   .order_hint     = priv->order_hint,
>>               };
>> +            memcpy(priv->ref[i].loop_filter_ref_deltas,
>> current->loop_filter_ref_deltas,
>> +                   sizeof(current->loop_filter_ref_deltas));
>> +            memcpy(priv->ref[i].loop_filter_mode_deltas,
>> current->loop_filter_mode_deltas,
>> +                   sizeof(current->loop_filter_mode_deltas));
>>           }
>>       }
>>  
> 
> Looks sensible.

If you'd rather let the caller derive these values (because atm, none of
the bsfs care, only av1dec), then that's fine by me too.
See
https://github.com/jamrial/FFmpeg/commit/0803491e2e794a8d6cf409432b4d970da68a717b
for how it would be done there (replacing patch 3 in this set).
I personally prefer the approach in this patch since it works for all
cases and prevents code duplication in callers.

Patch 2 in this set however is probably needed because feature_enabled[]
and feature_value[] are both taken into account when setting
coded_lossless after segmentation_params() was called.

> 
> Thanks,
> 
> - Mark
> _______________________________________________
> 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 Oct. 27, 2020, 9:12 p.m. UTC | #3
On 27/10/2020 20:53, James Almer wrote:
> On 10/27/2020 5:38 PM, Mark Thompson wrote:
>> On 21/10/2020 01:11, James Almer wrote:
>>> Partially implements of setup_past_independence() and load_previous().
>>> These ensures they are always set, even if the values were not coded
>>> in the input bitstream and will not be coded in the output bitstream.
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>>    libavcodec/cbs_av1.h                 |  3 +++
>>>    libavcodec/cbs_av1_syntax_template.c | 40 +++++++++++++++++++++++++---
>>>    2 files changed, 39 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
>>> index 7a0c08c596..97aeee9795 100644
>>> --- a/libavcodec/cbs_av1.h
>>> +++ b/libavcodec/cbs_av1.h
>>> @@ -413,6 +413,9 @@ typedef struct AV1ReferenceFrameState {
>>>        int subsampling_y;  // RefSubsamplingY
>>>        int bit_depth;      // RefBitDepth
>>>        int order_hint;     // RefOrderHint
>>> +
>>> +    int8_t  loop_filter_ref_deltas[AV1_TOTAL_REFS_PER_FRAME];
>>> +    int8_t  loop_filter_mode_deltas[2];
>>>    } AV1ReferenceFrameState;
>>>      typedef struct CodedBitstreamAV1Context {
>>> diff --git a/libavcodec/cbs_av1_syntax_template.c
>>> b/libavcodec/cbs_av1_syntax_template.c
>>> index bcaa334134..4edf4fd47c 100644
>>> --- a/libavcodec/cbs_av1_syntax_template.c
>>> +++ b/libavcodec/cbs_av1_syntax_template.c
>>> @@ -837,6 +837,9 @@ static int
>>> FUNC(loop_filter_params)(CodedBitstreamContext *ctx, RWContext *rw,
>>>                                        AV1RawFrameHeader *current)
>>>    {
>>>        CodedBitstreamAV1Context *priv = ctx->priv_data;
>>> +    static const int8_t
>>> default_loop_filter_ref_deltas[AV1_TOTAL_REFS_PER_FRAME] =
>>> +        { 1, 0, 0, 0, -1, 0, -1, -1 };
>>> +    static const int8_t default_loop_filter_mode_deltas[2] = { 0 };
>>
>> I realise it's the same, but the single zero there looks like an error
>> so I think put two of them.
>>
>>>        int i, err;
>>>          if (priv->coded_lossless || current->allow_intrabc) {
>>> @@ -870,19 +873,44 @@ static int
>>> FUNC(loop_filter_params)(CodedBitstreamContext *ctx, RWContext *rw,
>>>          flag(loop_filter_delta_enabled);
>>>        if (current->loop_filter_delta_enabled) {
>>> +        const int8_t *loop_filter_ref_deltas, *loop_filter_mode_deltas;
>>
>> Maybe call these ref_* to make the below a little clearer?  (foo[n] is
>> inferred from ref_foo[n].)
>>
>>> +
>>> +        if (current->primary_ref_frame == AV1_PRIMARY_REF_NONE) {
>>> +            loop_filter_ref_deltas = default_loop_filter_ref_deltas;
>>> +            loop_filter_mode_deltas = default_loop_filter_mode_deltas;
>>> +        } else {
>>> +            loop_filter_ref_deltas =
>>> +
>>> priv->ref[current->ref_frame_idx[current->primary_ref_frame]].loop_filter_ref_deltas;
>>>
>>> +            loop_filter_mode_deltas =
>>> +
>>> priv->ref[current->ref_frame_idx[current->primary_ref_frame]].loop_filter_mode_deltas;
>>>
>>> +        }
>>> +
>>>            flag(loop_filter_delta_update);
>>> -        if (current->loop_filter_delta_update) {
>>>                for (i = 0; i < AV1_TOTAL_REFS_PER_FRAME; i++) {
>>> -                flags(update_ref_delta[i], 1, i);
>>> +                if (current->loop_filter_delta_update)
>>> +                    flags(update_ref_delta[i], 1, i);
>>> +                else
>>> +                    infer(update_ref_delta[i], 0);
>>>                    if (current->update_ref_delta[i])
>>>                        sus(1 + 6, loop_filter_ref_deltas[i], 1, i);
>>> +                else
>>> +                    infer(loop_filter_ref_deltas[i],
>>> loop_filter_ref_deltas[i]);
>>>                }
>>>                for (i = 0; i < 2; i++) {
>>> -                flags(update_mode_delta[i], 1, i);
>>> +                if (current->loop_filter_delta_update)
>>> +                    flags(update_mode_delta[i], 1, i);
>>> +                else
>>> +                    infer(update_mode_delta[i], 0);
>>>                    if (current->update_mode_delta[i])
>>>                        sus(1 + 6, loop_filter_mode_deltas[i], 1, i);
>>> +                else
>>> +                    infer(loop_filter_mode_deltas[i],
>>> loop_filter_mode_deltas[i]);
>>>                }
>>> -        }
>>> +    } else {
>>> +        for (i = 0; i < AV1_TOTAL_REFS_PER_FRAME; i++)
>>> +            infer(loop_filter_ref_deltas[i],
>>> default_loop_filter_ref_deltas[i]);
>>> +        for (i = 0; i < 2; i++)
>>> +            infer(loop_filter_mode_deltas[i],
>>> default_loop_filter_mode_deltas[i]);
>>>        }
>>>          return 0;
>>> @@ -1613,6 +1641,10 @@ update_refs:
>>>                    .bit_depth      = priv->bit_depth,
>>>                    .order_hint     = priv->order_hint,
>>>                };
>>> +            memcpy(priv->ref[i].loop_filter_ref_deltas,
>>> current->loop_filter_ref_deltas,
>>> +                   sizeof(current->loop_filter_ref_deltas));
>>> +            memcpy(priv->ref[i].loop_filter_mode_deltas,
>>> current->loop_filter_mode_deltas,
>>> +                   sizeof(current->loop_filter_mode_deltas));
>>>            }
>>>        }
>>>   
>>
>> Looks sensible.
> 
> If you'd rather let the caller derive these values (because atm, none of
> the bsfs care, only av1dec), then that's fine by me too.
> See
> https://github.com/jamrial/FFmpeg/commit/0803491e2e794a8d6cf409432b4d970da68a717b
> for how it would be done there (replacing patch 3 in this set).
> I personally prefer the approach in this patch since it works for all
> cases and prevents code duplication in callers.
> 
> Patch 2 in this set however is probably needed because feature_enabled[]
> and feature_value[] are both taken into account when setting
> coded_lossless after segmentation_params() was called.

Yes, I agree with the preference for this one.  It's a bitstream value so inferring the cases where it isn't actually present is reasonable and has little overhead (when in H.26n standards you get a nice "when not present, the value of foo is inferred to be X" line); it's only a problem when we start trying to carry a lot of derived values around.

(I'm still rather dubious on gm_params, though I guess I could be convinced.)

- Mark
James Almer Oct. 28, 2020, 2:52 p.m. UTC | #4
On 10/27/2020 6:12 PM, Mark Thompson wrote:
> On 27/10/2020 20:53, James Almer wrote:
>> On 10/27/2020 5:38 PM, Mark Thompson wrote:
>>> On 21/10/2020 01:11, James Almer wrote:
>>>> Partially implements of setup_past_independence() and load_previous().
>>>> These ensures they are always set, even if the values were not coded
>>>> in the input bitstream and will not be coded in the output bitstream.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>>    libavcodec/cbs_av1.h                 |  3 +++
>>>>    libavcodec/cbs_av1_syntax_template.c | 40
>>>> +++++++++++++++++++++++++---
>>>>    2 files changed, 39 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
>>>> index 7a0c08c596..97aeee9795 100644
>>>> --- a/libavcodec/cbs_av1.h
>>>> +++ b/libavcodec/cbs_av1.h
>>>> @@ -413,6 +413,9 @@ typedef struct AV1ReferenceFrameState {
>>>>        int subsampling_y;  // RefSubsamplingY
>>>>        int bit_depth;      // RefBitDepth
>>>>        int order_hint;     // RefOrderHint
>>>> +
>>>> +    int8_t  loop_filter_ref_deltas[AV1_TOTAL_REFS_PER_FRAME];
>>>> +    int8_t  loop_filter_mode_deltas[2];
>>>>    } AV1ReferenceFrameState;
>>>>      typedef struct CodedBitstreamAV1Context {
>>>> diff --git a/libavcodec/cbs_av1_syntax_template.c
>>>> b/libavcodec/cbs_av1_syntax_template.c
>>>> index bcaa334134..4edf4fd47c 100644
>>>> --- a/libavcodec/cbs_av1_syntax_template.c
>>>> +++ b/libavcodec/cbs_av1_syntax_template.c
>>>> @@ -837,6 +837,9 @@ static int
>>>> FUNC(loop_filter_params)(CodedBitstreamContext *ctx, RWContext *rw,
>>>>                                        AV1RawFrameHeader *current)
>>>>    {
>>>>        CodedBitstreamAV1Context *priv = ctx->priv_data;
>>>> +    static const int8_t
>>>> default_loop_filter_ref_deltas[AV1_TOTAL_REFS_PER_FRAME] =
>>>> +        { 1, 0, 0, 0, -1, 0, -1, -1 };
>>>> +    static const int8_t default_loop_filter_mode_deltas[2] = { 0 };
>>>
>>> I realise it's the same, but the single zero there looks like an error
>>> so I think put two of them.
>>>
>>>>        int i, err;
>>>>          if (priv->coded_lossless || current->allow_intrabc) {
>>>> @@ -870,19 +873,44 @@ static int
>>>> FUNC(loop_filter_params)(CodedBitstreamContext *ctx, RWContext *rw,
>>>>          flag(loop_filter_delta_enabled);
>>>>        if (current->loop_filter_delta_enabled) {
>>>> +        const int8_t *loop_filter_ref_deltas,
>>>> *loop_filter_mode_deltas;
>>>
>>> Maybe call these ref_* to make the below a little clearer?  (foo[n] is
>>> inferred from ref_foo[n].)
>>>
>>>> +
>>>> +        if (current->primary_ref_frame == AV1_PRIMARY_REF_NONE) {
>>>> +            loop_filter_ref_deltas = default_loop_filter_ref_deltas;
>>>> +            loop_filter_mode_deltas = default_loop_filter_mode_deltas;
>>>> +        } else {
>>>> +            loop_filter_ref_deltas =
>>>> +
>>>> priv->ref[current->ref_frame_idx[current->primary_ref_frame]].loop_filter_ref_deltas;
>>>>
>>>>
>>>> +            loop_filter_mode_deltas =
>>>> +
>>>> priv->ref[current->ref_frame_idx[current->primary_ref_frame]].loop_filter_mode_deltas;
>>>>
>>>>
>>>> +        }
>>>> +
>>>>            flag(loop_filter_delta_update);
>>>> -        if (current->loop_filter_delta_update) {
>>>>                for (i = 0; i < AV1_TOTAL_REFS_PER_FRAME; i++) {
>>>> -                flags(update_ref_delta[i], 1, i);
>>>> +                if (current->loop_filter_delta_update)
>>>> +                    flags(update_ref_delta[i], 1, i);
>>>> +                else
>>>> +                    infer(update_ref_delta[i], 0);
>>>>                    if (current->update_ref_delta[i])
>>>>                        sus(1 + 6, loop_filter_ref_deltas[i], 1, i);
>>>> +                else
>>>> +                    infer(loop_filter_ref_deltas[i],
>>>> loop_filter_ref_deltas[i]);
>>>>                }
>>>>                for (i = 0; i < 2; i++) {
>>>> -                flags(update_mode_delta[i], 1, i);
>>>> +                if (current->loop_filter_delta_update)
>>>> +                    flags(update_mode_delta[i], 1, i);
>>>> +                else
>>>> +                    infer(update_mode_delta[i], 0);
>>>>                    if (current->update_mode_delta[i])
>>>>                        sus(1 + 6, loop_filter_mode_deltas[i], 1, i);
>>>> +                else
>>>> +                    infer(loop_filter_mode_deltas[i],
>>>> loop_filter_mode_deltas[i]);
>>>>                }
>>>> -        }
>>>> +    } else {
>>>> +        for (i = 0; i < AV1_TOTAL_REFS_PER_FRAME; i++)
>>>> +            infer(loop_filter_ref_deltas[i],
>>>> default_loop_filter_ref_deltas[i]);
>>>> +        for (i = 0; i < 2; i++)
>>>> +            infer(loop_filter_mode_deltas[i],
>>>> default_loop_filter_mode_deltas[i]);
>>>>        }
>>>>          return 0;
>>>> @@ -1613,6 +1641,10 @@ update_refs:
>>>>                    .bit_depth      = priv->bit_depth,
>>>>                    .order_hint     = priv->order_hint,
>>>>                };
>>>> +            memcpy(priv->ref[i].loop_filter_ref_deltas,
>>>> current->loop_filter_ref_deltas,
>>>> +                   sizeof(current->loop_filter_ref_deltas));
>>>> +            memcpy(priv->ref[i].loop_filter_mode_deltas,
>>>> current->loop_filter_mode_deltas,
>>>> +                   sizeof(current->loop_filter_mode_deltas));
>>>>            }
>>>>        }
>>>>   
>>>
>>> Looks sensible.
>>
>> If you'd rather let the caller derive these values (because atm, none of
>> the bsfs care, only av1dec), then that's fine by me too.
>> See
>> https://github.com/jamrial/FFmpeg/commit/0803491e2e794a8d6cf409432b4d970da68a717b
>>
>> for how it would be done there (replacing patch 3 in this set).
>> I personally prefer the approach in this patch since it works for all
>> cases and prevents code duplication in callers.
>>
>> Patch 2 in this set however is probably needed because feature_enabled[]
>> and feature_value[] are both taken into account when setting
>> coded_lossless after segmentation_params() was called.
> 
> Yes, I agree with the preference for this one.  It's a bitstream value
> so inferring the cases where it isn't actually present is reasonable and
> has little overhead (when in H.26n standards you get a nice "when not
> present, the value of foo is inferred to be X" line); it's only a
> problem when we start trying to carry a lot of derived values around.
> 
> (I'm still rather dubious on gm_params, though I guess I could be
> convinced.)
> 
> - Mark

Made the changes you requested and applied, thanks.
diff mbox series

Patch

diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
index 7a0c08c596..97aeee9795 100644
--- a/libavcodec/cbs_av1.h
+++ b/libavcodec/cbs_av1.h
@@ -413,6 +413,9 @@  typedef struct AV1ReferenceFrameState {
     int subsampling_y;  // RefSubsamplingY
     int bit_depth;      // RefBitDepth
     int order_hint;     // RefOrderHint
+
+    int8_t  loop_filter_ref_deltas[AV1_TOTAL_REFS_PER_FRAME];
+    int8_t  loop_filter_mode_deltas[2];
 } AV1ReferenceFrameState;
 
 typedef struct CodedBitstreamAV1Context {
diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
index bcaa334134..4edf4fd47c 100644
--- a/libavcodec/cbs_av1_syntax_template.c
+++ b/libavcodec/cbs_av1_syntax_template.c
@@ -837,6 +837,9 @@  static int FUNC(loop_filter_params)(CodedBitstreamContext *ctx, RWContext *rw,
                                     AV1RawFrameHeader *current)
 {
     CodedBitstreamAV1Context *priv = ctx->priv_data;
+    static const int8_t default_loop_filter_ref_deltas[AV1_TOTAL_REFS_PER_FRAME] =
+        { 1, 0, 0, 0, -1, 0, -1, -1 };
+    static const int8_t default_loop_filter_mode_deltas[2] = { 0 };
     int i, err;
 
     if (priv->coded_lossless || current->allow_intrabc) {
@@ -870,19 +873,44 @@  static int FUNC(loop_filter_params)(CodedBitstreamContext *ctx, RWContext *rw,
 
     flag(loop_filter_delta_enabled);
     if (current->loop_filter_delta_enabled) {
+        const int8_t *loop_filter_ref_deltas, *loop_filter_mode_deltas;
+
+        if (current->primary_ref_frame == AV1_PRIMARY_REF_NONE) {
+            loop_filter_ref_deltas = default_loop_filter_ref_deltas;
+            loop_filter_mode_deltas = default_loop_filter_mode_deltas;
+        } else {
+            loop_filter_ref_deltas =
+                priv->ref[current->ref_frame_idx[current->primary_ref_frame]].loop_filter_ref_deltas;
+            loop_filter_mode_deltas =
+                priv->ref[current->ref_frame_idx[current->primary_ref_frame]].loop_filter_mode_deltas;
+        }
+
         flag(loop_filter_delta_update);
-        if (current->loop_filter_delta_update) {
             for (i = 0; i < AV1_TOTAL_REFS_PER_FRAME; i++) {
-                flags(update_ref_delta[i], 1, i);
+                if (current->loop_filter_delta_update)
+                    flags(update_ref_delta[i], 1, i);
+                else
+                    infer(update_ref_delta[i], 0);
                 if (current->update_ref_delta[i])
                     sus(1 + 6, loop_filter_ref_deltas[i], 1, i);
+                else
+                    infer(loop_filter_ref_deltas[i], loop_filter_ref_deltas[i]);
             }
             for (i = 0; i < 2; i++) {
-                flags(update_mode_delta[i], 1, i);
+                if (current->loop_filter_delta_update)
+                    flags(update_mode_delta[i], 1, i);
+                else
+                    infer(update_mode_delta[i], 0);
                 if (current->update_mode_delta[i])
                     sus(1 + 6, loop_filter_mode_deltas[i], 1, i);
+                else
+                    infer(loop_filter_mode_deltas[i], loop_filter_mode_deltas[i]);
             }
-        }
+    } else {
+        for (i = 0; i < AV1_TOTAL_REFS_PER_FRAME; i++)
+            infer(loop_filter_ref_deltas[i], default_loop_filter_ref_deltas[i]);
+        for (i = 0; i < 2; i++)
+            infer(loop_filter_mode_deltas[i], default_loop_filter_mode_deltas[i]);
     }
 
     return 0;
@@ -1613,6 +1641,10 @@  update_refs:
                 .bit_depth      = priv->bit_depth,
                 .order_hint     = priv->order_hint,
             };
+            memcpy(priv->ref[i].loop_filter_ref_deltas, current->loop_filter_ref_deltas,
+                   sizeof(current->loop_filter_ref_deltas));
+            memcpy(priv->ref[i].loop_filter_mode_deltas, current->loop_filter_mode_deltas,
+                   sizeof(current->loop_filter_mode_deltas));
         }
     }