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

[Laurent Pinchart]

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 ?

> 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