[FFmpeg-devel] [PATCH 1/4] avcodec/av1dec: expose skip mode frame index
James Almer
jamrial at gmail.com
Tue Nov 10 04:24:30 EET 2020
On 11/9/2020 8:02 PM, Mark Thompson wrote:
> 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.
He should do whatever cuviddec does, since one would assume is what the
hardware expects. primary_ref_frame in CUVIDAV1PICPARAMS for example is
set to -1 (255) to signal AV1_PRIMARY_REF_NONE.
Also, both AV1Frame and CUVIDAV1PICPARAMS are zero initialized, so not
setting it to anything is the same as setting it to 0.
>
>>>> +
>>>> + 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
>
> _______________________________________________
> 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".
More information about the ffmpeg-devel
mailing list