[libcamera-devel] [PATCH] libcamera: stream: Expose stride value
Niklas Söderlund
niklas.soderlund at ragnatech.se
Fri May 1 17:08:41 CEST 2020
Hi Laurent,
Thanks for your comments. I have taken them all and pushed this to
master.
On 2020-05-01 17:46:07 +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Fri, May 01, 2020 at 04:30:21PM +0200, Niklas Söderlund wrote:
> > Expose the image stride which may be retrieved after a video device
> > have been configured. It may only be retrieved at that point as the
>
> s/have/has/
>
> > assignment of video devices takes place at this point.
> >
> > In the future video devices should be assign at configuration validation
>
> s/assign/assigned/
>
> > time and the stride value retrieved at that point.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > include/libcamera/stream.h | 1 +
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 42 ++++++++++++--------
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 1 +
> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 1 +
> > src/libcamera/pipeline/vimc/vimc.cpp | 1 +
> > src/libcamera/stream.cpp | 7 ++++
> > 6 files changed, 36 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > index 18142dc997bb8885..b3cbea3d52294846 100644
> > --- a/include/libcamera/stream.h
> > +++ b/include/libcamera/stream.h
> > @@ -42,6 +42,7 @@ struct StreamConfiguration {
> >
> > PixelFormat pixelFormat;
> > Size size;
> > + unsigned int stride;
> >
> > unsigned int bufferCount;
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index ff33f15f9eac3b68..0f9ffd411a680ad9 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -77,7 +77,8 @@ public:
> > int configureInput(const Size &size,
> > V4L2DeviceFormat *inputFormat);
> > int configureOutput(ImgUOutput *output,
> > - const StreamConfiguration &cfg);
> > + const StreamConfiguration &cfg,
> > + V4L2DeviceFormat *outputFormat);
> >
> > int allocateBuffers(IPU3CameraData *data, unsigned int bufferCount);
> > void freeBuffers(IPU3CameraData *data);
> > @@ -546,6 +547,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > IPU3Stream *vfStream = &data->vfStream_;
> > CIO2Device *cio2 = &data->cio2_;
> > ImgUDevice *imgu = data->imgu_;
> > + V4L2DeviceFormat outputFormat;
> > int ret;
> >
> > /*
> > @@ -625,12 +627,15 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > * The RAW still capture stream just copies buffers from the
> > * internal queue and doesn't need any specific configuration.
> > */
> > - if (stream->raw_)
> > - continue;
> > -
> > - ret = imgu->configureOutput(stream->device_, cfg);
> > - if (ret)
> > - return ret;
> > + if (stream->raw_) {
> > + cfg.stride = cio2Format.planes[0].bpl;
> > + } else {
> > + ret = imgu->configureOutput(stream->device_, cfg,
> > + &outputFormat);
> > + if (ret)
> > + return ret;
> > + cfg.stride = outputFormat.planes[0].bpl;
> > + }
> > }
> >
> > /*
> > @@ -639,13 +644,15 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > * be at least one active stream in the configuration request).
> > */
> > if (!outStream->active_) {
> > - ret = imgu->configureOutput(outStream->device_, config->at(0));
> > + ret = imgu->configureOutput(outStream->device_, config->at(0),
> > + &outputFormat);
> > if (ret)
> > return ret;
> > }
> >
> > if (!vfStream->active_) {
> > - ret = imgu->configureOutput(vfStream->device_, config->at(0));
> > + ret = imgu->configureOutput(vfStream->device_, config->at(0),
> > + &outputFormat);
> > if (ret)
> > return ret;
> > }
> > @@ -657,7 +664,7 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > StreamConfiguration statCfg = {};
> > statCfg.size = cio2Format.size;
> >
> > - ret = imgu->configureOutput(&imgu->stat_, statCfg);
> > + ret = imgu->configureOutput(&imgu->stat_, statCfg, &outputFormat);
> > if (ret)
> > return ret;
> >
>
> This isn't very nice, but will do for now as we know we'll have to
> rework the code anyway.
>
> > @@ -1166,7 +1173,8 @@ int ImgUDevice::configureInput(const Size &size,
> > * \return 0 on success or a negative error code otherwise
> > */
> > int ImgUDevice::configureOutput(ImgUOutput *output,
> > - const StreamConfiguration &cfg)
> > + const StreamConfiguration &cfg,
> > + V4L2DeviceFormat *outputFormat)
> > {
> > V4L2VideoDevice *dev = output->dev;
> > unsigned int pad = output->pad;
> > @@ -1183,17 +1191,17 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
> > if (output == &stat_)
> > return 0;
> >
> > - V4L2DeviceFormat outputFormat = {};
> > - outputFormat.fourcc = dev->toV4L2PixelFormat(PixelFormat(DRM_FORMAT_NV12));
> > - outputFormat.size = cfg.size;
> > - outputFormat.planesCount = 2;
> > + *outputFormat = {};
> > + outputFormat->fourcc = dev->toV4L2PixelFormat(PixelFormat(DRM_FORMAT_NV12));
> > + outputFormat->size = cfg.size;
> > + outputFormat->planesCount = 2;
> >
> > - ret = dev->setFormat(&outputFormat);
> > + ret = dev->setFormat(outputFormat);
> > if (ret)
> > return ret;
> >
> > LOG(IPU3, Debug) << "ImgU " << output->name << " format = "
> > - << outputFormat.toString();
> > + << outputFormat->toString();
> >
> > return 0;
> > }
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 6aa3178669b40a37..1e81a0048f093d8f 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -681,6 +681,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> > return ret;
> >
> > cfg.setStream(&data->stream_);
> > + cfg.stride = outputFormat.planes[0].bpl;
> >
> > return 0;
> > }
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 455693264c2e3e67..f0c1337de8623a6b 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -204,6 +204,7 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> > return -EINVAL;
> >
> > cfg.setStream(&data->stream_);
> > + cfg.stride = format.planes[0].bpl;
> >
> > return 0;
> > }
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index ccfd7f86d15860c5..7019ac322e6c2c59 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -253,6 +253,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> > return ret;
> >
> > cfg.setStream(&data->stream_);
> > + cfg.stride = format.planes[0].bpl;
> >
> > return 0;
> > }
> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> > index ef16aaa1b2f4586a..0a2468d92039676c 100644
> > --- a/src/libcamera/stream.cpp
> > +++ b/src/libcamera/stream.cpp
> > @@ -301,6 +301,13 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
> > * \brief Stream pixel format
> > */
> >
> > +/**
> > + * \var StreamConfiguration::stride
> > + * \brief Stream stride in bytes
>
> Maybe "Image stride for the stream, in bytes" ?
>
> I would add
>
> * The stride value reports the number of bytes between the beginning of
> * successive lines in an image buffer for this stream. The value is
> * valid after successfully configuring the camera with this
> * configuration with a call to Camera::Configure().
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > + * \todo Update this value when configuration is validated instead of when
> > + * the camera is configured.
> > + */
> > +
> > /**
> > * \var StreamConfiguration::bufferCount
> > * \brief Requested number of buffers to allocate for the stream
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list