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

[James Almer]

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.

> 
> Thanks,
> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".