Re: [Rapp-dev] Pixelwise threshold and conditional pixel operations pathes

[Hans-Peter Nilsson]

> From: Willie Betschart <address@hidden>
> Date: Wed, 4 May 2016 08:54:01 +0000

> Content-Type: text/x-patch; name="thresh-pixelv2.patch"

Thanks, I've committed this, with no functional change. There
were some minor changes besides omitting the unwanted
reformatting.  (Comments also are now complete sentences and
there's less vertical alignment in indentation.)

Just for the record:

> --- a/benchmark/arch/benchmarkplot-x86_64-gnu-sse2.html

(Not actually useful for *this* common platform.)

> diff --git a/benchmark/rapp_benchmark.c b/benchmark/rapp_benchmark.c

+        /* The speed is not dependent of the content or calculation results
+         * so the aux buffer is reused for both high and low thresholds.
+         * This minimize changes of the entire benchmark test,
+           i.e. only require a single aux buffer. */

It made more sense to actually add another aux buffer and drop
the need to excuse oneself in a macro and adding an soon-to-be
anachronism (referring to a "change" is soon lost in time, just
like using "new" in a name).  Also, the formatting wasn't canon. :)
Same for compute/tune/benchmark/rc_benchmark.c.

> diff --git a/compute/generic/rc_template.h b/compute/generic/rc_template.h
> deleted file mode 100644

> diff --git a/compute/generic/rc_thresh_pixel_tpl.h 
> b/compute/generic/rc_thresh_pixel_tpl.h
> new file mode 100644

This renaming of a mostly-unrelated file confused me at first,
but I then realised that this was a *cleanup* to align the
generic header files with the general naming scheme of header
files; that the name was overly generalized for a header files
containing only thresholding macros.  Sending the cleanup as a
separate patch would have helped the review.

> diff --git a/compute/generic/rc_thresh.c b/compute/generic/rc_thresh.c

> @@ -82,7 +84,7 @@ rc_thresh_gt_u8(uint8_t *restrict dst, int dst_dim,
>                  const uint8_t *restrict src, int src_dim,
>                  int width, int height, int thresh)
>  {
> -    RC_TEMPLATE_THRESH(dst, dst_dim, src, src_dim, width, height,
> +    RC_THRESH_TEMPLATE(dst, dst_dim, src, src_dim, width, height,
>                         thresh, 0, RC_THRESH_CMPGT,
>                         RC_UNROLL(rc_thresh_gt_u8));
>  }

(Unrelated naming change as part of the cleanup.  Confusing to
see this in what's supposedly just additional functionality.)

> +#if RC_IMPL(rc_thresh_gtlt_pixel_u8, 1)
> +void
> +rc_thresh_gtlt_pixel_u8(uint8_t *restrict dst, int dst_dim,
> +                        const uint8_t *restrict src, int src_dim,
> +                        const uint8_t *restrict low, int low_dim,
> +                        const uint8_t *restrict high, int high_dim,
> +                        int width, int height)
> +{
> +    RC_THRESH_PIXEL_TEMPLATE(dst, dst_dim, src, src_dim,
> +                             low, low_dim, high, high_dim,
> +                             width, height, RC_THRESH_CMPGTLT,
> +                             RC_THRESH_PIXEL_DOUBLE_ARG,
> +                             RC_UNROLL(rc_thresh_gtlt_pixel_u8));

That RC_THRESH_PIXEL_DOUBLE/SINGLE_ARG with an additional
separate unused NULL argument for single-operand operations is a
bit too ugly.  I wrapped this in the rc_thresh_pixel_tpl.h
header with separate RC_THRESH_PIXEL_TEMPLATE_SINGLE and
RC_THRESH_PIXEL_TEMPLATE_DOUBLE macros instead.  While we do
have one-or-two arguments both handled in a single macro before,
those cases only have a single used/unused scalar argument and
not a used/unused separate memory source.

> +rc_thresh_gt_pixel_u8(uint8_t *restrict dst, int dst_dim,
> +                      const uint8_t *restrict src, int src_dim,
> +                      const uint8_t *restrict thresh, int thresh_dim,
> +                      int width, int height)

So, not:

> +{
> +    const uint8_t *thresh_high = NULL;
> +    RC_THRESH_PIXEL_TEMPLATE(dst, dst_dim, src, src_dim,
> +                             thresh, thresh_dim, thresh_high, 0,
> +                             width, height, RC_THRESH_CMPGT,
> +                             RC_THRESH_PIXEL_SINGLE_ARG,
> +                             RC_UNROLL(rc_thresh_gt_pixel_u8));
> +}

but instead (no single_arg and unused memory pointer/dim argument):
{
    RC_THRESH_PIXEL_TEMPLATE_SINGLE(dst, dst_dim, src, src_dim,
                                    thresh, thresh_dim,
                                    width, height, RC_THRESH_CMPGT,
                                    RC_UNROLL(rc_thresh_gt_pixel_u8));
}

That's only a levelling detail wrapping in another macro; they
expand to the same macro in the header.  Similarly for the
vector implementation.

> diff --git a/compute/tune/benchmark/rc_benchmark.c 
> b/compute/tune/benchmark/rc_benchmark.c

> diff --git a/include/rapp_thresh.h b/include/rapp_thresh.h
> index c6948c8..88133c3 100644
> --- a/include/rapp_thresh.h
> +++ b/include/rapp_thresh.h

It almost always makes sense to update the head comment too, so
I did that.

mvh H-P