diff mbox series

[FFmpeg-devel,07/10] avfilter/vf_nlmeans: Move ff_nlmeans_init into a header

Message ID AS8PR01MB79444DB5BC9AE98E12E417C58FC09@AS8PR01MB7944.eurprd01.prod.exchangelabs.com
State Accepted
Commit c499f9bc38506443550716761158fe00bb655224
Headers show
Series [FFmpeg-devel,01/10] avfilter/af_afir: Only keep DSP stuff in header | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Andreas Rheinhardt May 3, 2022, 6:37 a.m. UTC 
This removes a dependency of checkasm on lavfi/vf_nlmeans.o
and also allows to inline ff_nlmeans_init() irrespectively of
interposing.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavfilter/vf_nlmeans.c      | 108 +-------------------------
 libavfilter/vf_nlmeans.h      |   1 -
 libavfilter/vf_nlmeans_init.h | 139 ++++++++++++++++++++++++++++++++++
 tests/checkasm/vf_nlmeans.c   |   2 +-
 4 files changed, 141 insertions(+), 109 deletions(-)
 create mode 100644 libavfilter/vf_nlmeans_init.h

Comments

Soft Works May 13, 2022, 6:28 a.m. UTC | #1 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Tuesday, May 3, 2022 8:38 AM
> To: ffmpeg-devel@ffmpeg.org
> Cc: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> Subject: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> ff_nlmeans_init into a header
> 
> This removes a dependency of checkasm on lavfi/vf_nlmeans.o
> and also allows to inline ff_nlmeans_init() irrespectively of
> interposing.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---

[..]

