[libcamera-devel] [PATCH v3 0/5] Improve BayerFormat class

[Laurent Pinchart]

Hi Sebastian,

Thank you for the patch.

On Tue, Jan 26, 2021 at 07:48:49PM +0100, Sebastian Fricke wrote:
> Improve BayerFormat class
> 
> This patch series adds unit-tests, the `fromV4L2PixelFormat` static
> member function and the ==/!= operators to the BayerFormat class.

This looks good overall. I forgot to mention in the review of individual
commits that the subject line should start with "libcamera: bayer_format: "
for patch 1/5 to 4/5.

The issues are minor and I can address them when applying the patches. I
would however like David's feedback on patch 4/5 before pushing this
series.

> ---
> 
> Changes since version 2:
> * Improve the error messages of the unit tests by reducing them to a
>   single-line if possible.
> * Change the test that makes sure that an invalid BayerFormat doesn't
>   yield a V4L2PixelFormat to use an empty BayerFormat instead of an
>   imaginary one.
> * Add the transpose transformation to the transform method.
> * Add an extra test that makes sure that a transpose on a format causes
>   red and blue pixels that are on the antidiagonal to switch their position.
> * Change the transpose tests to not compare BayerFormat string
>   representations.
> * Combine the modifications in the Raspberry Pi pipeline with the first
>   patch that adds the `fromV4L2PixelFormat` static member function.
> * Add documentation for the `!=` operator.
> * Split the removal of the deprecated constructor and mapping table from
>   the addition of the `fromV4L2PixelFormat` function.
> * Fix indentation issues.
> 
> Changes since version 1:
> * Convert the `fromV4L2PixelFormat` to a static member function, as it doesn't
>   contain any self use.
> * Adjust the current user of the old constructor (raspberryPi pipeline) to use
>   the new function.
> * Add the ==/!= operators to improve tests and to add helpful helper functions.
> * Change the logic of `fromV4L2PixelFormat` by replace the search of a matching
>   bayer format to a v4l2 pixel format with a search for a matching v4l2 pixel
>   format to a bayer format.
> * Remove the v4l2ToBayer mapping table as it lost it's last user
> * Remove 'TEST \d:' & 'TEST \d: FAIL:' pattern from comments and messages
>   within the unit tests.
> * Convert variable names from snake_case to camelCase.
> * Add an additional to test to confirm that we cannot get a V4L2PixelFormat
>   from a BayerFormat not found in the mapping table.
> * Add tests for the == & != operators
> * Change the test within the Transform unit tests from a comparision of string
>   formats to a direct compare of the BayerFormat objects.
> * Unify the test error messages by enclosing the expected and actual string
>   representations in ''
> * Fix checkstyle errors.
> * Add explaination for why the transpose functionality was ignored
>   within the unit test.
> 
> ---
> 
> Sebastian Fricke (5):
>   libcamera: Add the fromV4L2PixelFormat function
>   libcamera: Remove unnecessary constructor
>   libcamera: Overload ==/!= operators for BayerFormats
>   libcamera: Add the transpose transformation
>   test: Add unit tests for the BayerFormat class
> 
>  include/libcamera/internal/bayer_format.h     |   8 +-
>  src/libcamera/bayer_format.cpp                |  89 ++++----
>  .../pipeline/raspberrypi/raspberrypi.cpp      |   4 +-
>  test/bayer_format.cpp                         | 210 ++++++++++++++++++
>  test/meson.build                              |   1 +
>  5 files changed, 260 insertions(+), 52 deletions(-)
>  create mode 100644 test/bayer_format.cpp

-- 
Regards,

Laurent Pinchart