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

[Willie Betschart]

Dear Savannah!


The pixelwise threshold operations is updated with width and height as last 
arguments. I complemented with Greater than, Greater than AND Less than and 
Greater than​ OR Less than operations as well.


I have updated the conditional pixop patch again since I found some shadowed 
variables, otherwise it is the same.


I attach a set of tune and benchmark files from available platforms. These are 
combined with all new functions.


Best wishes

Willie Betschart


________________________________
Från: Willie Betschart
Skickat: den 26 april 2016 13:00
Till: Johan Almbladh
Kopia: address@hidden
Ämne: SV: [Rapp-dev] Pixelwise threshold and conditional pixel operations pathes


Hello Johan!


I have updated the conditional pixop patch with underscore declarations in 
[vector/generic]/rc_cond.c. I think these were the ones you refer to?


Best regards

Willie


________________________________
Från: Willie Betschart
Skickat: den 25 april 2016 12:52
Till: Johan Almbladh
Kopia: address@hidden
Ämne: SV: [Rapp-dev] Pixelwise threshold and conditional pixel operations pathes


Hello Johan


Thanks for a quick response!


There's a reason why the threshold buffer and dim is declared last, it is 
considered as arguments as the traditional threshold functions and how 
rapp_pixop_lut_u8 declares the lut buffer. The difference to lut is the dim 
parameter. Currently we didn't had any need for two buffer thresholds as 
rapp_thresh_gtlt_pixel_u8 even if the tests are designed to handle it. The idea 
is that it should look like this:


RAPP_API(int, rapp_thresh_gtlt_pixel_u8,
         (uint8_t *restrict dst, int dst_dim,
         const uint8_t *restrict src, int src_dim,
         int width, int height,
         const uint8_t *restrict thresh_low, int low_dim,

         const uint8_t *restrict thresh_high, int high_dim));


Since we consider it as arguments, it became natural to put the buffers last to 
be consistent with other functions with arguments and it also simplifies the 
use of function pointers (as the test's does).


    "Do you see a need for more advanced conditional operations? In particular, 
I think it would make a lot of sense to provide conditional versions of the     
pixel-wise operations in general. This would mean conditional versions of all 
pixop functions. Those variants would then accept a binary selection mask     
as an extra input buffer. The implementation would be very straight-forward 
using the macro template mechanism with driver + operation, but it will     
contribute to a larger library code size of course."


This was only a matter of priority and current demands. If time allows all 
existing pixops could be created as conditional pixops. The structure is 
already there. The same motivation applies to the threshold_[cmp]_pixel_u8 
operations. Would you accept the proposed functions until then?


    "I also recall that the template macros name their internal variables with 
a trailing underscore to minimize the risk of clashes. If this is done 
consistently     in other parts, consider changing your template macros to 
follow this convention."


Yes I totally agree, I will update this.


Best regards

Willie



________________________________
Från: Johan Almbladh <address@hidden>
Skickat: den 23 april 2016 09:09
Till: Willie Betschart
Kopia: address@hidden
Ämne: Re: [Rapp-dev] Pixelwise threshold and conditional pixel operations pathes

Hi Willie,

Thank you for the patches. They look very solid, but I have a few minor 
comments below.

+RAPP_API(int, rapp_thresh_lt_pixel_u8,
+        (uint8_t *restrict dst, int dst_dim,
+         const uint8_t *restrict src, int src_dim,
+         int width, int height,
+         const uint8_t *restrict thresh, int thresh_dim));

+RC_EXPORT void
+rc_thresh_lt_pixel_u8(uint8_t *restrict dst, int dst_dim,
+                      const uint8_t *restrict src, int src_dim,
+                      int width, int height,
+                      const uint8_t *restrict thresh, int thresh_dim);

I believe the function arguments in the library are ordered according to the 
following convention.

1. Output buffer(s): dst, dst_dim, ...
2. Input buffer(s): src, src_dim, ...
3. Image width in pixels: width
4. Image height in pixels: height
5. Any additional parameters

For consistency, I would rather see that the threshold buffer specification 
follows after the source buffer spec.

I also recall that the template macros name their internal variables with a 
trailing underscore to minimize the risk of clashes. If this is done 
consistently in other parts, consider changing your template macros to follow 
this convention.

Why not using gather scatter instead? The reason is dependent on choice of use 
case rather than a speed competitor. The conditional pixop is used when a 
selection of pixels in an image should be updated with a single instruction and 
then you continue to do processing on the same image.

I agree. There is definitely a tradeoff here. If the processed subset is very 
sparse and/or you want to do several operations on this subset, then it makes 
sense to perform data reduction via gather/scatter followed by processing in 
the reduced domain. On the other hand, if the operation is only a single pixop 
that is to be applied selectively, it is more efficient to do this in one pass.

The original implementation provided the gather/scatter mechanism as a general 
way of dealing with sparse images. In this model, the problem of selecting data 
was separated from the problem of processing data. Since the pixels in the 
reduced domain formed a valid image, all processing operations could be applied 
in this domain as well.

As you point out, this separation has an overhead. For small operations it 
makes sense to provide a composite operation that accepts a binary selection 
mask. RAPP originally provided conditional set and copy operations only, and 
now you have extended that with addc as well.

Do you see a need for more advanced conditional operations? In particular, I 
think it would make a lot of sense to provide conditional versions of the 
pixel-wise operations in general. This would mean conditional versions of all 
pixop functions. Those variants would then accept a binary selection mask as an 
extra input buffer. The implementation would be very straight-forward using the 
macro template mechanism with driver + operation, but it will contribute to a 
larger library code size of course.

What do you think?

Best regards,
Johan

thresh-pixelv2.patch
Description: thresh-pixelv2.patch

cond-pixop-add-v3.patch
Description: cond-pixop-add-v3.patch

tuning.tar.gz
Description: tuning.tar.gz