[PATCH v14 3/7] libcamera: virtual: Add VirtualPipelineHandler

[Kieran Bingham]

Quoting Kieran Bingham (2024-10-03 09:38:26)
> Quoting Harvey Yang (2024-09-30 07:29:44)
> > 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>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > ---
... snip ...

> > +bool PipelineHandlerVirtual::match([[maybe_unused]] DeviceEnumerator *enumerator)
> > +{
> > +       if (created_)
> > +               return false;
> > +
> > +       created_ = true;
> > +
> > +       /* \todo Add virtual cameras according to a config file. */
> > +
> > +       std::vector<VirtualCameraData::Resolution> supportedResolutions;
> > +       supportedResolutions.resize(2);
> > +       supportedResolutions[0] = { .size = Size(1920, 1080), .frameRates = { 30 } };
> > +       supportedResolutions[1] = { .size = Size(1280, 720), .frameRates = { 30 } };
> > +
> > +       std::unique_ptr<VirtualCameraData> data =
> > +               std::make_unique<VirtualCameraData>(this, supportedResolutions);
> > +
> > +       data->properties_.set(properties::Location, properties::CameraLocationFront);
> > +       data->properties_.set(properties::Model, "Virtual Video Device");
> > +       data->properties_.set(properties::PixelArrayActiveAreas, { Rectangle(Size(1920, 1080)) });
> > +
> > +       /* \todo Set FrameDurationLimits based on config. */
> > +       ControlInfoMap::Map controls;
> > +       int64_t min_frame_duration = 33333, max_frame_duration = 33333;
> > +       controls[&controls::FrameDurationLimits] = ControlInfo(min_frame_duration, max_frame_duration);
> > +       data->controlInfo_ = ControlInfoMap(std::move(controls), controls::controls);
> > +
> > +       /* Create and register the camera. */
> > +       std::set<Stream *> streams;
> > +       for (auto &streamConfig : data->streamConfigs_)
> > +               streams.insert(&streamConfig.stream);
> > +
> > +       const std::string id = "Virtual0";
> > +       std::shared_ptr<Camera> camera = Camera::create(std::move(data), id, streams);
> > +       registerCamera(std::move(camera));
> > +
> > +       return true;
> 
> I think if you return false here, you don't need any of the created_; 
> 
>         /*
>          * Ensure we only ever register and create a single virtual
>          * pipeline handler.
>          */
>         return false;
> 
> This is core libcamera not your patch, - I still dislike this return
> usage of match(). It's either not clear or not fitting well to the needs
> of most pipeline handlers.
> 
> It's should be clearer at core that returning true here simply means
> 'please try to create more pipeline handlers'

Hrm - Jacopo has highlighted that if the 'successful' creation returns
false - then there would be no output that the pipeline handler had
matched successfully ...

So given that - what you have is correct...

I'd like it to be different but that requires core pipeline handler
iteration rework ;-) so lets not dwell on that right now.


> Anyway, comments above could be applied on top if you wish - or handled
> how you prefer.
> 
> The created_ just seems like an awkward workaround, but it's a
> workaround for trying to return the right thing at the end of match. But
> I think the 'right thing' to return there is poorly defined maybe...
> 
> Anyway, with any of the above handled or not - or handled on top - none
> of that blocks this patch for me.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>