[libcamera-devel] [PATCH v3 4/4] pipeline: raspberrypi: Avoid invalid PixelFormat warning message

[Naushir Patuck]

Hi Kieran,

On Tue, 23 Nov 2021 at 12:18, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Quoting Naushir Patuck (2021-11-19 16:12:58)
> > Hi Laurent,
> >
> >
> > On Fri, 19 Nov 2021 at 12:13, Laurent Pinchart <
> > laurent.pinchart at ideasonboard.com> wrote:
> >
> > > Hi Naush,
> > >
> > > On Fri, Nov 19, 2021 at 08:38:00AM +0000, Naushir Patuck wrote:
> > > > On Thu, 18 Nov 2021 at 19:55, Laurent Pinchart wrote:
> > > > > On Thu, Nov 18, 2021 at 04:42:16PM +0000, Naushir Patuck wrote:
> > > > > > PixelFormatInfo::info() would log a warning message if the
> > > PixelFormat was
> > > > > > invalid when called from the isRaw() function. Add a validity
> test
> > > in isRaw()
> > > > > > to avoid this warning message.
> > > > > >
> > > > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > > > ---
> > > > > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 +++
> > > > > >  1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > index 4f6c699a4379..ad526a8be6a2 100644
> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > @@ -102,6 +102,9 @@ bool isRaw(const PixelFormat &pixFmt)
> > > > > >        * The isRaw test might be redundant right now the pipeline
> > > handler only
> > > > > >        * supports RAW sensors. Leave it in for now, just as a
> sanity
> > > check.
> > > > > >        */
> > > > > > +     if (!pixFmt.isValid())
> > > > > > +             return false;
> > > > > > +
> > > > >
> > > > > This will only catch some of the issues, as PixelFormat::isValid()
> > > > > returns false only when the fourcc is zero, while
> > > > > PixelFormatInfo::info() will warn for every unknown format.
> > > > >
> > > > > How did you trigger this issue ?
> > > >
> > > > With the change in patch 3/4, we check if a raw stream is requested
> > >
> > > I assume you meant in 2/4.
> > >
> > > > and use the buffer count value to make up at least 2 internal buffers
> > > > for unicam.  If no RAW stream is present, the pixelformat is invalid
> > > > (fourcc == 0).
> > > > This is the bit of code that triggers it:
> > > >
> > > > for (Stream *s : camera->streams()) {
> > > >     if (isRaw(s->configuration().pixelFormat)) {
> > > >         numRawBuffers = s->configuration().bufferCount;
> > > >         break;
> > > >     }
> > > > }
> > >
> > > Ah yes, I had misread the code and thought this was looping over the
> > > configuration, not over the stream. It makes sense now.
> > >
> > > I think there's one issue though. Camera::configure() stores the
> > > configuration of each stream in Stream::configuration_, which is
> > > accessible through Stream::configuration(), but it won't reset the
> > > configuration of other streams. If you configure the camera with a raw
> > > stream, and then reconfigure it with a YUV stream only, the above loop
> > > will find a raw stream.
> > >
> > > This issue isn't introduced by your patch series, but I believe it
> > > affects it. Would you be able to test that ?
> > >
> >
> > Sure, I'll give this a run next week to confirm the behavior..
> >
> > Regards,
> > Naush
> >
>
> Were there any issues to worry about here?
>

I still need to investigate the behavior, but that will be separate
to the changes in this patch.


>
> As far as I can tell - the patch itself is still sane. If the format is
> invalid, it is ... not a raw format...
>

>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
> If you're happy, I'll run the integration tests and merge this series.
>

I'm happy to get this merged now, thanks!

Naush


>
> --
> Kieran
>
>
> >
> > >
> > > > This change may not pick up all invalid formats where fourcc !=0, but
> > > should
> > > > be sufficient for this particular usage.
> > > >
> > > > > >       const PixelFormatInfo &info =
> PixelFormatInfo::info(pixFmt);
> > > > > >       if (!info.isValid())
> > > > > >               return false;
> > >
> > > --
> > > Regards,
> > >
> > > Laurent Pinchart
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211123/855428ec/attachment.htm>