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

[Johan Almbladh]

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