pipeline: rpi: Avoid duplicating size range for the same pixel format
diff mbox series

Message ID 20240425105200.41167-1-david.plowman@raspberrypi.com
State Accepted
Commit 5e85157bcf156137259c57a2ebc5d5616dbb2e37
Headers show
Series
  • pipeline: rpi: Avoid duplicating size range for the same pixel format
Related show

Commit Message

David Plowman April 25, 2024, 10:52 a.m. UTC
Some V4L2 formats translate to the same pixel format, e.g. YU12 and
YM12 both produce YUV420. In this case our ISP driver advertises the
same size range for both, but we must not record the same thing twice
for the same pixel format (which will cause a failure later on).

Instead, ignore the V4l2 format if the pixel format has already been
seen.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Naushir Patuck April 25, 2024, 11:37 a.m. UTC | #1
Hi David,

Thank you for this patch.

On Thu, 25 Apr 2024 at 11:52, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> Some V4L2 formats translate to the same pixel format, e.g. YU12 and
> YM12 both produce YUV420. In this case our ISP driver advertises the
> same size range for both, but we must not record the same thing twice
> for the same pixel format (which will cause a failure later on).
>
> Instead, ignore the V4l2 format if the pixel format has already been
> seen.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

> ---
>  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 7e420b3f..0034e06d 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -474,7 +474,11 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
>                          */
>                         for (const auto &format : fmts) {
>                                 PixelFormat pf = format.first.toPixelFormat();
> -                               if (pf.isValid()) {
> +                               /*
> +                                * Some V4L2 formats translate to the same pixel format (e.g. YU12, YM12
> +                                * both give YUV420). We must avoid duplicating the range in this case.
> +                                */
> +                               if (pf.isValid() && deviceFormats.find(pf) == deviceFormats.end()) {
>                                         const SizeRange &ispSizes = format.second[0];
>                                         deviceFormats[pf].emplace_back(ispSizes.min, sensorSize,
>                                                                        ispSizes.hStep, ispSizes.vStep);
> --
> 2.39.2
>
Jacopo Mondi April 26, 2024, 1:06 p.m. UTC | #2
Hi David

On Thu, Apr 25, 2024 at 12:37:48PM +0100, Naushir Patuck wrote:
> Hi David,
>
> Thank you for this patch.
>
> On Thu, 25 Apr 2024 at 11:52, David Plowman
> <david.plowman@raspberrypi.com> wrote:
> >
> > Some V4L2 formats translate to the same pixel format, e.g. YU12 and
> > YM12 both produce YUV420. In this case our ISP driver advertises the
> > same size range for both, but we must not record the same thing twice
> > for the same pixel format (which will cause a failure later on).

Am I wrong or this mostly (only) apply to contiguous and non-contiguous
format versions ?

> >
> > Instead, ignore the V4l2 format if the pixel format has already been
> > seen.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

>
> > ---
> >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 7e420b3f..0034e06d 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -474,7 +474,11 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
> >                          */
> >                         for (const auto &format : fmts) {
> >                                 PixelFormat pf = format.first.toPixelFormat();
> > -                               if (pf.isValid()) {
> > +                               /*
> > +                                * Some V4L2 formats translate to the same pixel format (e.g. YU12, YM12
> > +                                * both give YUV420). We must avoid duplicating the range in this case.
> > +                                */
> > +                               if (pf.isValid() && deviceFormats.find(pf) == deviceFormats.end()) {

Or
                                    if (pf.isValid() && !deviceFormats.count(pf)) {


The change is anyway good!
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> >                                         const SizeRange &ispSizes = format.second[0];
> >                                         deviceFormats[pf].emplace_back(ispSizes.min, sensorSize,
> >                                                                        ispSizes.hStep, ispSizes.vStep);
> > --
> > 2.39.2
> >
David Plowman April 26, 2024, 1:16 p.m. UTC | #3
Hi Jacopo

Thanks for reviewing the patch!

On Fri, 26 Apr 2024 at 14:06, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi David
>
> On Thu, Apr 25, 2024 at 12:37:48PM +0100, Naushir Patuck wrote:
> > Hi David,
> >
> > Thank you for this patch.
> >
> > On Thu, 25 Apr 2024 at 11:52, David Plowman
> > <david.plowman@raspberrypi.com> wrote:
> > >
> > > Some V4L2 formats translate to the same pixel format, e.g. YU12 and
> > > YM12 both produce YUV420. In this case our ISP driver advertises the
> > > same size range for both, but we must not record the same thing twice
> > > for the same pixel format (which will cause a failure later on).
>
> Am I wrong or this mostly (only) apply to contiguous and non-contiguous
> format versions ?

I think that's right. I quoted the YU12/YM12 example, but we have the
same thing with NV12 contiguous/non-contiguous and probably a few
others as well.

>
> > >
> > > Instead, ignore the V4l2 format if the pixel format has already been
> > > seen.
> > >
> > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> >
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
>
> >
> > > ---
> > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > index 7e420b3f..0034e06d 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > @@ -474,7 +474,11 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
> > >                          */
> > >                         for (const auto &format : fmts) {
> > >                                 PixelFormat pf = format.first.toPixelFormat();
> > > -                               if (pf.isValid()) {
> > > +                               /*
> > > +                                * Some V4L2 formats translate to the same pixel format (e.g. YU12, YM12
> > > +                                * both give YUV420). We must avoid duplicating the range in this case.
> > > +                                */
> > > +                               if (pf.isValid() && deviceFormats.find(pf) == deviceFormats.end()) {
>
> Or
>                                     if (pf.isValid() && !deviceFormats.count(pf)) {

True, I mostly don't think to use count because it's not such a good
way of testing "are there any?". But for "are there none?" of course
it's fine!

Thanks!
David

>
>
> The change is anyway good!
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> > >                                         const SizeRange &ispSizes = format.second[0];
> > >                                         deviceFormats[pf].emplace_back(ispSizes.min, sensorSize,
> > >                                                                        ispSizes.hStep, ispSizes.vStep);
> > > --
> > > 2.39.2
> > >
Laurent Pinchart May 9, 2024, 9:52 a.m. UTC | #4
Hello,

On Fri, Apr 26, 2024 at 02:16:03PM +0100, David Plowman wrote:
> On Fri, 26 Apr 2024 at 14:06, Jacopo Mondi wrote:
> > On Thu, Apr 25, 2024 at 12:37:48PM +0100, Naushir Patuck wrote:
> > > On Thu, 25 Apr 2024 at 11:52, David Plowman wrote:
> > > >
> > > > Some V4L2 formats translate to the same pixel format, e.g. YU12 and
> > > > YM12 both produce YUV420. In this case our ISP driver advertises the
> > > > same size range for both, but we must not record the same thing twice
> > > > for the same pixel format (which will cause a failure later on).
> >
> > Am I wrong or this mostly (only) apply to contiguous and non-contiguous
> > format versions ?
> 
> I think that's right. I quoted the YU12/YM12 example, but we have the
> same thing with NV12 contiguous/non-contiguous and probably a few
> others as well.
> 
> > > > Instead, ignore the V4l2 format if the pixel format has already been
> > > > seen.
> > > >
> > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > >
> > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> >
> > > > ---
> > > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 6 +++++-
> > > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > index 7e420b3f..0034e06d 100644
> > > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > > @@ -474,7 +474,11 @@ PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
> > > >                          */
> > > >                         for (const auto &format : fmts) {
> > > >                                 PixelFormat pf = format.first.toPixelFormat();
> > > > -                               if (pf.isValid()) {
> > > > +                               /*
> > > > +                                * Some V4L2 formats translate to the same pixel format (e.g. YU12, YM12
> > > > +                                * both give YUV420). We must avoid duplicating the range in this case.
> > > > +                                */
> > > > +                               if (pf.isValid() && deviceFormats.find(pf) == deviceFormats.end()) {
> >
> > Or
> >                                     if (pf.isValid() && !deviceFormats.count(pf)) {
> 
> True, I mostly don't think to use count because it's not such a good
> way of testing "are there any?". But for "are there none?" of course
> it's fine!

I'm fine either way. std::map::contains() will be nice, once we move to
C++20.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > The change is anyway good!
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > > >                                         const SizeRange &ispSizes = format.second[0];
> > > >                                         deviceFormats[pf].emplace_back(ispSizes.min, sensorSize,
> > > >                                                                        ispSizes.hStep, ispSizes.vStep);

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
index 7e420b3f..0034e06d 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
@@ -474,7 +474,11 @@  PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole
 			 */
 			for (const auto &format : fmts) {
 				PixelFormat pf = format.first.toPixelFormat();
-				if (pf.isValid()) {
+				/*
+				 * Some V4L2 formats translate to the same pixel format (e.g. YU12, YM12
+				 * both give YUV420). We must avoid duplicating the range in this case.
+				 */
+				if (pf.isValid() && deviceFormats.find(pf) == deviceFormats.end()) {
 					const SizeRange &ispSizes = format.second[0];
 					deviceFormats[pf].emplace_back(ispSizes.min, sensorSize,
 								       ispSizes.hStep, ispSizes.vStep);