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

2024-04-27 Thread Lynne 
Apr 27, 2024, 16:07 by s...@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  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-requ...@ffmpeg.org with subject "unsubscribe".

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

2024-04-27 Thread 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  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-requ...@ffmpeg.org with subject "unsubscribe".

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

2024-04-27 Thread Mark Thompson 
On 27/04/2024 14:47, Ronald S. Bultje wrote:
> Hi,
> 
> On Sat, Apr 27, 2024 at 9:40 AM Mark Thompson  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-requ...@ffmpeg.org with subject "unsubscribe".

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

2024-04-27 Thread Ronald S. Bultje 
Hi,

On Sat, Apr 27, 2024 at 9:40 AM Mark Thompson  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-requ...@ffmpeg.org with subject "unsubscribe".