[libcamera-devel] [PATCH v1] pipeline: raspberrypi: Return the sensor formats from generateConfiguration()

[Naushir Patuck]

Hi all,

Thank you for your reviews!

On Mon, 6 Dec 2021 at 17:49, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hello,
>
> On Mon, Dec 06, 2021 at 05:19:56PM +0000, Kieran Bingham wrote:
> > Quoting Naushir Patuck (2021-12-03 11:32:05)
> > > Return the available sensor PixelFormats and sizes from
> generateConfiguration()
> > > if the StreamRole is set to StreamRole::Raw. The existing code returns
> the
> > > PixelFormats and sizes for all other StreamRole types.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 21 ++++++++++++++-----
> > >  1 file changed, 16 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 5b76916e9e98..cbfb58562626 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -595,12 +595,23 @@ CameraConfiguration
> *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> > >                         return nullptr;
> > >                 }
> > >
> > > -               /* Translate the V4L2PixelFormat to PixelFormat. */
> > >                 std::map<PixelFormat, std::vector<SizeRange>>
> deviceFormats;
> > > -               for (const auto &format : fmts) {
> > > -                       PixelFormat pf = format.first.toPixelFormat();
> > > -                       if (pf.isValid())
> > > -                               deviceFormats[pf] = format.second;
> > > +               if (role == StreamRole::Raw) {
> > > +                       /* Translate the MBUS codes to a PixelFormat.
> */
> > > +                       for (const auto &format :
> data->sensorFormats_) {
> > > +                               PixelFormat pf =
> mbusCodeToPixelFormat(format.first,
> > > +
> BayerFormat::Packing::CSI2);
> > > +                               if (pf.isValid())
> > > +
>  deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf),
> > > +
>  std::forward_as_tuple(format.second.begin(), format.second.end()));
> >
> > I can't tell right now if this could be simpler, but I think it's fine
> > as it is if it works for you and produces the right result.
>
> That's likely the shortest piece of code possible, even if not the most
> explicit one (declaring an explicit intermediate std::vector<SizeRange>
> would be more readable I think).
>

This is the only way I could think of constructing deviceFormat without an
intermediate temporary std::vector.  If others feel this construct is
somewhat
obfuscated, I am happy to add a temporary for readability.  If not, then
feel
free to merge this :-)

Regards,
Naush


>
> > I think this functionality could or rather /should/ be done in the
> > CameraSensor class to return these formats to simplify this for each
> > pipeline handler, as they should all do this in the same way if they use
> > a CameraSensor.
>
> Except that how a media bus code is translated to a pixel format is
> specific to the video device, it's not an intrinsic property of the
> camera sensor.
>
> > However, I think that move to CameraSensor and adapting of the other
> > handlers can be done on top.
> >
> > So,
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > > +                       }
> > > +               } else {
> > > +                       /* Translate the V4L2PixelFormat to
> PixelFormat. */
> > > +                       for (const auto &format : fmts) {
> > > +                               PixelFormat pf =
> format.first.toPixelFormat();
> > > +                               if (pf.isValid())
> > > +                                       deviceFormats[pf] =
> format.second;
> > > +                       }
> > >                 }
> > >
> > >                 /* Add the stream format based on the device node used
> for the use case. */
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211207/6f30dd9c/attachment.htm>