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

[Mark Thompson]

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 at 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