diff mbox series

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

Message ID b105c31f-0349-4062-9f7e-9bcd8ea7a606@jkqxz.net
State New
Headers show
Series [FFmpeg-devel] vulkan_av1: Set force_integer_mv on intra frames | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished

Commit Message

Mark Thompson April 27, 2024, 1:40 p.m. UTC 
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(-)

Comments

Ronald S. Bultje April 27, 2024, 1:47 p.m. UTC | #1 
Hi,

On Sat, Apr 27, 2024 at 9:40 AM Mark Thompson <sw@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.

Ronald
Mark Thompson April 27, 2024, 1:58 p.m. UTC | #2 
On 27/04/2024 14:47, Ronald S. Bultje wrote:
> Hi,
> 
> On Sat, Apr 27, 2024 at 9:40 AM Mark Thompson <sw@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.

- Mark
Mark Thompson April 27, 2024, 2:07 p.m. UTC | #3 
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@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,
Lynne April 27, 2024, 2:58 p.m. UTC | #4 
Apr 27, 2024, 16:07 by sw@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@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.
diff mbox series

Patch

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,