| Message ID | 20200501143021.404090-1-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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@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
Le vendredi 01 mai 2020 à 16:30 +0200, Niklas Söderlund a écrit : > 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 > assignment of video devices takes place at this point. > > In the future video devices should be assign at configuration validation > time and the stride value retrieved at that point. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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; In my opinion, as we have full knowledge of it already, we should directly start with an array of up to 4 strides. We can implement extrapolation for non-mplane V4L2 formats. This way we don't need to port the userspace bits twice. > > 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; > > @@ -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 > + * \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
Le vendredi 01 mai 2020 à 17:46 +0300, Laurent Pinchart a écrit : > 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@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(). This does not apply to an image, but to one plane (well first plane) of an image. I really think we should do this right on first go and expose an array of strides, and while at it, offsets. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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
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@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@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
Hi Nicolas, Sorry for jumping the gun and merging this minuets after your comments. I had not seen them at the time :-( On 2020-05-01 11:08:13 -0400, Nicolas Dufresne wrote: > Le vendredi 01 mai 2020 à 17:46 +0300, Laurent Pinchart a écrit : > > 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@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(). > > This does not apply to an image, but to one plane (well first plane) of > an image. I really think we should do this right on first go and expose > an array of strides, and while at it, offsets. I agree it would be best to do this correct on the first implementation. I just don't know what solution covers all future use-cases. I agree more work is needed here in to cover all needs as StreamConfiguration should be made plane aware for other reasons then stride but also offsets as you point out. What else could be needed? Information of the usage of each plane? > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@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 >
Hi Nicolas, On Fri, May 01, 2020 at 11:05:47AM -0400, Nicolas Dufresne wrote: > Le vendredi 01 mai 2020 à 16:30 +0200, Niklas Söderlund a écrit : > > 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 > > assignment of video devices takes place at this point. > > > > In the future video devices should be assign at configuration validation > > time and the stride value retrieved at that point. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@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; > > In my opinion, as we have full knowledge of it already, we should > directly start with an array of up to 4 strides. We can implement > extrapolation for non-mplane V4L2 formats. This way we don't need to > port the userspace bits twice. I agree with you. Let's not use the stride in the gstreamer element yet, we'll update that to 4 values. I would however like to also move stride computation to CameraConfiguration::validate() time too, and that requires a bit of work on the pipeline handler side. Please bear with us until we complete that :-) > > 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; > > > > @@ -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 > > + * \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
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; @@ -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 + * \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
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 assignment of video devices takes place at this point. In the future video devices should be assign at configuration validation time and the stride value retrieved at that point. Signed-off-by: Niklas Söderlund <niklas.soderlund@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(-)