[RFC,1/4] libcamera: swstats_cpu: Update statsProcessFn() / processLine0() documentation

21572 diff mbox series

Update the documentation of the statsProcessFn() / processLine0() src[]
pointer argument to take into account that swstats_cpu may also be used
with planar input data or with non Bayer single plane input data.

The statsProcessFn typedef is private, so no documentation is generated
for it. Move the new updated src[] pointer argument documentation to
processLine0() so that it gets included in the generated docs.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 src/libcamera/software_isp/swstats_cpu.cpp | 27 +++++++++++-----------
 1 file changed, 13 insertions(+), 14 deletions(-)

Quoting Hans de Goede (2024-10-09 21:01:07)
> Update the documentation of the statsProcessFn() / processLine0() src[]
> pointer argument to take into account that swstats_cpu may also be used
> with planar input data or with non Bayer single plane input data.
> 
> The statsProcessFn typedef is private, so no documentation is generated
> for it. Move the new updated src[] pointer argument documentation to
> processLine0() so that it gets included in the generated docs.
> 

This also seems reasonable on it's own.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  src/libcamera/software_isp/swstats_cpu.cpp | 27 +++++++++++-----------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
> index c520c806..a9a3e77a 100644
> --- a/src/libcamera/software_isp/swstats_cpu.cpp
> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
> @@ -71,6 +71,19 @@ namespace libcamera {
>   * patternSize height == 1.
>   * It'll process line 0 and 1 for input formats with patternSize height >= 2.
>   * This function may only be called after a successful setWindow() call.
> + *
> + * This function takes an array of src pointers each pointing to a line in
> + * the source image.
> + *
> + * Bayer input data requires (patternSize_.height + 1) src pointers, with
> + * the middle element of the array pointing to the actual line being processed.
> + * Earlier element(s) will point to the previous line(s) and later element(s)
> + * to the next line(s). See the DebayerCpu::debayerFn documentation for details.
> + *
> + * Planar input data requires a src pointer for each plane, with src[0] pointing
> + * to the line in plane 0, etc.
> + *
> + * For non Bayer single plane input data only a single src pointer is required.
>   */
>  
>  /**
> @@ -89,20 +102,6 @@ namespace libcamera {
>   * \brief Signals that the statistics are ready
>   */
>  
> -/**
> - * \typedef SwStatsCpu::statsProcessFn
> - * \brief Called when there is data to get statistics from
> - * \param[in] src The input data
> - *
> - * These functions take an array of (patternSize_.height + 1) src
> - * pointers each pointing to a line in the source image. The middle
> - * element of the array will point to the actual line being processed.
> - * Earlier element(s) will point to the previous line(s) and later
> - * element(s) to the next line(s).
> - *
> - * See the documentation of DebayerCpu::debayerFn for more details.
> - */
> -
>  /**
>   * \var unsigned int SwStatsCpu::ySkipMask_
>   * \brief Skip lines where this bitmask is set in y
> -- 
> 2.46.2
>

Hans de Goede <hdegoede@redhat.com> writes:

> Update the documentation of the statsProcessFn() / processLine0() src[]
> pointer argument to take into account that swstats_cpu may also be used
> with planar input data or with non Bayer single plane input data.
>
> The statsProcessFn typedef is private, so no documentation is generated
> for it. Move the new updated src[] pointer argument documentation to
> processLine0() so that it gets included in the generated docs.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  src/libcamera/software_isp/swstats_cpu.cpp | 27 +++++++++++-----------
>  1 file changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
> index c520c806..a9a3e77a 100644
> --- a/src/libcamera/software_isp/swstats_cpu.cpp
> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
> @@ -71,6 +71,19 @@ namespace libcamera {
>   * patternSize height == 1.
>   * It'll process line 0 and 1 for input formats with patternSize height >= 2.
>   * This function may only be called after a successful setWindow() call.
> + *
> + * This function takes an array of src pointers each pointing to a line in
> + * the source image.
> + *
> + * Bayer input data requires (patternSize_.height + 1) src pointers, with
> + * the middle element of the array pointing to the actual line being processed.
> + * Earlier element(s) will point to the previous line(s) and later element(s)
> + * to the next line(s). See the DebayerCpu::debayerFn documentation for details.
> + *
> + * Planar input data requires a src pointer for each plane, with src[0] pointing
> + * to the line in plane 0, etc.

I'm having trouble to be comfortable with the overloading.  It's true
that the method implementation doesn't really care about the meaning of
`src' contents and stats0_ signature is satisfied.  I'm just nervous
about making something that's already not very clear even more
complicated.

Since I don't have a better suggestion at the moment:

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> + *
> + * For non Bayer single plane input data only a single src pointer is required.
>   */
>  
>  /**
> @@ -89,20 +102,6 @@ namespace libcamera {
>   * \brief Signals that the statistics are ready
>   */
>  
> -/**
> - * \typedef SwStatsCpu::statsProcessFn
> - * \brief Called when there is data to get statistics from
> - * \param[in] src The input data
> - *
> - * These functions take an array of (patternSize_.height + 1) src
> - * pointers each pointing to a line in the source image. The middle
> - * element of the array will point to the actual line being processed.
> - * Earlier element(s) will point to the previous line(s) and later
> - * element(s) to the next line(s).
> - *
> - * See the documentation of DebayerCpu::debayerFn for more details.
> - */
> -
>  /**
>   * \var unsigned int SwStatsCpu::ySkipMask_
>   * \brief Skip lines where this bitmask is set in y