[libcamera-devel] [RFC PATCH v2] lc-compliance: Add test to set test pattern mode

[Hirokazu Honda]

Hi Laurent,

On Wed, Oct 13, 2021 at 1:07 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Tue, Oct 05, 2021 at 02:40:27PM +0900, Hirokazu Honda wrote:
> > This adds the test to verify the requested test pattern mode is
> > correctly applied in request metadata.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  src/lc-compliance/meson.build                |   1 +
> >  src/lc-compliance/test_pattern_mode_test.cpp | 157 +++++++++++++++++++
> >  2 files changed, 158 insertions(+)
> >  create mode 100644 src/lc-compliance/test_pattern_mode_test.cpp
> >
> > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build
> > index aa5852f6..0fd280df 100644
> > --- a/src/lc-compliance/meson.build
> > +++ b/src/lc-compliance/meson.build
> > @@ -17,6 +17,7 @@ lc_compliance_sources = files([
> >      'main.cpp',
> >      'simple_capture.cpp',
> >      'capture_test.cpp',
> > +    'test_pattern_mode_test.cpp',
> >  ])
> >
> >  lc_compliance  = executable('lc-compliance', lc_compliance_sources,
> > diff --git a/src/lc-compliance/test_pattern_mode_test.cpp b/src/lc-compliance/test_pattern_mode_test.cpp
> > new file mode 100644
> > index 00000000..d8599b64
> > --- /dev/null
> > +++ b/src/lc-compliance/test_pattern_mode_test.cpp
> > @@ -0,0 +1,157 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + * Copyright (C) 2021, Collabora Ltd.
> > + *
> > + * test_pattern_mode_test.cpp - Test a capture with a test pattern mode
> > + */
> > +
> > +#include <libcamera/libcamera.h>
> > +
> > +#include <gtest/gtest.h>
> > +
> > +#include "environment.h"
> > +#include "simple_capture.h"
> > +
> > +using namespace libcamera;
> > +
> > +namespace {
>
> You can add a blank line here.
>
> > +/*
> > + * SimpleCaptureTestPatternMode verifies captures with a test pattern mode. It
> > + * checks the request has the specified test pattern mode in the metadata, but
> > + * doesn't verify the capture content.
> > + */
> > +class SimpleCaptureTestPatternMode : public SimpleCapture
> > +{
> > +public:
> > +     SimpleCaptureTestPatternMode(std::shared_ptr<Camera> camera);
> > +
> > +     std::vector<int32_t> availableTestPatternModes() const;
> > +     void capture(int32_t testPatternMode);
> > +
> > +private:
> > +     void requestComplete(Request *request) override;
> > +
> > +     unsigned int captureCount_;
> > +     unsigned int captureLimit_;
> > +     int32_t testPatternMode_;
> > +};
> > +
> > +/* SimpleCaptureTestPatternMode */
> > +
> > +SimpleCaptureTestPatternMode::SimpleCaptureTestPatternMode(std::shared_ptr<Camera> camera)
> > +     : SimpleCapture(camera)
> > +{
> > +}
> > +
> > +std::vector<int32_t> SimpleCaptureTestPatternMode::availableTestPatternModes() const
> > +{
> > +     const auto &controls = camera_->controls();
> > +     const auto it = controls.find(&controls::draft::TestPatternMode);
> > +     if (it == controls.end())
> > +             return {};
> > +
> > +     std::vector<int32_t> testPatternModes;
> > +     const std::vector<ControlValue> &values = it->second.values();
> > +     for (const auto &value : values)
> > +             testPatternModes.push_back(value.get<int32_t>());
> > +
> > +     return testPatternModes;
> > +}
> > +
> > +void SimpleCaptureTestPatternMode::capture(int32_t testPatternMode)
> > +{
> > +     start();
> > +
> > +     Stream *stream = config_->at(0).stream();
> > +     const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
> > +     ASSERT_FALSE(buffers.empty());
> > +
> > +     captureCount_ = 0;
> > +     captureLimit_ = buffers.size();
> > +     testPatternMode_ = testPatternMode;
> > +
> > +     std::vector<std::unique_ptr<libcamera::Request>> requests;
> > +     for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> > +             std::unique_ptr<Request> request = camera_->createRequest();
> > +             ASSERT_TRUE(request) << "Can't create request";
> > +
> > +             request->controls().set(controls::draft::TestPatternMode,
> > +                                     testPatternMode);
>
> Would it be useful to start without a test pattern and only enable it
> after a few frames, to test that it gets enabled exactly when requested
> ?
>
> > +
> > +             ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0)
> > +                     << "Can't set buffer for request";
> > +
> > +             ASSERT_EQ(camera_->queueRequest(request.get()), 0)
> > +                     << "Failed to queue request";
> > +
> > +             requests.push_back(std::move(request));
> > +     }
> > +
> > +     /* Run capture session. */
> > +     loop_ = new EventLoop();
> > +     int status = loop_->exec();
> > +     stop();
> > +     delete loop_;
> > +
> > +     ASSERT_EQ(status, 0);
> > +}
> > +
> > +void SimpleCaptureTestPatternMode::requestComplete(Request *request)
> > +{
> > +     captureCount_++;
> > +     if (captureCount_ > captureLimit_) {
> > +             loop_->exit(0);
> > +             return;
> > +     }
> > +
> > +     if (!request->metadata().contains(controls::draft::TestPatternMode)) {
> > +             if (testPatternMode_ != controls::draft::TestPatternModeOff) {
> > +                     std::cerr << "No test pattern mode mismatch: expected="
>
> s/mismatch/reported/
>
> > +                               << testPatternMode_ << std::endl;
> > +                     loop_->exit(-EINVAL);
>
> Is there a reason why we can't use ASSERT here, and have to delay it
> until the status check in SimpleCaptureTestPatternMode::capture() ?
>

It is a bit difficult to use ASSERT and EXPECT.
The rule in my mind is https://stackoverflow.com/a/2565309.
In this case, we can continue the test even though the test pattern
mode mismatches.
We may want to see test pattern modes in succeeding captures.

I felt like changing some ASSERT in other tests to EXPECT, but I don't
know the policy in libcamera.

-Hiro
> > +                     return;
> > +             }
> > +     } else {
> > +             const int32_t testPatternMode =
> > +                     request->metadata().get(controls::draft::TestPatternMode);
> > +             if (testPatternMode_ != testPatternMode) {
> > +                     std::cerr << "Test pattern mode mismatch: expected="
> > +                               << testPatternMode_ << ", actual="
> > +                               << testPatternMode << std::endl;
> > +
> > +                     loop_->exit(-EINVAL);
> > +                     return;
> > +             }
> > +     }
> > +
> > +     request->reuse(Request::ReuseBuffers);
> > +     request->controls().set(controls::draft::TestPatternMode,
> > +                             testPatternMode_);
> > +     if (camera_->queueRequest(request))
> > +             loop_->exit(-EINVAL);
> > +}
> > +} /* namespace */
> > +
> > +TEST(TestPatternModeTest, SetTestPatternMode)
> > +{
> > +     Environment *env = Environment::get();
> > +
> > +     auto camera = env->cm()->get(env->cameraId());
> > +     ASSERT_TRUE(camera);
> > +     ASSERT_EQ(camera->acquire(), 0);
> > +
> > +     SimpleCaptureTestPatternMode capture(camera);
> > +     capture.configure(StreamRole::Viewfinder);
> > +
> > +     auto testPatternModes = capture.availableTestPatternModes();
> > +     if (testPatternModes.empty()) {
> > +             camera->release();
> > +             GTEST_SKIP() << "No test pattern is available";
> > +     }
> > +
> > +     for (const int32_t testPatternMode : testPatternModes)
> > +             capture.capture(testPatternMode);
> > +
> > +     camera->release();
> > +}
>
> --
> Regards,
>
> Laurent Pinchart