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