[libcamera-devel] [RFC PATCH v2 5/5] libcamera: pipelines: uvcvideo: add IPAManager

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Thu, May 23, 2019 at 12:42:10PM -0400, Paul Elder wrote:
> Make the UVC pipeline query for one of the test IPA modules.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
> Changes in v2:
> - remove IPAManager from PipelineHandler::match parameter
> 
> This is a sample of how a IPAManager would be used.

I'm afraid you picked one of the pipeline handlers that will very likely
not need an IPA :-) UVC cameras are high-level cameras, and implement
the 3A algorithms internally. For test purpose this could still be
useful as the hardware is easier to source, but in that case I would
move it to the VIMC pipeline handler as that's even easier to source.

>  src/libcamera/pipeline/uvcvideo.cpp | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 351712c..8275f5a 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -6,16 +6,20 @@
>   */
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/ipa/ipa_module_info.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
>  #include "device_enumerator.h"
> +#include "ipa_manager.h"
>  #include "log.h"
>  #include "media_device.h"
>  #include "pipeline_handler.h"
>  #include "utils.h"
>  #include "v4l2_device.h"
>  
> +#include <string.h>
> +

This should move up, before the libcamera includes.

>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(UVC)
> @@ -175,6 +179,16 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  	if (!media)
>  		return false;
>  
> +	IPAManager *ipaManager = IPAManager::instance();
> +	ipaManager->addDir("test/ipa");
> +	struct IPAModuleInfo info;
> +	info.ipaAPIVersion = 1;
> +	info.pipelineVersion = 8999;
> +	strcpy(info.pipelineName, "bleep");
> +	IPAModule *ipa = ipaManager->acquireIPA(info);
> +	if (ipa == nullptr)
> +		LOG(UVC, Warning) << "no matching IPA found";
> +

Too complicated for pipeline handlers :-) Here's what you want to
achieve.

	IPAModule *ipa = IPAManager::instance()->find(this);

and the ipa variable should be stored in the camera data (as it will be
used later, that's the whole point :-)).

The next step will be to create the IPA implementation object that I
mentioned in the review of 2/5 (created by a factory method exposed by
the module), and I think that the IPAModule itself will be hidden from
the pipeline handler. I foresee something along the lines of

	/* In the camera data class */
	std::unique_ptr<IPAImplementation> ipa_;

(IPAImplementation should be renamed)

	/* Here */
	ipa_ = IPAManager::create(this);

This will internally find a corresponding IPAModule, and call its
factory method to create an IPAImplementation object, that will point
internally to the IPAModule in case the module needs to be accessed at
runtime.

>  	std::unique_ptr<UVCCameraData> data = utils::make_unique<UVCCameraData>(this);
>  
>  	/* Locate and open the default video node. */
> @@ -197,7 +211,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  
>  	/* Create and register the camera. */
>  	std::set<Stream *> streams{ &data->stream_ };
> -	std::shared_ptr<Camera> camera = Camera::create(this, media->model(), streams);
> +	std::shared_ptr<Camera> camera = Camera::create(this, media->model() + " " + (ipa == nullptr ? "" : ipa->info().name), streams);

That's a very long line, and I'm not sure to understand why you would
like to include this in the camera name. The camera name is meant to be
exposed to users, I don't think they should even know that an IPA is
running.

>  	registerCamera(std::move(camera), std::move(data));
>  
>  	/* Enable hot-unplug notifications. */

-- 
Regards,

Laurent Pinchart