[FFmpeg-devel,06/10] avfilter/avgblur_vulkan: call av_vkfmt_from_pixfmt only one time

There is a loop in av_vkfmt_from_pixfmt. And, even though some
compilers will optimize it when the optimization option is on, to
consider more situations, just call the function only one time here.

Signed-off-by: Wu Jianhua <jianhua.wu@intel.com>
---
 libavfilter/vf_avgblur_vulkan.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Wu Jianhua:
> There is a loop in av_vkfmt_from_pixfmt. And, even though some
> compilers will optimize it when the optimization option is on, to
> consider more situations, just call the function only one time here.
> 
> Signed-off-by: Wu Jianhua <jianhua.wu@intel.com>
> ---

What compilers optimize this? And how? After all, this function is not
marked as const.

>  libavfilter/vf_avgblur_vulkan.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/libavfilter/vf_avgblur_vulkan.c b/libavfilter/vf_avgblur_vulkan.c
> index 243e932f35..080a8b1b63 100644
> --- a/libavfilter/vf_avgblur_vulkan.c
> +++ b/libavfilter/vf_avgblur_vulkan.c
> @@ -210,6 +210,10 @@ static int process_frames(AVFilterContext *avctx, AVFrame *out_f, AVFrame *tmp_f
>      AVVkFrame *in = (AVVkFrame *)in_f->data[0];
>      AVVkFrame *tmp = (AVVkFrame *)tmp_f->data[0];
>      AVVkFrame *out = (AVVkFrame *)out_f->data[0];
> +
> +    const VkFormat *input_formats = av_vkfmt_from_pixfmt(s->vkctx.input_format);
> +    const VkFormat *output_formats = av_vkfmt_from_pixfmt(s->vkctx.output_format);
> +
>      int planes = av_pix_fmt_count_planes(s->vkctx.output_format);
>  
>      /* Update descriptors and init the exec context */
> @@ -219,17 +223,17 @@ static int process_frames(AVFilterContext *avctx, AVFrame *out_f, AVFrame *tmp_f
>      for (int i = 0; i < planes; i++) {
>          RET(ff_vk_create_imageview(avctx, s->exec, &s->input_images[i].imageView,
>                                     in->img[i],
> -                                   av_vkfmt_from_pixfmt(s->vkctx.input_format)[i],
> +                                   input_formats[i],
>                                     ff_comp_identity_map));
>  
>          RET(ff_vk_create_imageview(avctx, s->exec, &s->tmp_images[i].imageView,
>                                     tmp->img[i],
> -                                   av_vkfmt_from_pixfmt(s->vkctx.output_format)[i],
> +                                   output_formats[i],
>                                     ff_comp_identity_map));
>  
>          RET(ff_vk_create_imageview(avctx, s->exec, &s->output_images[i].imageView,
>                                     out->img[i],
> -                                   av_vkfmt_from_pixfmt(s->vkctx.output_format)[i],
> +                                   output_formats[i],
>                                     ff_comp_identity_map));
>  
>          s->input_images[i].imageLayout = VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL;
>

Andreas Rheinhardt wrote:
> 
> Wu Jianhua:
> > There is a loop in av_vkfmt_from_pixfmt. And, even though some
> > compilers will optimize it when the optimization option is on, to
> > consider more situations, just call the function only one time here.
> >
> > Signed-off-by: Wu Jianhua <jianhua.wu@intel.com>
> > ---
> 
> What compilers optimize this? And how? After all, this function is not marked
> as const.

When there is some same operations in the codes the compiler will help the
developer to reuse the same result when the optimization is on, like -O3 options
in GCC. However the -O# was not used, the function would be called again and
again. And I check the assembly codes, it did. If someone compiled with the
optimizations, this commit didn't changed anything in assembly codes. But it
did help when there is no optimizations compiling.

I am more prefer this way to just call the function only one time literally. I am
not sure if anybody like this so I put it on the back of the patch set. And I check
this function and it did return a const VkFormat * that supposed to
mean we should not change the content.

Thanks,
Jianhua

> 
> >  libavfilter/vf_avgblur_vulkan.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavfilter/vf_avgblur_vulkan.c
> > b/libavfilter/vf_avgblur_vulkan.c index 243e932f35..080a8b1b63 100644
> > --- a/libavfilter/vf_avgblur_vulkan.c
> > +++ b/libavfilter/vf_avgblur_vulkan.c
> > @@ -210,6 +210,10 @@ static int process_frames(AVFilterContext *avctx,
> AVFrame *out_f, AVFrame *tmp_f
> >      AVVkFrame *in = (AVVkFrame *)in_f->data[0];
> >      AVVkFrame *tmp = (AVVkFrame *)tmp_f->data[0];
> >      AVVkFrame *out = (AVVkFrame *)out_f->data[0];
> > +
> > +    const VkFormat *input_formats = av_vkfmt_from_pixfmt(s-
> >vkctx.input_format);
> > +    const VkFormat *output_formats =
> > + av_vkfmt_from_pixfmt(s->vkctx.output_format);
> > +
> >      int planes = av_pix_fmt_count_planes(s->vkctx.output_format);
> >
> >      /* Update descriptors and init the exec context */ @@ -219,17
> > +223,17 @@ static int process_frames(AVFilterContext *avctx, AVFrame
> *out_f, AVFrame *tmp_f
> >      for (int i = 0; i < planes; i++) {
> >          RET(ff_vk_create_imageview(avctx, s->exec, &s-
> >input_images[i].imageView,
> >                                     in->img[i],
> > -                                   av_vkfmt_from_pixfmt(s->vkctx.input_format)[i],
> > +                                   input_formats[i],
> >                                     ff_comp_identity_map));
> >
> >          RET(ff_vk_create_imageview(avctx, s->exec, &s-
> >tmp_images[i].imageView,
> >                                     tmp->img[i],
> > -                                   av_vkfmt_from_pixfmt(s->vkctx.output_format)[i],
> > +                                   output_formats[i],
> >                                     ff_comp_identity_map));
> >
> >          RET(ff_vk_create_imageview(avctx, s->exec, &s-
> >output_images[i].imageView,
> >                                     out->img[i],
> > -                                   av_vkfmt_from_pixfmt(s->vkctx.output_format)[i],
> > +                                   output_formats[i],
> >                                     ff_comp_identity_map));
> >
> >          s->input_images[i].imageLayout =
> > VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL;
> >
> 
> _______________________________________________
> 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".

Wu, Jianhua:
> Andreas Rheinhardt wrote:
>>
>> Wu Jianhua:
>>> There is a loop in av_vkfmt_from_pixfmt. And, even though some
>>> compilers will optimize it when the optimization option is on, to
>>> consider more situations, just call the function only one time here.
>>>
>>> Signed-off-by: Wu Jianhua <jianhua.wu@intel.com>
>>> ---
>>
>> What compilers optimize this? And how? After all, this function is not marked
>> as const.
> 
> When there is some same operations in the codes the compiler will help the
> developer to reuse the same result when the optimization is on, like -O3 options
> in GCC. However the -O# was not used, the function would be called again and
> again. And I check the assembly codes, it did. If someone compiled with the
> optimizations, this commit didn't changed anything in assembly codes. But it
> did help when there is no optimizations compiling.
> 

That is very surprising: Without a pure/const attribute the compiler is
not allowed to optimize multiple calls to the same functions into one;
and even with said attribute it is only allowed to do so if it can prove
that the argument (and in case of pure also the whole state of the
program) didn't change. I don't see how the compiler could know this, so
regardless of whether you add the attribute it would be a compiler bug
if the compiler actually optimized the code in that way.

> I am more prefer this way to just call the function only one time literally. I am
> not sure if anybody like this so I put it on the back of the patch set. And I check
> this function and it did return a const VkFormat * that supposed to
> mean we should not change the content.
> 
I also prefer to not emit unnecessary calls; it's just that the commit
message is surprising/unbelievable.

- Andreas

Andreas Rheinhardt:
> Sent: Thursday, November 18, 2021 1:24 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 06/10] avfilter/avgblur_vulkan: call
> av_vkfmt_from_pixfmt only one time
> 
> Wu, Jianhua:
> > Andreas Rheinhardt wrote:
> >>
> >> Wu Jianhua:
> >>> There is a loop in av_vkfmt_from_pixfmt. And, even though some
> >>> compilers will optimize it when the optimization option is on, to
> >>> consider more situations, just call the function only one time here.
> >>>
> >>> Signed-off-by: Wu Jianhua <jianhua.wu@intel.com>
> >>> ---
> >>
> >> What compilers optimize this? And how? After all, this function is
> >> not marked as const.
> >
> > When there is some same operations in the codes the compiler will help
> > the developer to reuse the same result when the optimization is on,
> > like -O3 options in GCC. However the -O# was not used, the function
> > would be called again and again. And I check the assembly codes, it
> > did. If someone compiled with the optimizations, this commit didn't
> > changed anything in assembly codes. But it did help when there is no
> optimizations compiling.
> >
> 
> That is very surprising: Without a pure/const attribute the compiler is not
> allowed to optimize multiple calls to the same functions into one; and even
> with said attribute it is only allowed to do so if it can prove that the argument
> (and in case of pure also the whole state of the
> program) didn't change. I don't see how the compiler could know this, so
> regardless of whether you add the attribute it would be a compiler bug if the
> compiler actually optimized the code in that way.
> 

It seemed that it's just the compiler know how we call the functions because we
expose the implementation codes. If we repeatedly call a function from an opaque
binary API, the multiple calling would not be optimized at all.

> > I am more prefer this way to just call the function only one time
> > literally. I am not sure if anybody like this so I put it on the back
> > of the patch set. And I check this function and it did return a const
> > VkFormat * that supposed to mean we should not change the content.
> >
> I also prefer to not emit unnecessary calls; it's just that the commit message
> is surprising/unbelievable.
> 

Yeah. Bad description perhaps. The commit title is enough, I'm supposed to remove the description.