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

[David Plowman]

Actually I found myself looking at this bit of code again as part of
my occasional quest to figure out why the gstreamer element and v4l2
compatibility library work so poorly.

I was wondering if we should limit the output sizes to the max sensor
resolution here. The trouble with advertising 16384x16384 is that
applications (cheese, for example) decide that we really want 8K video
and unsurprisingly it's all a complete disaster.

What do you think - is this the right place to tackle the problem? (Or
if not, where else?)

Though I've not checked, maybe this would help the v4l2 compatibility
layer too? (anyone know?)

David

On Fri, 3 Dec 2021 at 12:40, David Plowman
<david.plowman at raspberrypi.com> wrote:
>
> Hi Naush
>
> Thanks for this patch!
>
> On Fri, 3 Dec 2021 at 11:32, Naushir Patuck <naush at raspberrypi.com> wrote:
> >
> > 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 won't even begin to pretend that I understand this! I guess I wonder
> just a little bit whether one could write it in a more "obvious" way
> that the compiler could still optimise, but really I have no clue. So
> anyway:
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks!
> David
>
> > +                       }
> > +               } 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. */
> > --
> > 2.25.1
> >