> +
> +static av_unused void ff_nlmeans_init(NLMeansDSPContext *dsp)
> +{
> +    dsp->compute_safe_ssd_integral_image =
> compute_safe_ssd_integral_image_c;
> +    dsp->compute_weights_line = compute_weights_line_c;
> +
> +    if (ARCH_AARCH64)
> +        ff_nlmeans_init_aarch64(dsp);

Hi Andreas,

the above breaks compilation for me:

1>libavfilterd.lib(libavfilter_vf_nlmeans.obj) : error LNK2019: unresolved external symbol ff_nlmeans_init_aarch64 referenced in function ff_nlmeans_init

The reason is that I'm (obviously) not compiling stuff from the 
libavfilter\aarch64 subfolder.

It might need an #ifdef ?

I haven't taken a deeper look at it, though.

Thanks,
softworkz
Andreas Rheinhardt May 13, 2022, 8:27 a.m. UTC | #2 
Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Andreas Rheinhardt
>> Sent: Tuesday, May 3, 2022 8:38 AM
>> To: ffmpeg-devel@ffmpeg.org
>> Cc: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> Subject: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
>> ff_nlmeans_init into a header
>>
>> This removes a dependency of checkasm on lavfi/vf_nlmeans.o
>> and also allows to inline ff_nlmeans_init() irrespectively of
>> interposing.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
> 
> [..]
> 
>> +
>> +static av_unused void ff_nlmeans_init(NLMeansDSPContext *dsp)
>> +{
>> +    dsp->compute_safe_ssd_integral_image =
>> compute_safe_ssd_integral_image_c;
>> +    dsp->compute_weights_line = compute_weights_line_c;
>> +
>> +    if (ARCH_AARCH64)
>> +        ff_nlmeans_init_aarch64(dsp);
> 
> Hi Andreas,
> 
> the above breaks compilation for me:
> 
> 1>libavfilterd.lib(libavfilter_vf_nlmeans.obj) : error LNK2019: unresolved external symbol ff_nlmeans_init_aarch64 referenced in function ff_nlmeans_init
> 
> The reason is that I'm (obviously) not compiling stuff from the 
> libavfilter\aarch64 subfolder.
> 
> It might need an #ifdef ?
> 
> I haven't taken a deeper look at it, though.
> 
> Thanks,
> softworkz
> 
> 

That surprises me: The earlier code did exactly the same; in fact, using
if (ARCH_*) is our typical check for arches in dsp-init code.
Is this the only place where this happens?
#ifdef is certainly wrong: All ARCH_* are always defined; they are just
0 or 1.
Anyway, will send a patch with #if.

- Andreas
Hendrik Leppkes May 13, 2022, 9 a.m. UTC | #3 
On Fri, May 13, 2022 at 10:27 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Andreas Rheinhardt
> >> Sent: Tuesday, May 3, 2022 8:38 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Cc: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >> Subject: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> >> ff_nlmeans_init into a header
> >>
> >> This removes a dependency of checkasm on lavfi/vf_nlmeans.o
> >> and also allows to inline ff_nlmeans_init() irrespectively of
> >> interposing.
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >> ---
> >
> > [..]
> >
> >> +
> >> +static av_unused void ff_nlmeans_init(NLMeansDSPContext *dsp)
> >> +{
> >> +    dsp->compute_safe_ssd_integral_image =
> >> compute_safe_ssd_integral_image_c;
> >> +    dsp->compute_weights_line = compute_weights_line_c;
> >> +
> >> +    if (ARCH_AARCH64)
> >> +        ff_nlmeans_init_aarch64(dsp);
> >
> > Hi Andreas,
> >
> > the above breaks compilation for me:
> >
> > 1>libavfilterd.lib(libavfilter_vf_nlmeans.obj) : error LNK2019: unresolved external symbol ff_nlmeans_init_aarch64 referenced in function ff_nlmeans_init
> >
> > The reason is that I'm (obviously) not compiling stuff from the
> > libavfilter\aarch64 subfolder.
> >
> > It might need an #ifdef ?
> >
> > I haven't taken a deeper look at it, though.
> >
> > Thanks,
> > softworkz
> >
> >
>
> That surprises me: The earlier code did exactly the same; in fact, using
> if (ARCH_*) is our typical check for arches in dsp-init code.
> Is this the only place where this happens?
> #ifdef is certainly wrong: All ARCH_* are always defined; they are just
> 0 or 1.
> Anyway, will send a patch with #if.
>

The inlining due to the code coming from a header probably breaks
dead-code-elimination in some compilers.

- Hendrik
Soft Works May 13, 2022, 9:01 a.m. UTC | #4 
> -----Original Message-----
> From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> Sent: Friday, May 13, 2022 10:27 AM
> To: Soft Works <softworkz@hotmail.com>; FFmpeg development discussions
> and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> ff_nlmeans_init into a header
> 
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Andreas Rheinhardt
> >> Sent: Tuesday, May 3, 2022 8:38 AM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Cc: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >> Subject: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> >> ff_nlmeans_init into a header
> >>
> >> This removes a dependency of checkasm on lavfi/vf_nlmeans.o
> >> and also allows to inline ff_nlmeans_init() irrespectively of
> >> interposing.
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >> ---
> >
> > [..]
> >
> >> +
> >> +static av_unused void ff_nlmeans_init(NLMeansDSPContext *dsp)
> >> +{
> >> +    dsp->compute_safe_ssd_integral_image =
> >> compute_safe_ssd_integral_image_c;
> >> +    dsp->compute_weights_line = compute_weights_line_c;
> >> +
> >> +    if (ARCH_AARCH64)
> >> +        ff_nlmeans_init_aarch64(dsp);
> >
> > Hi Andreas,
> >
> > the above breaks compilation for me:
> >
> > 1>libavfilterd.lib(libavfilter_vf_nlmeans.obj) : error LNK2019:
> unresolved external symbol ff_nlmeans_init_aarch64 referenced in
> function ff_nlmeans_init
> >
> > The reason is that I'm (obviously) not compiling stuff from the
> > libavfilter\aarch64 subfolder.
> >
> > It might need an #ifdef ?
> >
> > I haven't taken a deeper look at it, though.
> >
> > Thanks,
> > softworkz
> >
> >
> 
> That surprises me: The earlier code did exactly the same; in fact,
> using
> if (ARCH_*) is our typical check for arches in dsp-init code.

I looked at this a bit further. It seems that the VS project 
generation tool that I'm using is creating dummy definitions
for such cases. In the previous workspace it had generated

void ff_nlmeans_init_aarch64(NLMeansDSPContext *dsp) {return;}

in a separate code file for being able to work with the ffmpeg
code in VS without modifying any of the code.

Now that you have moved that code to a header file, this logic 
doesn't work anymore.

> Is this the only place where this happens?

Yes.

> Anyway, will send a patch with #if.

Great, thanks!

softworkz
Soft Works May 13, 2022, 9:03 a.m. UTC | #5 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Hendrik Leppkes
> Sent: Friday, May 13, 2022 11:00 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> ff_nlmeans_init into a header
> 
> On Fri, May 13, 2022 at 10:27 AM Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
> >
> > Soft Works:
> > >
> > >
> > >> -----Original Message-----
> > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > >> Andreas Rheinhardt
> > >> Sent: Tuesday, May 3, 2022 8:38 AM
> > >> To: ffmpeg-devel@ffmpeg.org
> > >> Cc: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> > >> Subject: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> > >> ff_nlmeans_init into a header
> > >>
> > >> This removes a dependency of checkasm on lavfi/vf_nlmeans.o
> > >> and also allows to inline ff_nlmeans_init() irrespectively of
> > >> interposing.
> > >>
> > >> Signed-off-by: Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com>
> > >> ---
> > >
> > > [..]
> > >
> > >> +
> > >> +static av_unused void ff_nlmeans_init(NLMeansDSPContext *dsp)
> > >> +{
> > >> +    dsp->compute_safe_ssd_integral_image =
> > >> compute_safe_ssd_integral_image_c;
> > >> +    dsp->compute_weights_line = compute_weights_line_c;
> > >> +
> > >> +    if (ARCH_AARCH64)
> > >> +        ff_nlmeans_init_aarch64(dsp);
> > >
> > > Hi Andreas,
> > >
> > > the above breaks compilation for me:
> > >
> > > 1>libavfilterd.lib(libavfilter_vf_nlmeans.obj) : error LNK2019:
> unresolved external symbol ff_nlmeans_init_aarch64 referenced in
> function ff_nlmeans_init
> > >
> > > The reason is that I'm (obviously) not compiling stuff from the
> > > libavfilter\aarch64 subfolder.
> > >
> > > It might need an #ifdef ?
> > >
> > > I haven't taken a deeper look at it, though.
> > >
> > > Thanks,
> > > softworkz
> > >
> > >
> >
> > That surprises me: The earlier code did exactly the same; in fact,
> using
> > if (ARCH_*) is our typical check for arches in dsp-init code.
> > Is this the only place where this happens?
> > #ifdef is certainly wrong: All ARCH_* are always defined; they are
> just
> > 0 or 1.
> > Anyway, will send a patch with #if.
> >
> 
> The inlining due to the code coming from a header probably breaks
> dead-code-elimination in some compilers.

Ah, that explains why that file with the empty stubs is named 
dce_defs.c :-)

Thanks,
softworkz
Hendrik Leppkes May 13, 2022, 9:13 a.m. UTC | #6 
On Fri, May 13, 2022 at 11:01 AM Soft Works <softworkz@hotmail.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> > Sent: Friday, May 13, 2022 10:27 AM
> > To: Soft Works <softworkz@hotmail.com>; FFmpeg development discussions
> > and patches <ffmpeg-devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> > ff_nlmeans_init into a header
> >
> > Soft Works:
> > >
> > >
> > >> -----Original Message-----
> > >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> > >> Andreas Rheinhardt
> > >> Sent: Tuesday, May 3, 2022 8:38 AM
> > >> To: ffmpeg-devel@ffmpeg.org
> > >> Cc: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> > >> Subject: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> > >> ff_nlmeans_init into a header
> > >>
> > >> This removes a dependency of checkasm on lavfi/vf_nlmeans.o
> > >> and also allows to inline ff_nlmeans_init() irrespectively of
> > >> interposing.
> > >>
> > >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> > >> ---
> > >
> > > [..]
> > >
> > >> +
> > >> +static av_unused void ff_nlmeans_init(NLMeansDSPContext *dsp)
> > >> +{
> > >> +    dsp->compute_safe_ssd_integral_image =
> > >> compute_safe_ssd_integral_image_c;
> > >> +    dsp->compute_weights_line = compute_weights_line_c;
> > >> +
> > >> +    if (ARCH_AARCH64)
> > >> +        ff_nlmeans_init_aarch64(dsp);
> > >
> > > Hi Andreas,
> > >
> > > the above breaks compilation for me:
> > >
> > > 1>libavfilterd.lib(libavfilter_vf_nlmeans.obj) : error LNK2019:
> > unresolved external symbol ff_nlmeans_init_aarch64 referenced in
> > function ff_nlmeans_init
> > >
> > > The reason is that I'm (obviously) not compiling stuff from the
> > > libavfilter\aarch64 subfolder.
> > >
> > > It might need an #ifdef ?
> > >
> > > I haven't taken a deeper look at it, though.
> > >
> > > Thanks,
> > > softworkz
> > >
> > >
> >
> > That surprises me: The earlier code did exactly the same; in fact,
> > using
> > if (ARCH_*) is our typical check for arches in dsp-init code.
>
> I looked at this a bit further. It seems that the VS project
> generation tool that I'm using is creating dummy definitions
> for such cases. In the previous workspace it had generated
>
> void ff_nlmeans_init_aarch64(NLMeansDSPContext *dsp) {return;}
>
> in a separate code file for being able to work with the ffmpeg
> code in VS without modifying any of the code.
>
> Now that you have moved that code to a header file, this logic
> doesn't work anymore.
>

Nevermind my previous comment then, perhaps external tools should be
updated if they somehow work around our need for DCE (which is well
documented).

- Hendrik
Andreas Rheinhardt May 13, 2022, 9:25 a.m. UTC | #7 
Soft Works:
> 
> 
>> -----Original Message-----
>> From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> Sent: Friday, May 13, 2022 10:27 AM
>> To: Soft Works <softworkz@hotmail.com>; FFmpeg development discussions
>> and patches <ffmpeg-devel@ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
>> ff_nlmeans_init into a header
>>
>> Soft Works:
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>>>> Andreas Rheinhardt
>>>> Sent: Tuesday, May 3, 2022 8:38 AM
>>>> To: ffmpeg-devel@ffmpeg.org
>>>> Cc: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>>>> Subject: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
>>>> ff_nlmeans_init into a header
>>>>
>>>> This removes a dependency of checkasm on lavfi/vf_nlmeans.o
>>>> and also allows to inline ff_nlmeans_init() irrespectively of
>>>> interposing.
>>>>
>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>>>> ---
>>>
>>> [..]
>>>
>>>> +
>>>> +static av_unused void ff_nlmeans_init(NLMeansDSPContext *dsp)
>>>> +{
>>>> +    dsp->compute_safe_ssd_integral_image =
>>>> compute_safe_ssd_integral_image_c;
>>>> +    dsp->compute_weights_line = compute_weights_line_c;
>>>> +
>>>> +    if (ARCH_AARCH64)
>>>> +        ff_nlmeans_init_aarch64(dsp);
>>>
>>> Hi Andreas,
>>>
>>> the above breaks compilation for me:
>>>
>>> 1>libavfilterd.lib(libavfilter_vf_nlmeans.obj) : error LNK2019:
>> unresolved external symbol ff_nlmeans_init_aarch64 referenced in
>> function ff_nlmeans_init
>>>
>>> The reason is that I'm (obviously) not compiling stuff from the
>>> libavfilter\aarch64 subfolder.
>>>
>>> It might need an #ifdef ?
>>>
>>> I haven't taken a deeper look at it, though.
>>>
>>> Thanks,
>>> softworkz
>>>
>>>
>>
>> That surprises me: The earlier code did exactly the same; in fact,
>> using
>> if (ARCH_*) is our typical check for arches in dsp-init code.
> 
> I looked at this a bit further. It seems that the VS project 
> generation tool that I'm using is creating dummy definitions
> for such cases. In the previous workspace it had generated
> 
> void ff_nlmeans_init_aarch64(NLMeansDSPContext *dsp) {return;}
> 
> in a separate code file for being able to work with the ffmpeg
> code in VS without modifying any of the code.
> 
> Now that you have moved that code to a header file, this logic 
> doesn't work anymore.
> 

Why does your compiler not just eliminate dead code like all the others?
Is this MSVC -O0 behaviour?
I just sent the patch
https://ffmpeg.org/pipermail/ffmpeg-devel/2022-May/296380.html, but now
that I read this I have to agree with Hendrik: Why not update your tool
instead?

- Andreas
Hendrik Leppkes May 13, 2022, 9:27 a.m. UTC | #8 
On Fri, May 13, 2022 at 11:26 AM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >> Sent: Friday, May 13, 2022 10:27 AM
> >> To: Soft Works <softworkz@hotmail.com>; FFmpeg development discussions
> >> and patches <ffmpeg-devel@ffmpeg.org>
> >> Subject: Re: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> >> ff_nlmeans_init into a header
> >>
> >> Soft Works:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >>>> Andreas Rheinhardt
> >>>> Sent: Tuesday, May 3, 2022 8:38 AM
> >>>> To: ffmpeg-devel@ffmpeg.org
> >>>> Cc: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >>>> Subject: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> >>>> ff_nlmeans_init into a header
> >>>>
> >>>> This removes a dependency of checkasm on lavfi/vf_nlmeans.o
> >>>> and also allows to inline ff_nlmeans_init() irrespectively of
> >>>> interposing.
> >>>>
> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >>>> ---
> >>>
> >>> [..]
> >>>
> >>>> +
> >>>> +static av_unused void ff_nlmeans_init(NLMeansDSPContext *dsp)
> >>>> +{
> >>>> +    dsp->compute_safe_ssd_integral_image =
> >>>> compute_safe_ssd_integral_image_c;
> >>>> +    dsp->compute_weights_line = compute_weights_line_c;
> >>>> +
> >>>> +    if (ARCH_AARCH64)
> >>>> +        ff_nlmeans_init_aarch64(dsp);
> >>>
> >>> Hi Andreas,
> >>>
> >>> the above breaks compilation for me:
> >>>
> >>> 1>libavfilterd.lib(libavfilter_vf_nlmeans.obj) : error LNK2019:
> >> unresolved external symbol ff_nlmeans_init_aarch64 referenced in
> >> function ff_nlmeans_init
> >>>
> >>> The reason is that I'm (obviously) not compiling stuff from the
> >>> libavfilter\aarch64 subfolder.
> >>>
> >>> It might need an #ifdef ?
> >>>
> >>> I haven't taken a deeper look at it, though.
> >>>
> >>> Thanks,
> >>> softworkz
> >>>
> >>>
> >>
> >> That surprises me: The earlier code did exactly the same; in fact,
> >> using
> >> if (ARCH_*) is our typical check for arches in dsp-init code.
> >
> > I looked at this a bit further. It seems that the VS project
> > generation tool that I'm using is creating dummy definitions
> > for such cases. In the previous workspace it had generated
> >
> > void ff_nlmeans_init_aarch64(NLMeansDSPContext *dsp) {return;}
> >
> > in a separate code file for being able to work with the ffmpeg
> > code in VS without modifying any of the code.
> >
> > Now that you have moved that code to a header file, this logic
> > doesn't work anymore.
> >
>
> Why does your compiler not just eliminate dead code like all the others?
> Is this MSVC -O0 behaviour?


MSVC does not do DCE when optimizations are disabled, yes. So this is
where this is coming from.

> I just sent the patch
> https://ffmpeg.org/pipermail/ffmpeg-devel/2022-May/296380.html, but now
> that I read this I have to agree with Hendrik: Why not update your tool
> instead?
>

Actually looking at the tool, it seems like it handles header files.
Did you actually re-run the generation tool after applying this patch?

- Hendrik
Soft Works May 13, 2022, 9:32 a.m. UTC | #9 
> -----Original Message-----
> From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> Sent: Friday, May 13, 2022 11:26 AM
> To: Soft Works <softworkz@hotmail.com>; FFmpeg development discussions
> and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> ff_nlmeans_init into a header
> 
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >> Sent: Friday, May 13, 2022 10:27 AM
> >> To: Soft Works <softworkz@hotmail.com>; FFmpeg development
> discussions
> >> and patches <ffmpeg-devel@ffmpeg.org>
> >> Subject: Re: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> >> ff_nlmeans_init into a header
> >>
> >> Soft Works:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >>>> Andreas Rheinhardt
> >>>> Sent: Tuesday, May 3, 2022 8:38 AM
> >>>> To: ffmpeg-devel@ffmpeg.org
> >>>> Cc: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >>>> Subject: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> >>>> ff_nlmeans_init into a header
> >>>>
> >>>> This removes a dependency of checkasm on lavfi/vf_nlmeans.o
> >>>> and also allows to inline ff_nlmeans_init() irrespectively of
> >>>> interposing.
> >>>>
> >>>> Signed-off-by: Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com>
> >>>> ---
> >>>
> >>> [..]
> >>>
> >>>> +
> >>>> +static av_unused void ff_nlmeans_init(NLMeansDSPContext *dsp)
> >>>> +{
> >>>> +    dsp->compute_safe_ssd_integral_image =
> >>>> compute_safe_ssd_integral_image_c;
> >>>> +    dsp->compute_weights_line = compute_weights_line_c;
> >>>> +
> >>>> +    if (ARCH_AARCH64)
> >>>> +        ff_nlmeans_init_aarch64(dsp);
> >>>
> >>> Hi Andreas,
> >>>
> >>> the above breaks compilation for me:
> >>>
> >>> 1>libavfilterd.lib(libavfilter_vf_nlmeans.obj) : error LNK2019:
> >> unresolved external symbol ff_nlmeans_init_aarch64 referenced in
> >> function ff_nlmeans_init
> >>>
> >>> The reason is that I'm (obviously) not compiling stuff from the
> >>> libavfilter\aarch64 subfolder.
> >>>
> >>> It might need an #ifdef ?
> >>>
> >>> I haven't taken a deeper look at it, though.
> >>>
> >>> Thanks,
> >>> softworkz
> >>>
> >>>
> >>
> >> That surprises me: The earlier code did exactly the same; in fact,
> >> using
> >> if (ARCH_*) is our typical check for arches in dsp-init code.
> >
> > I looked at this a bit further. It seems that the VS project
> > generation tool that I'm using is creating dummy definitions
> > for such cases. In the previous workspace it had generated
> >
> > void ff_nlmeans_init_aarch64(NLMeansDSPContext *dsp) {return;}
> >
> > in a separate code file for being able to work with the ffmpeg
> > code in VS without modifying any of the code.
> >
> > Now that you have moved that code to a header file, this logic
> > doesn't work anymore.
> >
> 
> Why does your compiler not just eliminate dead code like all the
> others?
> Is this MSVC -O0 behaviour?
> I just sent the patch
> https://ffmpeg.org/pipermail/ffmpeg-devel/2022-May/296380.html, but
> now
> that I read this I have to agree with Hendrik: Why not update your
> tool
> instead?

I'm using https://github.com/ShiftMediaProject/FFVS-Project-Generator
for years and I'm sure he will adapt.

When I had posted, the whole situation wasn't clear to me.

Thanks a lot for the patch!

softworkz
Soft Works May 13, 2022, 9:34 a.m. UTC | #10 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Hendrik Leppkes
> Sent: Friday, May 13, 2022 11:27 AM
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> ff_nlmeans_init into a header
> 
> On Fri, May 13, 2022 at 11:26 AM Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
> >
> > Soft Works:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> > >> Sent: Friday, May 13, 2022 10:27 AM
> > >> To: Soft Works <softworkz@hotmail.com>; FFmpeg development
> discussions
> > >> and patches <ffmpeg-devel@ffmpeg.org>
> > >> Subject: Re: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans:
> Move
> > >> ff_nlmeans_init into a header
> > >>
> > >> Soft Works:
> > >>>
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf
> Of
> > >>>> Andreas Rheinhardt
> > >>>> Sent: Tuesday, May 3, 2022 8:38 AM
> > >>>> To: ffmpeg-devel@ffmpeg.org
> > >>>> Cc: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> > >>>> Subject: [FFmpeg-devel] [PATCH 07/10] avfilter/vf_nlmeans: Move
> > >>>> ff_nlmeans_init into a header
> > >>>>
> > >>>> This removes a dependency of checkasm on lavfi/vf_nlmeans.o
> > >>>> and also allows to inline ff_nlmeans_init() irrespectively of
> > >>>> interposing.
> > >>>>
> > >>>> Signed-off-by: Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com>
> > >>>> ---
> > >>>
> > >>> [..]
> > >>>
> > >>>> +
> > >>>> +static av_unused void ff_nlmeans_init(NLMeansDSPContext *dsp)
> > >>>> +{
> > >>>> +    dsp->compute_safe_ssd_integral_image =
> > >>>> compute_safe_ssd_integral_image_c;
> > >>>> +    dsp->compute_weights_line = compute_weights_line_c;
> > >>>> +
> > >>>> +    if (ARCH_AARCH64)
> > >>>> +        ff_nlmeans_init_aarch64(dsp);
> > >>>
> > >>> Hi Andreas,
> > >>>
> > >>> the above breaks compilation for me:
> > >>>
> > >>> 1>libavfilterd.lib(libavfilter_vf_nlmeans.obj) : error LNK2019:
> > >> unresolved external symbol ff_nlmeans_init_aarch64 referenced in
> > >> function ff_nlmeans_init
> > >>>
> > >>> The reason is that I'm (obviously) not compiling stuff from the
> > >>> libavfilter\aarch64 subfolder.
> > >>>
> > >>> It might need an #ifdef ?
> > >>>
> > >>> I haven't taken a deeper look at it, though.
> > >>>
> > >>> Thanks,
> > >>> softworkz
> > >>>
> > >>>
> > >>
> > >> That surprises me: The earlier code did exactly the same; in
> fact,
> > >> using
> > >> if (ARCH_*) is our typical check for arches in dsp-init code.
> > >
> > > I looked at this a bit further. It seems that the VS project
> > > generation tool that I'm using is creating dummy definitions
> > > for such cases. In the previous workspace it had generated
> > >
> > > void ff_nlmeans_init_aarch64(NLMeansDSPContext *dsp) {return;}
> > >
> > > in a separate code file for being able to work with the ffmpeg
> > > code in VS without modifying any of the code.
> > >
> > > Now that you have moved that code to a header file, this logic
> > > doesn't work anymore.
> > >
> >
> > Why does your compiler not just eliminate dead code like all the
> others?
> > Is this MSVC -O0 behaviour?
> 
> 
> MSVC does not do DCE when optimizations are disabled, yes. So this is
> where this is coming from.
> 
> > I just sent the patch
> > https://ffmpeg.org/pipermail/ffmpeg-devel/2022-May/296380.html, but
> now
> > that I read this I have to agree with Hendrik: Why not update your
> tool
> > instead?
> >
> 
> Actually looking at the tool, it seems like it handles header files.
> Did you actually re-run the generation tool after applying this patch?

Today I pulled HEAD and ran the tool (after I had pulled the latest 
version of the tool and compiled it). It didn't detect the header 
definition. That's why I wrote to Andreas.

Thanks a lot for your help!

softworkz
diff mbox series

Patch

diff --git a/libavfilter/vf_nlmeans.c b/libavfilter/vf_nlmeans.c
index 8a05965c9b..2fc3adacca 100644
--- a/libavfilter/vf_nlmeans.c
+++ b/libavfilter/vf_nlmeans.c
@@ -36,6 +36,7 @@ 
 #include "formats.h"
 #include "internal.h"
 #include "vf_nlmeans.h"
+#include "vf_nlmeans_init.h"
 #include "video.h"
 
 typedef struct NLMeansContext {
@@ -84,48 +85,6 @@  static const enum AVPixelFormat pix_fmts[] = {
     AV_PIX_FMT_NONE
 };
 
-/**
- * Compute squared difference of the safe area (the zone where s1 and s2
- * overlap). It is likely the largest integral zone, so it is interesting to do
- * as little checks as possible; contrary to the unsafe version of this
- * function, we do not need any clipping here.
- *
- * The line above dst and the column to its left are always readable.
- */
-static void compute_safe_ssd_integral_image_c(uint32_t *dst, ptrdiff_t dst_linesize_32,
-                                              const uint8_t *s1, ptrdiff_t linesize1,
-                                              const uint8_t *s2, ptrdiff_t linesize2,
-                                              int w, int h)
-{
-    const uint32_t *dst_top = dst - dst_linesize_32;
-
-    /* SIMD-friendly assumptions allowed here */
-    av_assert2(!(w & 0xf) && w >= 16 && h >= 1);
-
-    for (int y = 0; y < h; y++) {
-        for (int x = 0; x < w; x += 4) {
-            const int d0 = s1[x    ] - s2[x    ];
-            const int d1 = s1[x + 1] - s2[x + 1];
-            const int d2 = s1[x + 2] - s2[x + 2];
-            const int d3 = s1[x + 3] - s2[x + 3];
-
-            dst[x    ] = dst_top[x    ] - dst_top[x - 1] + d0*d0;
-            dst[x + 1] = dst_top[x + 1] - dst_top[x    ] + d1*d1;
-            dst[x + 2] = dst_top[x + 2] - dst_top[x + 1] + d2*d2;
-            dst[x + 3] = dst_top[x + 3] - dst_top[x + 2] + d3*d3;
-
-            dst[x    ] += dst[x - 1];
-            dst[x + 1] += dst[x    ];
-            dst[x + 2] += dst[x + 1];
-            dst[x + 3] += dst[x + 2];
-        }
-        s1  += linesize1;
-        s2  += linesize2;
-        dst += dst_linesize_32;
-        dst_top += dst_linesize_32;
-    }
-}
-
 /**
  * Compute squared difference of an unsafe area (the zone nor s1 nor s2 could
  * be readable).
@@ -326,59 +285,6 @@  struct thread_data {
     int p;
 };
 
-static void compute_weights_line_c(const uint32_t *const iia,
-                                   const uint32_t *const iib,
-                                   const uint32_t *const iid,
-                                   const uint32_t *const iie,
-                                   const uint8_t *const src,
-                                   float *total_weight,
-                                   float *sum,
-                                   const float *const weight_lut,
-                                   int max_meaningful_diff,
-                                   int startx, int endx)
-{
-    for (int x = startx; x < endx; x++) {
-        /*
-         * M is a discrete map where every entry contains the sum of all the entries
-         * in the rectangle from the top-left origin of M to its coordinate. In the
-         * following schema, "i" contains the sum of the whole map:
-         *
-         * M = +----------+-----------------+----+
-         *     |          |                 |    |
-         *     |          |                 |    |
-         *     |         a|                b|   c|
-         *     +----------+-----------------+----+
-         *     |          |                 |    |
-         *     |          |                 |    |
-         *     |          |        X        |    |
-         *     |          |                 |    |
-         *     |         d|                e|   f|
-         *     +----------+-----------------+----+
-         *     |          |                 |    |
-         *     |         g|                h|   i|
-         *     +----------+-----------------+----+
-         *
-         * The sum of the X box can be calculated with:
-         *    X = e-d-b+a
-         *
-         * See https://en.wikipedia.org/wiki/Summed_area_table
-         *
-         * The compute*_ssd functions compute the integral image M where every entry
-         * contains the sum of the squared difference of every corresponding pixels of
-         * two input planes of the same size as M.
-         */
-        const uint32_t a = iia[x];
-        const uint32_t b = iib[x];
-        const uint32_t d = iid[x];
-        const uint32_t e = iie[x];
-        const uint32_t patch_diff_sq = FFMIN(e - d - b + a, max_meaningful_diff);
-        const float weight = weight_lut[patch_diff_sq]; // exp(-patch_diff_sq * s->pdiff_scale)
-
-        total_weight[x] += weight;
-        sum[x] += weight * src[x];
-    }
-}
-
 static int nlmeans_slice(AVFilterContext *ctx, void *arg, int jobnr, int nb_jobs)
 {
     NLMeansContext *s = ctx->priv;
@@ -512,18 +418,6 @@  static int filter_frame(AVFilterLink *inlink, AVFrame *in)
     }                                                           \
 } while (0)
 
-void ff_nlmeans_init(NLMeansDSPContext *dsp)
-{
-    dsp->compute_safe_ssd_integral_image = compute_safe_ssd_integral_image_c;
-    dsp->compute_weights_line = compute_weights_line_c;
-
-    if (ARCH_AARCH64)
-        ff_nlmeans_init_aarch64(dsp);
-
-    if (ARCH_X86)
-        ff_nlmeans_init_x86(dsp);
-}
-
 static av_cold int init(AVFilterContext *ctx)
 {
     NLMeansContext *s = ctx->priv;
diff --git a/libavfilter/vf_nlmeans.h b/libavfilter/vf_nlmeans.h
index 43611a03bd..61377f8c69 100644
--- a/libavfilter/vf_nlmeans.h
+++ b/libavfilter/vf_nlmeans.h
@@ -39,7 +39,6 @@  typedef struct NLMeansDSPContext {
                                  int startx, int endx);
 } NLMeansDSPContext;
 
-void ff_nlmeans_init(NLMeansDSPContext *dsp);
 void ff_nlmeans_init_aarch64(NLMeansDSPContext *dsp);
 void ff_nlmeans_init_x86(NLMeansDSPContext *dsp);
 
diff --git a/libavfilter/vf_nlmeans_init.h b/libavfilter/vf_nlmeans_init.h
new file mode 100644
index 0000000000..04ad8801b6
--- /dev/null
+++ b/libavfilter/vf_nlmeans_init.h
@@ -0,0 +1,139 @@ 
+/*
+ * Copyright (c) 2016 Clément Bœsch <u pkh me>
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#ifndef AVFILTER_NLMEANS_INIT_H
+#define AVFILTER_NLMEANS_INIT_H
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include "config.h"
+#include "libavutil/avassert.h"
+#include "libavutil/macros.h"
+#include "vf_nlmeans.h"
+
+/**
+ * Compute squared difference of the safe area (the zone where s1 and s2
+ * overlap). It is likely the largest integral zone, so it is interesting to do
+ * as little checks as possible; contrary to the unsafe version of this
+ * function, we do not need any clipping here.
+ *
+ * The line above dst and the column to its left are always readable.
+ */
+static void compute_safe_ssd_integral_image_c(uint32_t *dst, ptrdiff_t dst_linesize_32,
+                                              const uint8_t *s1, ptrdiff_t linesize1,
+                                              const uint8_t *s2, ptrdiff_t linesize2,
+                                              int w, int h)
+{
+    const uint32_t *dst_top = dst - dst_linesize_32;
+
+    /* SIMD-friendly assumptions allowed here */
+    av_assert2(!(w & 0xf) && w >= 16 && h >= 1);
+
+    for (int y = 0; y < h; y++) {
+        for (int x = 0; x < w; x += 4) {
+            const int d0 = s1[x    ] - s2[x    ];
+            const int d1 = s1[x + 1] - s2[x + 1];
+            const int d2 = s1[x + 2] - s2[x + 2];
+            const int d3 = s1[x + 3] - s2[x + 3];
+
+            dst[x    ] = dst_top[x    ] - dst_top[x - 1] + d0*d0;
+            dst[x + 1] = dst_top[x + 1] - dst_top[x    ] + d1*d1;
+            dst[x + 2] = dst_top[x + 2] - dst_top[x + 1] + d2*d2;
+            dst[x + 3] = dst_top[x + 3] - dst_top[x + 2] + d3*d3;
+
+            dst[x    ] += dst[x - 1];
+            dst[x + 1] += dst[x    ];
+            dst[x + 2] += dst[x + 1];
+            dst[x + 3] += dst[x + 2];
+        }
+        s1  += linesize1;
+        s2  += linesize2;
+        dst += dst_linesize_32;
+        dst_top += dst_linesize_32;
+    }
+}
+
+static void compute_weights_line_c(const uint32_t *const iia,
+                                   const uint32_t *const iib,
+                                   const uint32_t *const iid,
+                                   const uint32_t *const iie,
+                                   const uint8_t *const src,
+                                   float *total_weight,
+                                   float *sum,
+                                   const float *const weight_lut,
+                                   int max_meaningful_diff,
+                                   int startx, int endx)
+{
+    for (int x = startx; x < endx; x++) {
+        /*
+         * M is a discrete map where every entry contains the sum of all the entries
+         * in the rectangle from the top-left origin of M to its coordinate. In the
+         * following schema, "i" contains the sum of the whole map:
+         *
+         * M = +----------+-----------------+----+
+         *     |          |                 |    |
+         *     |          |                 |    |
+         *     |         a|                b|   c|
+         *     +----------+-----------------+----+
+         *     |          |                 |    |
+         *     |          |                 |    |
+         *     |          |        X        |    |
+         *     |          |                 |    |
+         *     |         d|                e|   f|
+         *     +----------+-----------------+----+
+         *     |          |                 |    |
+         *     |         g|                h|   i|
+         *     +----------+-----------------+----+
+         *
+         * The sum of the X box can be calculated with:
+         *    X = e-d-b+a
+         *
+         * See https://en.wikipedia.org/wiki/Summed_area_table
+         *
+         * The compute*_ssd functions compute the integral image M where every entry
+         * contains the sum of the squared difference of every corresponding pixels of
+         * two input planes of the same size as M.
+         */
+        const uint32_t a = iia[x];
+        const uint32_t b = iib[x];
+        const uint32_t d = iid[x];
+        const uint32_t e = iie[x];
+        const uint32_t patch_diff_sq = FFMIN(e - d - b + a, max_meaningful_diff);
+        const float weight = weight_lut[patch_diff_sq]; // exp(-patch_diff_sq * s->pdiff_scale)
+
+        total_weight[x] += weight;
+        sum[x] += weight * src[x];
+    }
+}
+
+static av_unused void ff_nlmeans_init(NLMeansDSPContext *dsp)
+{
+    dsp->compute_safe_ssd_integral_image = compute_safe_ssd_integral_image_c;
+    dsp->compute_weights_line = compute_weights_line_c;
+
+    if (ARCH_AARCH64)
+        ff_nlmeans_init_aarch64(dsp);
+
+    if (ARCH_X86)
+        ff_nlmeans_init_x86(dsp);
+}
+
+#endif /* AVFILTER_NLMEANS_INIT_H */
diff --git a/tests/checkasm/vf_nlmeans.c b/tests/checkasm/vf_nlmeans.c
index 87474d6803..0f1f9fd403 100644
--- a/tests/checkasm/vf_nlmeans.c
+++ b/tests/checkasm/vf_nlmeans.c
@@ -19,7 +19,7 @@ 
  */
 
 #include "checkasm.h"
-#include "libavfilter/vf_nlmeans.h"
+#include "libavfilter/vf_nlmeans_init.h"
 #include "libavutil/avassert.h"
 
 #define randomize_buffer(buf, size) do {    \