[libcamera-devel] [PATCH v2 4/5] apply clang-format style
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 5 18:36:19 CEST 2022
Hi Christian,
Thank you for the patch.
On Tue, Apr 05, 2022 at 01:42:14AM +0100, Christian Rauch via libcamera-devel wrote:
> Signed-off-by: Christian Rauch <Rauch.Christian at gmx.de>
> ---
> include/libcamera/base/span.h | 45 +++---
> src/ipa/raspberrypi/raspberrypi.cpp | 3 +-
> .../pipeline/raspberrypi/raspberrypi.cpp | 34 +++--
> src/qcam/dng_writer.cpp | 140 +++++++++---------
> test/span.cpp | 4 +-
> 5 files changed, 116 insertions(+), 110 deletions(-)
>
> diff --git a/include/libcamera/base/span.h b/include/libcamera/base/span.h
> index bff4c115..b2fdd5fb 100644
> --- a/include/libcamera/base/span.h
> +++ b/include/libcamera/base/span.h
> @@ -124,7 +124,7 @@ public:
> constexpr Span(element_type (&arr)[N],
> std::enable_if_t<std::is_convertible<std::remove_pointer_t<decltype(utils::data(arr))> (*)[],
> element_type (*)[]>::value &&
> - N == Extent,
> + N == Extent,
clang-format is unfortunately not able to handle all the details of the
coding style, we can't express the alignment we would like here. I find
the existing alignment more readable (provided complex templates can
even be considered as readable :-)).
Same for most locations below, I've only kept the chunks for which I
have specific comments.
> std::nullptr_t> = nullptr) noexcept
> : data_(arr)
> {
[snip]
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 1bf4e270..926b3185 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -22,11 +22,12 @@
> #include <libcamera/control_ids.h>
> #include <libcamera/controls.h>
> #include <libcamera/framebuffer.h>
> +#include <libcamera/request.h>
> +
> #include <libcamera/ipa/ipa_interface.h>
> #include <libcamera/ipa/ipa_module_info.h>
> #include <libcamera/ipa/raspberrypi.h>
> #include <libcamera/ipa/raspberrypi_ipa_interface.h>
> -#include <libcamera/request.h>
This one is a good change.
>
> #include "libcamera/internal/mapped_framebuffer.h"
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 8fd79be6..34d9f4c4 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -10,26 +10,26 @@
> #include <fcntl.h>
> #include <memory>
> #include <mutex>
> -#include <queue>
That's a false positive, likely caused by the rule that tries to handle
Qt headers.
> #include <unordered_set>
> #include <utility>
>
> +#include <linux/bcm2835-isp.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/videodev2.h>
> +
> #include <libcamera/base/shared_fd.h>
> #include <libcamera/base/utils.h>
>
> #include <libcamera/camera.h>
> #include <libcamera/control_ids.h>
> #include <libcamera/formats.h>
> -#include <libcamera/ipa/raspberrypi.h>
> -#include <libcamera/ipa/raspberrypi_ipa_interface.h>
> -#include <libcamera/ipa/raspberrypi_ipa_proxy.h>
> #include <libcamera/logging.h>
> #include <libcamera/property_ids.h>
> #include <libcamera/request.h>
>
> -#include <linux/bcm2835-isp.h>
> -#include <linux/media-bus-format.h>
> -#include <linux/videodev2.h>
> +#include <libcamera/ipa/raspberrypi.h>
> +#include <libcamera/ipa/raspberrypi_ipa_interface.h>
> +#include <libcamera/ipa/raspberrypi_ipa_proxy.h>
The rest is good.
>
> #include "libcamera/internal/bayer_format.h"
> #include "libcamera/internal/camera.h"
[snip]
> @@ -544,7 +553,6 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>
> cfg.stride = format.planes[0].bpl;
> cfg.frameSize = format.planes[0].size;
> -
This is good too.
> }
>
> return status;
> @@ -651,8 +659,8 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> PixelFormat pf = mbusCodeToPixelFormat(format.first,
> BayerFormat::Packing::CSI2);
> if (pf.isValid())
> - deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf),
> - std::forward_as_tuple(format.second.begin(), format.second.end()));
> + deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf),
> + std::forward_as_tuple(format.second.begin(), format.second.end()));
While at it, you can write
deviceFormats.emplace(std::piecewise_construct,
std::forward_as_tuple(pf),
std::forward_as_tuple(format.second.begin(),
format.second.end()));
> }
> } else {
> /*
> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
> index 34c8df5a..a7dd30f8 100644
> --- a/src/qcam/dng_writer.cpp
> +++ b/src/qcam/dng_writer.cpp
> @@ -11,12 +11,12 @@
> #include <iostream>
> #include <map>
>
> -#include <tiffio.h>
> -
> #include <libcamera/control_ids.h>
> #include <libcamera/formats.h>
> #include <libcamera/property_ids.h>
>
> +#include <tiffio.h>
> +
Fine with me.
> using namespace libcamera;
>
> enum CFAPatternColour : uint8_t {
> @@ -201,7 +201,7 @@ void packScanlineIPU3(void *output, const void *input, unsigned int width)
> if (++x >= width)
> return;
>
> - *out++ = (in[4] & 0xff) << 8 | (in[3] & 0xc0) << 0;
> + *out++ = (in[4] & 0xff) << 8 | (in[3] & 0xc0) << 0;
Looks good too.
> if (++x >= width)
> return;
>
[snip]
> @@ -254,14 +253,14 @@ void thumbScanlineIPU3([[maybe_unused]] const FormatInfo &info, void *output,
> break;
> case 2:
> val1 = (in[3] & 0x3f) << 10 | (in[2] & 0xf0) << 2;
> - val2 = (in[4] & 0xff) << 8 | (in[3] & 0xc0) << 0;
> + val2 = (in[4] & 0xff) << 8 | (in[3] & 0xc0) << 0;
This was done on purpose for alignment reasons, I'd keep it as-is. Same
below.
> val3 = (in[stride + 3] & 0x3f) << 10 | (in[stride + 2] & 0xf0) << 2;
> - val4 = (in[stride + 4] & 0xff) << 8 | (in[stride + 3] & 0xc0) << 0;
> + val4 = (in[stride + 4] & 0xff) << 8 | (in[stride + 3] & 0xc0) << 0;
> break;
> case 3:
> - val1 = (in[4] & 0xff) << 8 | (in[3] & 0xc0) << 0;
> + val1 = (in[4] & 0xff) << 8 | (in[3] & 0xc0) << 0;
> val2 = (in[6] & 0x03) << 14 | (in[5] & 0xff) << 6;
> - val3 = (in[stride + 4] & 0xff) << 8 | (in[stride + 3] & 0xc0) << 0;
> + val3 = (in[stride + 4] & 0xff) << 8 | (in[stride + 3] & 0xc0) << 0;
> val4 = (in[stride + 6] & 0x03) << 14 | (in[stride + 5] & 0xff) << 6;
> break;
> }
[snip]
> diff --git a/test/span.cpp b/test/span.cpp
> index abf3a5d6..3c3f6937 100644
> --- a/test/span.cpp
> +++ b/test/span.cpp
> @@ -9,12 +9,12 @@
> * Include first to ensure the header is self-contained, as there's no span.cpp
> * in libcamera.
> */
> -#include <libcamera/base/span.h>
> -
See the comment above :-) This change should be dropped.
> #include <array>
> #include <iostream>
> #include <vector>
>
> +#include <libcamera/base/span.h>
> +
> #include "test.h"
>
> using namespace std;
Can you take these comments into account, and drop the changes that have
been [snip]'ed ?
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list