| Message ID | 20240425105200.41167-1-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | 5e85157bcf156137259c57a2ebc5d5616dbb2e37 |
| Headers | show |
| Series |
|
| Related | show |
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 >
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 > >
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 > > >
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);
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);
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(-)