[FFmpeg-devel] [PATCH 1/4] avcodec/av1dec: expose skip mode frame index
Mark Thompson
sw at jkqxz.net
Tue Nov 10 01:02:10 EET 2020
On 09/11/2020 22:50, Timo Rothenpieler wrote:
> On 09.11.2020 23:30, Mark Thompson wrote:
>> On 09/11/2020 16:30, Timo Rothenpieler wrote:
>>> Signed-off-by: Timo Rothenpieler <timo at rothenpieler.org>
>>> Co-authored-by: James Almer <jamrial at gmail.com>
>>> ---
>>> libavcodec/av1dec.c | 89 +++++++++++++++++++++++++++++++++++++++++++++
>>> libavcodec/av1dec.h | 3 ++
>>> 2 files changed, 92 insertions(+)
>>>
>>> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
>>> index 56712279aa..83295699e1 100644
>>> --- a/libavcodec/av1dec.c
>>> +++ b/libavcodec/av1dec.c
>>> @@ -145,6 +145,87 @@ static void global_motion_params(AV1DecContext *s)
>>> }
>>> }
>>> +static int get_relative_dist(const AV1RawSequenceHeader *seq,
>>> + unsigned int a, unsigned int b)
>>> +{
>>> + unsigned int diff, m;
>>> + if (!seq->enable_order_hint)
>>> + return 0;
>>
>> This never gets called if !enable_order_hint, so the argument isn't needed.
>>
>>> + diff = a - b;
>>> + m = 1 << seq->order_hint_bits_minus_1;
>>> + diff = (diff & (m - 1)) - (diff & m);
>>> + return diff;
>>> +}
>>> +
>>> +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;
>>> +
>>> + s->cur_frame.skip_mode_frame_idx[0] = 0;
>>> + s->cur_frame.skip_mode_frame_idx[1] = 0;
>>
>> 0 is AV1_REF_FRAME_INTRA, which doesn't make sense for skip. Does this value actually get used?
>
> What else would you set it to in that case?
> It's set to 0 to indicate an invalid value.
If it isn't used then don't set it at all? If it has to be set then I guess use a value outside the 0..7 valid range of ref frames.
>>> +
>>> + if (header->frame_type == AV1_FRAME_KEY ||
>>> + header->frame_type == AV1_FRAME_INTRA_ONLY ||
>>> + !header->reference_select || !seq->enable_order_hint)
>>
>> if (!header->skip_mode_present)
>>
>> would be much simpler and short-cut some empty cases that this condition misses.
>
> But that's not equivalent to the CBS code, is it?
> Cause skip_mode_allowed can be 1 and the flag it reads into skip_mode_present can still be 0?
skip_mode_present is set iff skip modes are present. If they aren't then calculating what frames could have been used for skip references had they been present is meaningless.
> The code is trying to behave the same the skip_mode_params params function in the cbs_av1 parser does.
Which isn't what you want, because here we know more than the parser did. (Notably: whether frames you have headers for were actually complete, which you are doing.)
>>> + 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]].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]].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)
>>> {
>>> @@ -318,6 +399,7 @@ static void av1_frame_unref(AVCodecContext *avctx, AV1Frame *f)
>>> av_buffer_unref(&f->hwaccel_priv_buf);
>>> f->hwaccel_picture_private = NULL;
>>> f->spatial_id = f->temporal_id = 0;
>>> + f->order_hint = 0;
>>> }
>>> static int av1_frame_ref(AVCodecContext *avctx, AV1Frame *dst, const AV1Frame *src)
>>> @@ -337,12 +419,16 @@ static int av1_frame_ref(AVCodecContext *avctx, AV1Frame *dst, const AV1Frame *s
>>> dst->spatial_id = src->spatial_id;
>>> dst->temporal_id = src->temporal_id;
>>> + memcpy(dst->skip_mode_frame_idx,
>>> + src->skip_mode_frame_idx,
>>> + 2 * sizeof(uint8_t));
>>
>> Keeping this in the same order as the structure would make it easier to check that all fields are copied.
>>
>>> memcpy(dst->gm_type,
>>> src->gm_type,
>>> AV1_NUM_REF_FRAMES * sizeof(uint8_t));
>>> memcpy(dst->gm_params,
>>> src->gm_params,
>>> AV1_NUM_REF_FRAMES * 6 * sizeof(int32_t));
>>> + dst->order_hint = src->order_hint;
>>> return 0;
>>> @@ -613,6 +699,7 @@ static int get_current_frame(AVCodecContext *avctx)
>>> }
>>> global_motion_params(s);
>>> + skip_mode_params(s);
>>> return ret;
>>> }
>>> @@ -740,6 +827,8 @@ static int av1_decode_frame(AVCodecContext *avctx, void *frame,
>>> s->cur_frame.spatial_id = header->spatial_id;
>>> s->cur_frame.temporal_id = header->temporal_id;
>>> + s->cur_frame.order_hint = s->raw_frame_header->order_hint;
>>> +
>>> if (avctx->hwaccel) {
>>> ret = avctx->hwaccel->start_frame(avctx, unit->data,
>>> unit->data_size);
>>> diff --git a/libavcodec/av1dec.h b/libavcodec/av1dec.h
>>> index b58bc53961..8cf50f0d59 100644
>>> --- a/libavcodec/av1dec.h
>>> +++ b/libavcodec/av1dec.h
>>> @@ -41,6 +41,9 @@ typedef struct AV1Frame {
>>> uint8_t gm_type[AV1_NUM_REF_FRAMES];
>>> int32_t gm_params[AV1_NUM_REF_FRAMES][6];
>>> +
>>> + uint8_t order_hint;
>>> + uint8_t skip_mode_frame_idx[2];
>>> } AV1Frame;
>>> typedef struct TileGroupInfo {
>>>
- Mark
More information about the ffmpeg-devel
mailing list