| Message ID | 20211118164216.2669449-5-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | dbd2e30ee3bfd1dded8048693d577dded664d05b |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. 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@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 ? > const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > if (!info.isValid()) > return false;
Hi Laurent, Thanks for your feedback. On Thu, 18 Nov 2021 at 19:55, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > 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@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 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; } } This change may not pick up all invalid formats where fourcc !=0, but should be sufficient for this particular usage. Regards, Naush > > > const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); > > if (!info.isValid()) > > return false; > > -- > Regards, > > 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@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;
On Fri, Nov 19, 2021 at 02:12:55PM +0200, Laurent Pinchart 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@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 ? And for this patch, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > 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;
Hi Laurent, On Fri, 19 Nov 2021 at 12:13, Laurent Pinchart < laurent.pinchart@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@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 > > > 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 >
Quoting Naushir Patuck (2021-11-19 16:12:58) > Hi Laurent, > > > On Fri, 19 Nov 2021 at 12:13, Laurent Pinchart < > laurent.pinchart@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@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? 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@ideasonboard.com> If you're happy, I'll run the integration tests and merge this series. -- 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 > >
Hi Kieran, On Tue, 23 Nov 2021 at 12:18, Kieran Bingham < kieran.bingham@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@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@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@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 > > > >
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; + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt); if (!info.isValid()) return false;
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@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 +++ 1 file changed, 3 insertions(+)