[libcamera-devel] [PATCH v3 2/8] libcamera: ipu3: Create camera with 2 streams
Jacopo Mondi
jacopo at jmondi.org
Mon Apr 8 15:23:30 CEST 2019
Hi Laurent,
On Mon, Apr 08, 2019 at 04:02:56PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you.
>
[snip]
> > >> + configs[&data->vfStream_] = config;
> > >
> > > Can both streams scale ?
> >
> > Can't they?
>
> I don't know, it was a true question :-)
>
> > Why is it relevant here?
>
> Because you're configuring both streams with the same output size,
> different than the sensor size, so you need the scale on both streams.
>
Oh, now I get what you mean here... Yes, both streams can scale
indeed. According to upstream IPU3 developers viewfinder could
additionally perform composition, but the field of view selection
procedure is handled internally by the driver, as their API
implementation currently does not allow to apply a composition
rectangle to the viewfinder's pad)
Thanks
j
> > >> LOG(IPU3, Debug)
> > >> - << "Stream format set to " << config->width << "x"
> > >> - << config->height << "-0x" << std::hex << std::setfill('0')
> > >> - << std::setw(8) << config->pixelFormat;
> > >> + << "Stream 'viewfinder' format set to " << config.width << "x"
> > >> + << config.height << "-0x" << std::hex << std::setfill('0')
> > >> + << std::setw(8) << config.pixelFormat;
> > >>
> > >> return configs;
> > >> }
> > >>
> > >> int PipelineHandlerIPU3::configureStreams(Camera *camera,
> > >> - std::map<Stream *, StreamConfiguration> &config)
> > >> + std::map<Stream *,
> > >> + StreamConfiguration> &config)
> > >
> > > That doesn't look nice, I think you could keep the longer line instead.
> >
> > I like 80-cols :(
>
> So do I, but something small exceptions are useful too :-)
>
> > >> {
> > >> IPU3CameraData *data = cameraData(camera);
> > >> - const StreamConfiguration &cfg = config[&data->stream_];
> > >> + StreamConfiguration CIO2Config = {};
> > >
> > > Variables should start with lower case.
> >
> > Ack.. cio2Config then
> >
> > >> CIO2Device *cio2 = &data->cio2_;
> > >> ImgUDevice *imgu = data->imgu_;
> > >> int ret;
> > >>
> > >> - LOG(IPU3, Info)
> > >> - << "Requested image format " << cfg.width << "x"
> > >> - << cfg.height << "-0x" << std::hex << std::setfill('0')
> > >> - << std::setw(8) << cfg.pixelFormat << " on camera '"
> > >> - << camera->name() << "'";
> > >> + /* Remove previously configured stream masks to store the new ones. */
> > >> + data->activeStreamsMask = 0;
> > >> + for (auto const &streamConfig : config) {
> > >> + Stream *stream = streamConfig.first;
> > >> + const StreamConfiguration &cfg = streamConfig.second;
> > >>
> > >> - /*
> > >> - * Verify that the requested size respects the IPU3 alignement
> > >> - * requirements (the image width shall be a multiple of 8 pixels and
> > >> - * its height a multiple of 4 pixels) and the camera maximum sizes.
> > >> - *
> > >> - * \todo: consider the BDS scaling factor requirements:
> > >> - * "the downscaling factor must be an integer value multiple of 1/32"
> > >> - */
> > >> - if (cfg.width % 8 || cfg.height % 4) {
> > >> - LOG(IPU3, Error) << "Invalid stream size: bad alignment";
> > >> - return -EINVAL;
> > >> - }
> > >> + /*
> > >> + * Verify that the requested size respects the IPU3 alignement
> > >> + * requirements (the image width shall be a multiple of 8
> > >> + * pixels and its height a multiple of 4 pixels) and the camera
> > >> + * maximum sizes.
> > >> + *
> > >> + * \todo: consider the BDS scaling factor requirements: "the
> > >> + * downscaling factor must be an integer value multiple of
> > >> + * 1/32"
> > >
> > > While at it, s/consider/Consider/ and s/32"/32"./
> > >
> > >> + */
> > >> + if (cfg.width % 8 || cfg.height % 4) {
> > >> + LOG(IPU3, Error)
> > >> + << "Invalid stream size: bad alignment";
> > >> + return -EINVAL;
> > >> + }
> > >>
> > >> - if (cfg.width > cio2->maxSize_.width ||
> > >> - cfg.height > cio2->maxSize_.height) {
> > >> - LOG(IPU3, Error)
> > >> - << "Invalid stream size: larger than sensor resolution";
> > >> - return -EINVAL;
> > >> + if (cfg.width > cio2->maxSize_.width ||
> > >> + cfg.height > cio2->maxSize_.height) {
> > >> + LOG(IPU3, Error)
> > >> + << "Invalid stream size: larger than sensor resolution";
> > >> + return -EINVAL;
> > >> + }
> > >> +
> > >> + LOG(IPU3, Info)
> > >> + << "Requested image format " << cfg.width << "x"
> > >> + << cfg.height << "-0x" << std::hex << std::setw(8)
> > >> + << cfg.pixelFormat << " on camera'"
> > >> + << camera->name() << "'";
> > >
> > > That will be a bit confusing if you don't print the stream name, again,
> > > with a Stream subclass, you could easily get the name :-)
> > >
> > >> +
> > >> + /*
> > >> + * FIXME: As viewfinder should be operated even when
> > >> + * applications do not intend to use it, we need to keep track
> > >> + * of which streams have to be configured, to make meaningful
> > >> + * decisions at configure and request queueing time.
> > >> + *
> > >> + * Walk here all the streams to configure and collect the
> > >> + * active ones in a bitmaks.
> > >> + */
> > >> + if (isOutput(data, stream))
> > >> + setOutputActive(data);
> > >> + if (isViewfinder(data, stream))
> > >> + setViewfinderActive(data);
> > >> +
> > >> + /*
> > >> + * Collect the maximum width and height: IPU3 can downscale
> > >> + * only.
> > >> + */
> > >> + if (cfg.width > CIO2Config.width)
> > >> + CIO2Config.width = cfg.width;
> > >> + if (cfg.height > CIO2Config.height)
> > >> + CIO2Config.height = cfg.height;
> > >
> > > Sounds good, but it's a bit strange to store that in a
> > > StreamConfiguration, given that you only use it to configure the CIO2
> > > and the ImgU input. How about replacing that by Size ?
> >
> > That's also a possibility, I only use the width and height (I now
> > wonder what should I do with the configuration's pixelformat fields though)
> >
> > >> }
> > >>
> > >> /*
> > >> @@ -281,26 +366,62 @@ int PipelineHandlerIPU3::configureStreams(Camera *camera,
> > >> * adjusted format to be propagated to the ImgU output devices.
> > >> */
> > >> V4L2DeviceFormat cio2Format = {};
> > >> - ret = cio2->configure(cfg, &cio2Format);
> > >> + ret = cio2->configure(CIO2Config, &cio2Format);
> > >> if (ret)
> > >> return ret;
> > >>
> > >> - ret = imgu->configureInput(cfg, &cio2Format);
> > >> + ret = imgu->configureInput(CIO2Config, &cio2Format);
> > >> if (ret)
> > >> return ret;
> > >>
> > >> /* Apply the format to the ImgU output, viewfinder and stat. */
> > >> - ret = imgu->configureOutput(&imgu->output_, cfg);
> > >> - if (ret)
> > >> - return ret;
> > >> + for (auto const &streamConfig : config) {
> > >> + Stream *stream = streamConfig.first;
> > >> + const StreamConfiguration &cfg = streamConfig.second;
> > >>
> > >> - ret = imgu->configureOutput(&imgu->viewfinder_, cfg);
> > >> - if (ret)
> > >> - return ret;
> > >> + if (isOutput(data, stream)) {
> > >> + ret = imgu->configureOutput(&imgu->output_, cfg);
> > >> + if (ret)
> > >> + return ret;
> > >>
> > >> - ret = imgu->configureOutput(&imgu->stat_, cfg);
> > >> - if (ret)
> > >> - return ret;
> > >> + ret = imgu->configureOutput(&imgu->stat_, cfg);
> > >> + if (ret)
> > >> + return ret;
> > >> +
> > >> +
> > >> + if (isViewfinderActive(data))
> > >> + continue;
> > >> +
> > >> + /*
> > >> + * Configure viewfinder using the output stream
> > >> + * configuration if it is not part of the active
> > >> + * streams list.
> > >> + */
> > >> + ret = imgu->configureOutput(&imgu->viewfinder_, cfg);
> > >> + if (ret)
> > >> + return ret;
> > >> + } else if (isViewfinder(data, stream)) {
> > >> + ret = imgu->configureOutput(&imgu->viewfinder_, cfg);
> > >> + if (ret)
> > >> + return ret;
> > >> +
> > >> + if (isOutputActive(data))
> > >> + continue;
> > >> +
> > >> + /*
> > >> + * Configure output using the viewfinder stream
> > >> + * configuration if it is not part of the active
> > >> + * streams list.
> > >> + */
> > >> + ret = imgu->configureOutput(&imgu->output_, cfg);
> > >> + if (ret)
> > >> + return ret;
> > >> +
> > >> + ret = imgu->configureOutput(&imgu->stat_, cfg);
> > >> + if (ret)
> > >> + return ret;
> > >> + }
> > >> + }
> > >
> > > This looks quite complicated. I think you can configure the streams
> > > included in the configuration in the loop without caring about their
> > > type, and then configure the streams that are not included outside of
> > > the loop. The code would look simpler.
> >
> > Niklas had a similar idea, I'll try to see how it looks like.
> >
> > >> return 0;
> > >> }
> > >> @@ -404,7 +525,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> > >> {
> > >> IPU3CameraData *data = cameraData(camera);
> > >> V4L2Device *output = data->imgu_->output_.dev;
> > >> - Stream *stream = &data->stream_;
> > >> + Stream *stream = &data->outStream_;
> > >>
> > >> /* Queue a buffer to the ImgU output for capture. */
> > >> Buffer *buffer = request->findBuffer(stream);
> > >> @@ -554,7 +675,10 @@ int PipelineHandlerIPU3::registerCameras()
> > >> for (unsigned int id = 0; id < 4 && numCameras < 2; ++id) {
> > >> std::unique_ptr<IPU3CameraData> data =
> > >> utils::make_unique<IPU3CameraData>(this);
> > >> - std::set<Stream *> streams{ &data->stream_ };
> > >> + std::set<Stream *> streams = {
> > >> + &data->outStream_,
> > >> + &data->vfStream_,
> > >> + };
> > >> CIO2Device *cio2 = &data->cio2_;
> > >>
> > >> ret = cio2->init(cio2MediaDev_.get(), id);
>
> --
> 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/20190408/1719cd3d/attachment-0001.sig>
More information about the libcamera-devel
mailing list