[PATCH v9 3/8] libcamera: pipeline: Add VirtualPipelineHandler

[Jacopo Mondi]

Hi Harvey

On Sat, Aug 31, 2024 at 02:44:40PM GMT, Jacopo Mondi wrote:
> Hi
>
> On Thu, Aug 29, 2024 at 09:57:58PM GMT, Cheng-Hao Yang wrote:
> > Thanks for the review, Jacopo!
> >
> > On Tue, Aug 27, 2024 at 5:43 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > wrote:
> >
> > > Hi Harvey
> > >
> > > On Tue, Aug 20, 2024 at 04:23:34PM GMT, Harvey Yang wrote:
> > > > From: Harvey Yang <chenghaoyang at chromium.org>
> > > >
> > > > Add VirtualPipelineHandler for more unit tests and verfiy libcamera
> > > > infrastructure works on devices without using hardware cameras.
> > > >
> > > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > ---
> > > >  meson.build                                |   1 +
> > > >  meson_options.txt                          |   3 +-
> > > >  src/libcamera/pipeline/virtual/meson.build |   5 +
> > > >  src/libcamera/pipeline/virtual/virtual.cpp | 251 +++++++++++++++++++++
> > > >  src/libcamera/pipeline/virtual/virtual.h   |  78 +++++++
> > >
> > > Do you expect other components to include this header in future ? If
> > > not, you can move its content to the .cpp file I guess
> > >
> > >
> > Actually `virtual/parser.h` needs to include it to return VirtualCameraData
> > when parsing the yaml config file [1]. Does that count :p?
>
> I guess it does :)
>
> >
> > [1]: https://patchwork.libcamera.org/patch/20971/
> >
> > >  5 files changed, 337 insertions(+), 1 deletion(-)
> > > >  create mode 100644 src/libcamera/pipeline/virtual/meson.build
> > > >  create mode 100644 src/libcamera/pipeline/virtual/virtual.cpp
> > > >  create mode 100644 src/libcamera/pipeline/virtual/virtual.h
> > > >
> > > > diff --git a/meson.build b/meson.build
> > > > index f946eba94..3cad3249a 100644
> > > > --- a/meson.build
> > > > +++ b/meson.build
> > > > @@ -222,6 +222,7 @@ pipelines_support = {
> > > >      'simple':       arch_arm,
> > > >      'uvcvideo':     ['any'],
> > > >      'vimc':         ['test'],
> > > > +    'virtual':      ['test'],
> > > >  }
> > > >
> > > >  if pipelines.contains('all')
> > > > diff --git a/meson_options.txt b/meson_options.txt
> > > > index 7aa412491..c91cd241a 100644
> > > > --- a/meson_options.txt
> > > > +++ b/meson_options.txt
> > > > @@ -53,7 +53,8 @@ option('pipelines',
> > > >              'rpi/vc4',
> > > >              'simple',
> > > >              'uvcvideo',
> > > > -            'vimc'
> > > > +            'vimc',
> > > > +            'virtual'
> > > >          ],
> > > >          description : 'Select which pipeline handlers to build. If this
> > > is set to "auto", all the pipelines applicable to the target architecture
> > > will be built. If this is set to "all", all the pipelines will be built. If
> > > both are selected then "all" will take precedence.')
> > > >
> > > > diff --git a/src/libcamera/pipeline/virtual/meson.build
> > > b/src/libcamera/pipeline/virtual/meson.build
> > > > new file mode 100644
> > > > index 000000000..ba7ff754e
> > > > --- /dev/null
> > > > +++ b/src/libcamera/pipeline/virtual/meson.build
> > > > @@ -0,0 +1,5 @@
> > > > +# SPDX-License-Identifier: CC0-1.0
> > > > +
> > > > +libcamera_sources += files([
> > > > +    'virtual.cpp',
> > > > +])
> > > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp
> > > b/src/libcamera/pipeline/virtual/virtual.cpp
> > > > new file mode 100644
> > > > index 000000000..74eb8c7ad
> > > > --- /dev/null
> > > > +++ b/src/libcamera/pipeline/virtual/virtual.cpp
> > > > @@ -0,0 +1,251 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2023, Google Inc.
> > > > + *
> > > > + * virtual.cpp - Pipeline handler for virtual cameras
> > > > + */
> > > > +
> > >
> > > You should include the header for the standard library construct you
> > > use. I see vectors, maps, unique_ptrs etc
> > >
> > Done, please check again.
> >
> >
> > >
> > > > +#include "virtual.h"
> > > > +
> > > > +#include <libcamera/base/log.h>
> > > > +
> > > > +#include <libcamera/camera.h>
> > > > +#include <libcamera/control_ids.h>
> > > > +#include <libcamera/controls.h>
> > > > +#include <libcamera/formats.h>
> > > > +#include <libcamera/property_ids.h>
> > > > +
> > > > +#include "libcamera/internal/camera.h"
> > >
> > > The internal header includes the public one by definition
> > >
> > Ack. Removed the public one.
> >
> >
> > >
> > > > +#include "libcamera/internal/formats.h"
> > >
> > > This doesn't as <libcamera/formats.h> is generated. I wonder if it
> > > should.
> > >
> > Keeping `#include <libcamera/formats.h>`.
> >
> >
> > >
> > > > +#include "libcamera/internal/pipeline_handler.h"
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +LOG_DEFINE_CATEGORY(Virtual)
> > > > +
> > > > +namespace {
> > > > +
> > > > +uint64_t currentTimestamp()
> > > > +{
> > > > +     struct timespec ts;
> > > > +     if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0) {
> > > > +             LOG(Virtual, Error) << "Get clock time fails";
> > > > +             return 0;
> > > > +     }
> > > > +
> > > > +     return ts.tv_sec * 1'000'000'000LL + ts.tv_nsec;
> > > > +}
> > >
> > > Could https://en.cppreference.com/w/cpp/chrono/steady_clock/now save
> > > you a custom function ?
> > >
> > > In example:
> > >
> > >         const auto now = std::chrono::steady_clock::now();
> > >         auto nsecs =
> > > std::chrono::duration_cast<std::chrono::nanoseconds>(now.time_since_epoch());
> > >         std::cout << nsecs.count();
> > >
> > > should give you the time in nanoseconds since system boot (if I got it
> > > right)
> > >
> > >
> > > > +
> > > > +} // namespace
> > >
> > > nit: /* namespace */
> > > here and in other places
> > >
> > > Done
> >
> >
> > > > +
> > > >
> > > +VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData
> > > *data)
> > > > +     : CameraConfiguration(), data_(data)
> > > > +{
> > > > +}
> > > > +
> > > > +CameraConfiguration::Status VirtualCameraConfiguration::validate()
> > > > +{
> > > > +     Status status = Valid;
> > > > +
> > > > +     if (config_.empty()) {
> > > > +             LOG(Virtual, Error) << "Empty config";
> > > > +             return Invalid;
> > > > +     }
> > > > +
> > > > +     /* Currently only one stream is supported */
> > > > +     if (config_.size() > 1) {
> > > > +             config_.resize(1);
> > > > +             status = Adjusted;
> > > > +     }
> > > > +
> > > > +     Size maxSize;
> > > > +     for (const auto &resolution : data_->supportedResolutions_)
> > > > +             maxSize = std::max(maxSize, resolution.size);
> > > > +
> > > > +     for (StreamConfiguration &cfg : config_) {
> > >
> > > you only have config, or in the next patches will this be augmented ?
> > >
> > > Do you mean that I should check `sensorConfig` or `orientation` as well?
> >
>
> No I meant I was assuming the virtual pipline works with a single
> stream by design. But seeing your replies to the next patches makes me
> think my assumption was wrong.
>

Let me clarify this once and for all: you currently have a single
stream in VirtualCameraData and your pipeline works with a single
stream, right ?

Do you plane to change this and the pipeline handler code is
structured in this way to prepare for that ?