[FFmpeg-devel] [PATCH] vulkan_av1: Set force_integer_mv on intra frames

[Lynne]

Apr 27, 2024, 16:07 by sw at jkqxz.net:

> On 27/04/2024 14:58, Mark Thompson wrote:
>
>> On 27/04/2024 14:47, Ronald S. Bultje wrote:
>>
>>> Hi,
>>>
>>> On Sat, Apr 27, 2024 at 9:40 AM Mark Thompson <sw at jkqxz.net> wrote:
>>>
>>>> The flag in CBS is the value read from the bitstream (as required for
>>>> passthrough), but the specification overrides that by setting it
>>>> immediately after reading if the frame is intra.
>>>> ---
>>>> This is already done for DXVA and VDPAU.  Also wondering whether this
>>>> should be done for VAAPI?
>>>>
>>>>  libavcodec/vulkan_av1.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavcodec/vulkan_av1.c b/libavcodec/vulkan_av1.c
>>>> index 25ab4ecc70..694e643954 100644
>>>> --- a/libavcodec/vulkan_av1.c
>>>> +++ b/libavcodec/vulkan_av1.c
>>>> @@ -435,7 +435,7 @@ static int vk_av1_start_frame(AVCodecContext
>>>> *avctx,
>>>>  .render_and_frame_size_different =
>>>> frame_header->render_and_frame_size_different,
>>>>  .allow_screen_content_tools =
>>>> frame_header->allow_screen_content_tools,
>>>>  .is_filter_switchable = frame_header->is_filter_switchable,
>>>> -            .force_integer_mv = frame_header->force_integer_mv,
>>>> +            .force_integer_mv = frame_header->force_integer_mv ||
>>>> !(frame_header->frame_type & 1),
>>>>  .frame_size_override_flag =
>>>> frame_header->frame_size_override_flag,
>>>>  .buffer_removal_time_present_flag =
>>>> frame_header->buffer_removal_time_present_flag,
>>>>  .allow_intrabc = frame_header->allow_intrabc,
>>>> --
>>>> 2.43.0
>>>>
>>>
>>> Wouldn't it be better to adjust the CBS reader then? Prevents us from
>>> having to re-do this in every hw wrapper.
>>>
>>
>> How would you suggest doing this?
>>
>> We could add a derived field force_integer_mv somewhere else (av1dec?) but that doesn't seem obviously better for use in the "copy stuff from frame header" function.
>>
>
> Something like this.  Unsure whether this is more or less confusing!
>
>  libavcodec/av1dec.c     | 7 +++++++
>  libavcodec/av1dec.h     | 3 +++
>  libavcodec/vulkan_av1.c | 2 +-
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/av1dec.c b/libavcodec/av1dec.c
> index 79a30a114d..601e3f05e1 100644
> --- a/libavcodec/av1dec.c
> +++ b/libavcodec/av1dec.c
> @@ -725,6 +725,8 @@ static void av1_frame_replace(AV1Frame *dst, const AV1Frame *src)
>  sizeof(dst->ref_frame_sign_bias));
>  memcpy(dst->order_hints, src->order_hints,
>  sizeof(dst->order_hints));
> +
> +    dst->force_integer_mv = src->force_integer_mv;
>  }
>
>  static av_cold int av1_decode_free(AVCodecContext *avctx)
> @@ -1255,6 +1257,11 @@ static int get_current_frame(AVCodecContext *avctx)
>  order_hint_info(s);
>  load_grain_params(s);
>
> +    s->cur_frame.force_integer_mv =
> +        s->raw_frame_header->force_integer_mv ||
> +        s->raw_frame_header->frame_type == AV1_FRAME_KEY ||
> +        s->raw_frame_header->frame_type == AV1_FRAME_INTRA_ONLY;
> +
>  return ret;
>  }
>
> diff --git a/libavcodec/av1dec.h b/libavcodec/av1dec.h
> index 66a732d781..e3016b82a3 100644
> --- a/libavcodec/av1dec.h
> +++ b/libavcodec/av1dec.h
> @@ -67,6 +67,9 @@ typedef struct AV1Frame {
>  // OrderHints[] when this is the current frame, otherwise
>  // SavedOrderHints[s][] when is the reference frame in slot s.
>  uint8_t order_hints[AV1_TOTAL_REFS_PER_FRAME];
> +
> +    // force_integer_mv value at the end of the frame header parsing.
> +    uint8_t force_integer_mv;
>  } AV1Frame;
>
>  typedef struct TileGroupInfo {
> diff --git a/libavcodec/vulkan_av1.c b/libavcodec/vulkan_av1.c
> index 25ab4ecc70..a550215e32 100644
> --- a/libavcodec/vulkan_av1.c
> +++ b/libavcodec/vulkan_av1.c
> @@ -435,7 +435,7 @@ static int vk_av1_start_frame(AVCodecContext          *avctx,
>  .render_and_frame_size_different = frame_header->render_and_frame_size_different,
>  .allow_screen_content_tools = frame_header->allow_screen_content_tools,
>  .is_filter_switchable = frame_header->is_filter_switchable,
> -            .force_integer_mv = frame_header->force_integer_mv,
> +            .force_integer_mv = pic->force_integer_mv,
>  .frame_size_override_flag = frame_header->frame_size_override_flag,
>  .buffer_removal_time_present_flag = frame_header->buffer_removal_time_present_flag,
>  .allow_intrabc = frame_header->allow_intrabc,
>

This looks better to me.