[libcamera-devel] [PATCH v4 11/12] libcamera: ipu3: Use roles in stream configuration
Jacopo Mondi
jacopo at jmondi.org
Thu Apr 18 12:16:29 CEST 2019
Hi Laurent, Niklas,
On Mon, Apr 15, 2019 at 05:42:44PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Sun, Apr 14, 2019 at 10:45:18PM +0200, Niklas Söderlund wrote:
> > On 2019-04-09 21:25:47 +0200, Jacopo Mondi wrote:
> > > Use and inspect the stream roles provided by the application to
> > > streamConfiguration() to assign streams to their intended roles and
> > > return a default configuration associated with them.
> > >
> > > Support a limited number of usages, as the viewfinder stream can
> > > optionally be used for capturing still images, but the main output
> > > stream cannot be used as viewfinder or for video recording purposes.
>
> Is this a limitation of the device, or of the pipeline handler ?
>
Is a limitation I introduced in the pipeline handler as I assume
viewfinder frame rate cannot be sustained by the still capture
output node. It's an assumption though, but I kept it there to
demonstrate how to use roles in assigning configurations.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 105 ++++++++++++++++++++-------
> > > 1 file changed, 79 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 75ffdc56d157..70a92783076f 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -227,36 +227,89 @@ CameraConfiguration
> > > PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> > > const std::vector<StreamUsage> &usages)
> > > {
> > > - CameraConfiguration configs;
> > > IPU3CameraData *data = cameraData(camera);
> > > - StreamConfiguration config = {};
> > > + CameraConfiguration configs;
>
> I wouldn't use the plural for a single object. How about
> CameraConfiguration config and StreamConfiguration cfg ? Or
> CameraConfiguration ccfg and StreamConfiguration scfg ? There's also the
> option of cameraConfig and streamConfig but that may be a bit long.
>
> > > + std::vector<IPU3Stream *> availableStreams = {
> >
> > Using a std::set instead of std::vector here would be useful. As you
> > can't have the same stream in the set twice there is no limitation
> > moving from a vector to a set.
> >
> > > + &data->outStream_,
> > > + &data->vfStream_,
> > > + };
> > >
> > > - /*
> > > - * FIXME: Soraka: the maximum resolution reported by both sensors
> > > - * (2592x1944 for ov5670 and 4224x3136 for ov13858) are returned as
> > > - * default configurations but they're not correctly processed by the
> > > - * ImgU. Resolutions up tp 2560x1920 have been validated.
> > > - *
> > > - * \todo Clarify ImgU alignement requirements.
> > > - */
> > > - config.width = 2560;
> > > - config.height = 1920;
> > > - config.pixelFormat = V4L2_PIX_FMT_NV12;
> > > - config.bufferCount = IPU3_BUFFER_COUNT;
> > > -
> > > - configs[&data->outStream_] = config;
> > > - LOG(IPU3, Debug)
> > > - << "Stream 'output' format set to " << config.width << "x"
> > > - << config.height << "-0x" << std::hex << std::setfill('0')
> > > - << std::setw(8) << config.pixelFormat;
> > > -
> > > - configs[&data->vfStream_] = config;
> > > - LOG(IPU3, Debug)
> > > - << "Stream 'viewfinder' format set to " << config.width << "x"
> > > - << config.height << "-0x" << std::hex << std::setfill('0')
> > > - << std::setw(8) << config.pixelFormat;
> > > + for (const StreamUsage &usage : usages) {
> > > + std::vector<IPU3Stream *>::iterator found;
> >
> > Can be refactored away if availableStreams is a std::set, see example
> > bellow.
> >
> > > + enum StreamUsage::Role r = usage.role();
> >
> > s/enum//
>
> And s/r/role/ ? r is a bit confusing, it also commonly refers to a
> rectangle.
>
> > > + StreamConfiguration config = {};
> > > + IPU3Stream *stream = nullptr;
> > > +
> > > + std::vector<IPU3Stream *>::iterator s = availableStreams.begin();
> > > + if (r == StreamUsage::Role::StillCapture) {
> > > + for (; s < availableStreams.end(); ++s) {
> > > + /*
> > > + * We can use the viewfinder stream in case
> > > + * the 'StillCapture' usage is required
> > > + * multiple times.
> > > + */
> > > + if (*s == &data->outStream_) {
> > > + stream = &data->outStream_;
> > > + found = s;
> > > + break;
> > > + } else {
> > > + stream = &data->vfStream_;
> > > + found = s;
> > > + }
> > > + }
> >
> > The for() loop can be replaced with this if availableStreams is std::set
> >
> >
> > if (availableStreams.find(&data->outStream_) != availableStreams.end())
> > stream = &data->outStream_;
> > else
> > stream = &data->vfStream_;
> >
Much better, thanks!
Just one note, this is not enough, as we might get asked for 3
viewfinders, so we have to make sure vfStream_ is available as well,
and fail eventually if it is not.
> > > +
> > > + /*
> > > + * FIXME: Soraka: the maximum resolution reported by
> > > + * both sensors (2592x1944 for ov5670 and 4224x3136 for
> > > + * ov13858) are returned as default configurations but
> > > + * they're not correctly processed by the ImgU.
> > > + * Resolutions up tp 2560x1920 have been validated.
> > > + *
> > > + * \todo Clarify ImgU alignment requirements.
> > > + */
> > > + config.width = 2560;
> > > + config.height = 1920;
> > > + } else if (r == StreamUsage::Role::Viewfinder ||
> > > + r == StreamUsage::Role::VideoRecording) {
> > > + for (; s < availableStreams.end(); ++s) {
> > > + /*
> > > + * We can't use the 'output' stream for
> > > + * viewfinder or video capture usages.
> > > + */
> > > + if (*s != &data->vfStream_)
> > > + continue;
> > > +
> > > + stream = &data->vfStream_;
> > > + found = s;
> > > + break;
> > > + }
>
> And here
>
> if (availableStreams.find(&data->vfStream_) !=
> availableStreams.end())
> stream = &data->vfStream_;
>
> You may want to rename availableStreams to streams if it helps
> shortening lines, up to you.
>
> > > +
> > > + config.width = 640;
> > > + config.height = 480;
>
> Should you use the resolution requested by the Viewfinder usage ?
>
Yes indeed!
Thanks, with Niklas and your suggestions this looks much much better!
> > > + }
> > > +
> > > + if (stream == nullptr)
> > > + goto error;
> > > +
> > > + availableStreams.erase(found);
> >
> > With std::set
> >
> > availableStreams.erase(stream);
> >
> > > +
> > > + config.pixelFormat = V4L2_PIX_FMT_NV12;
> > > + config.bufferCount = IPU3_BUFFER_COUNT;
> > > +
> > > + configs[stream] = config;
> > > +
> > > + LOG(IPU3, Debug)
> > > + << "Stream " << stream->name_ << " format set to "
> > > + << config.width << "x" << config.height
> > > + << "-0x" << std::hex << std::setfill('0')
> > > + << std::setw(8) << config.pixelFormat;
> > > + }
> > >
> > > return configs;
> > > +
> > > +error:
> > > + LOG(IPU3, Error) << "Requested stream roles not supported";
> > > + return CameraConfiguration{};
> > > }
> > >
> > > int PipelineHandlerIPU3::configureStreams(Camera *camera,
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190418/8e1cd5a7/attachment.sig>
More information about the libcamera-devel
mailing list