[libcamera-devel] [PATCH 2/4] libcamera: pipeline: uvcvideo: enforce stream configuration
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sat Feb 23 02:39:01 CET 2019
Hi Laurent,
Thanks for your feedback.
On 2019-02-22 12:48:53 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Thu, Feb 21, 2019 at 12:59:37AM +0100, Niklas Söderlund wrote:
> > The format requested by configureStreams() should exactly match what
>
> s/what$/what is/
>
> > programmed on the video device it self. If they do not match the call
>
> s/it self/itself/ or simply s/ it self//
>
> > should fail as the application in that case will not know what
> > configuration actually was programmed.
>
> s/actually was/was actually/
I agree, will update this for 2/4 and 3/4 for next version.
>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > src/libcamera/pipeline/uvcvideo.cpp | 12 +++++++++++-
> > 1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 37a3477c2c56f3ea..b7b8ff109109e9c5 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -84,6 +84,7 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,
> > std::map<Stream *, StreamConfiguration> &config)
> > {
> > StreamConfiguration *cfg = &config[&stream_];
> > + int ret;
> >
> > LOG(UVC, Debug) << "Configure the camera for resolution "
> > << cfg->width << "x" << cfg->height;
> > @@ -93,7 +94,16 @@ int PipelineHandlerUVC::configureStreams(Camera *camera,
> > format.height = cfg->height;
> > format.fourcc = cfg->pixelFormat;
> >
> > - return video_->setFormat(&format);
> > + ret = video_->setFormat(&format);
> > + if (ret)
> > + return ret;
> > +
> > + if (format.width != cfg->width ||
> > + format.height != cfg->height ||
> > + format.fourcc != cfg->pixelFormat)
> > + return -EINVAL;
>
> This is fine for now, but I wonder if longer term we should enumerate
> the available formats and resolutions, and avoiding calling setFormat()
> is the requested format can't be achieved. The documentation in patch
> 1/4 explains that no configuration should be applied if an exact match
> is impossible, and calling setFormat() modifies the hardware
> configuration. On the other hand this change in hardware configuration
> isn't visible from the application, so it won't matter much.
I concur that that this is a short- to mid- term solution that allows us
to move forward. Once we have a more complete StreamConfiguration
implementation and more pipeline handler implementations I think we
might be able to draw some more insights. At that point I think we will
be able to create one or more helper functions to assist pipeline
handlers, likely using the *_TRY_* IOCTLs. That way we could try a
complete set of configurations in a none invasive fashion and fail in a
more safe way if needed.
Bonus for *_TRY_* IOCTLs support will be that applications would be able
to call that themself before calling configureStreams() to 'update' and
inspect the format they request to hardware/driver limitations before
asking the hardware to be reprogrammed. At least this is how I model the
future of this part of the library in my head, if anyone have other
ideas please let me know so I can try to have that in mind when working
on the interactions between the Camera and applications.
>
> > +
> > + return 0;
> > }
> >
> > int PipelineHandlerUVC::allocateBuffers(Camera *camera, Stream *stream)
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list