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

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

[Mark Thompson]

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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Ronald S. Bultje]

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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Mark Thompson]

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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Mark Thompson]

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,
-- 
2.43.0
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

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

[Lynne]

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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".