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