[libcamera-devel] [PATCH v2 1/6] pipeline: raspberrypi: Refactor stream configuration routine

[Naushir Patuck]

Hi Kieran,

Thank you for your review feedback:

On Fri, 22 Jan 2021 at 11:38, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Hi Naush,
>
> On 22/01/2021 09:25, Naushir Patuck wrote:
> > Refactor the high/low resolution stream format and output selection
> > routine. This change is in preparation of adding 1/4 resolution output
> > for fast colour denoise.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 57 ++++++-------------
> >  1 file changed, 16 insertions(+), 41 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 524cc960dd37..7e92f7669126 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -624,52 +624,27 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> >                       continue;
> >               }
> >
> > -             if (i == maxIndex) {
> > -                     /* ISP main output format. */
> > -                     V4L2VideoDevice *dev =
> data->isp_[Isp::Output0].dev();
> > -                     V4L2PixelFormat fourcc =
> dev->toV4L2PixelFormat(cfg.pixelFormat);
> > -                     format.size = cfg.size;
> > -                     format.fourcc = fourcc;
> > -
> > -                     ret = dev->setFormat(&format);
> > -                     if (ret)
> > -                             return -EINVAL;
> > -
> > -                     if (format.size != cfg.size || format.fourcc !=
> fourcc) {
> > -                             LOG(RPI, Error)
> > -                                     << "Failed to set format on ISP
> capture0 device: "
> > -                                     << format.toString();
> > -                             return -EINVAL;
> > -                     }
> > -
> > -                     cfg.setStream(&data->isp_[Isp::Output0]);
> > -                     data->isp_[Isp::Output0].setExternal(true);
> > -             }
> > +             /* The largest resolution gets routed to the ISP Output 0
> node. */
> > +             RPi::Stream *stream = (i == maxIndex) ?
> &data->isp_[Isp::Output0] :
> > +
>  &data->isp_[Isp::Output1];
>
> Only minor, I think Laurent usually prefers the : below the ? in that
> style.
>

Ack.


>
>
>
> >
> > -             /*
> > -              * ISP second output format. This fallthrough means that
> if a
> > -              * second output stream has not been configured, we simply
> use
> > -              * the Output0 configuration.
> > -              */
> > -             V4L2VideoDevice *dev = data->isp_[Isp::Output1].dev();
> > -             format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
> > +             V4L2PixelFormat fourcc =
> stream->dev()->toV4L2PixelFormat(cfg.pixelFormat);
> >               format.size = cfg.size;
> > +             format.fourcc = fourcc;
> >
> > -             ret = dev->setFormat(&format);
> > -             if (ret) {
> > +             ret = stream->dev()->setFormat(&format);
> > +             if (ret)
> > +                     return -EINVAL;
> > +
> > +             if (format.size != cfg.size || format.fourcc != fourcc) {
> >                       LOG(RPI, Error)
> > -                             << "Failed to set format on ISP capture1
> device: "
> > -                             << format.toString();
> > -                     return ret;
> > -             }
> > -             /*
> > -              * If we have not yet provided a stream for this config, it
> > -              * means this is to be routed from Output1.
> > -              */
> > -             if (!cfg.stream()) {
> > -                     cfg.setStream(&data->isp_[Isp::Output1]);
> > -                     data->isp_[Isp::Output1].setExternal(true);
> > +                             << "Failed to set format on " <<
> stream->name()
> > +                             << " to " << format.toString();
>
> isn't format.toString() going to report what was set by setFormat(&format)?
>
> (i.e. it's not going to be the correct format that requested to be set
> 'to'.
>

Yes, you are right.  I ought to change the message text to "Failed to get
requested format, returned: " ... as I log the requested format earlier.

Regards,
Naush


>
>
>
>
> > +                     return -EINVAL;
> >               }
> > +
> > +             cfg.setStream(stream);
> > +             stream->setExternal(true);
> >       }
> >
> >       /* ISP statistics output format. */
> >
>
> --
> Regards
> --
> Kieran
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210122/8695b8b0/attachment-0001.htm>