| Message ID | 20220106113242.2383324-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | 7b84a17e213891f9c32b4b3d258c1c8343fc9c8a |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for this patch! On Thu, 6 Jan 2022 at 11:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > The libcamerasrc gstreamer component does seem to not allow stream resolutions > that are not advertised by PipelineHandler::generateConfiguration(). This has > been raised in a bug report [1]. > > Fix this behavior by advertising a SizeRange from the minimum ISP resolution, up > to the sensor resolution from PipelineHandlerRPi::generateConfiguration(). > > [1] https://bugs.libcamera.org/show_bug.cgi?id=105 > > Fixes: f16acb275c85 ("pipeline: raspberrypi: Restrict the advertised maximum ISP output resolution") > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index b5c687da467f..220a5749c0d9 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -639,8 +639,11 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > */ > for (const auto &format : fmts) { > PixelFormat pf = format.first.toPixelFormat(); > - if (pf.isValid()) > - deviceFormats[pf].emplace_back(sensorSize); > + if (pf.isValid()) { > + const SizeRange &ispSizes = format.second[0]; > + deviceFormats[pf].emplace_back(ispSizes.min, sensorSize, > + ispSizes.hStep, ispSizes.vStep); > + } Yes, looks fine to me. Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Just one related question... I wonder if we should munge those sizes (and step values) so that applications only ever see values where the width and stride match. If I do that then cheese (which uses gstreamer) actually produces non-garbled images. Hurray (never seen that before)! Though trying to run cheese using v4l2-compat.so seems to fail whatever I try. *sigh* David > } > } > > -- > 2.25.1 >
Hi David, On Thu, 6 Jan 2022 at 16:16, David Plowman <david.plowman@raspberrypi.com> wrote: > Hi Naush > > Thanks for this patch! > > On Thu, 6 Jan 2022 at 11:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > The libcamerasrc gstreamer component does seem to not allow stream > resolutions > > that are not advertised by PipelineHandler::generateConfiguration(). > This has > > been raised in a bug report [1]. > > > > Fix this behavior by advertising a SizeRange from the minimum ISP > resolution, up > > to the sensor resolution from > PipelineHandlerRPi::generateConfiguration(). > > > > [1] https://bugs.libcamera.org/show_bug.cgi?id=105 > > > > Fixes: f16acb275c85 ("pipeline: raspberrypi: Restrict the advertised > maximum ISP output resolution") > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index b5c687da467f..220a5749c0d9 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -639,8 +639,11 @@ CameraConfiguration > *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > */ > > for (const auto &format : fmts) { > > PixelFormat pf = > format.first.toPixelFormat(); > > - if (pf.isValid()) > > - > deviceFormats[pf].emplace_back(sensorSize); > > + if (pf.isValid()) { > > + const SizeRange &ispSizes = > format.second[0]; > > + > deviceFormats[pf].emplace_back(ispSizes.min, sensorSize, > > + > ispSizes.hStep, ispSizes.vStep); > > + } > > Yes, looks fine to me. > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Just one related question... I wonder if we should munge those sizes > (and step values) so that applications only ever see values where the > width and stride match. > > If I do that then cheese (which uses gstreamer) actually produces > non-garbled images. Hurray (never seen that before)! Though trying to > run cheese using v4l2-compat.so seems to fail whatever I try. *sigh* > I did think about doing this, but wasn't sure if it was how we should solve this problem... Still unsure! Regards, Naush > > David > > > } > > } > > > > -- > > 2.25.1 > > >
Quoting David Plowman (2022-01-06 16:16:47) > Hi Naush > > Thanks for this patch! > > On Thu, 6 Jan 2022 at 11:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > The libcamerasrc gstreamer component does seem to not allow stream resolutions > > that are not advertised by PipelineHandler::generateConfiguration(). This has > > been raised in a bug report [1]. > > > > Fix this behavior by advertising a SizeRange from the minimum ISP resolution, up > > to the sensor resolution from PipelineHandlerRPi::generateConfiguration(). > > > > [1] https://bugs.libcamera.org/show_bug.cgi?id=105 > > > > Fixes: f16acb275c85 ("pipeline: raspberrypi: Restrict the advertised maximum ISP output resolution") > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index b5c687da467f..220a5749c0d9 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -639,8 +639,11 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > */ > > for (const auto &format : fmts) { > > PixelFormat pf = format.first.toPixelFormat(); > > - if (pf.isValid()) > > - deviceFormats[pf].emplace_back(sensorSize); > > + if (pf.isValid()) { > > + const SizeRange &ispSizes = format.second[0]; > > + deviceFormats[pf].emplace_back(ispSizes.min, sensorSize, > > + ispSizes.hStep, ispSizes.vStep); > > + } > > Yes, looks fine to me. > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Just one related question... I wonder if we should munge those sizes > (and step values) so that applications only ever see values where the > width and stride match. > > If I do that then cheese (which uses gstreamer) actually produces > non-garbled images. Hurray (never seen that before)! Though trying to > run cheese using v4l2-compat.so seems to fail whatever I try. *sigh* I think if you try to run cheese with the v4l2-compat layer, while you have the gstreamer plugin available they'll conflict, as we can only have one CameraManager and you'll end up with two ... Perhaps that could be fixed somehow in the future if they could both end up mapping to the same one in the same process contexts ... This patch seems good though, I interpret it as exposing all the size options available from the isp min, only up to the sensor resolution which seems quite reasonable. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> -- Kieran > > David > > > } > > } > > > > -- > > 2.25.1 > >
Hi Kieran Thanks for the comments. On Fri, 7 Jan 2022 at 15:34, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting David Plowman (2022-01-06 16:16:47) > > Hi Naush > > > > Thanks for this patch! > > > > On Thu, 6 Jan 2022 at 11:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > The libcamerasrc gstreamer component does seem to not allow stream resolutions > > > that are not advertised by PipelineHandler::generateConfiguration(). This has > > > been raised in a bug report [1]. > > > > > > Fix this behavior by advertising a SizeRange from the minimum ISP resolution, up > > > to the sensor resolution from PipelineHandlerRPi::generateConfiguration(). > > > > > > [1] https://bugs.libcamera.org/show_bug.cgi?id=105 > > > > > > Fixes: f16acb275c85 ("pipeline: raspberrypi: Restrict the advertised maximum ISP output resolution") > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index b5c687da467f..220a5749c0d9 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -639,8 +639,11 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > > */ > > > for (const auto &format : fmts) { > > > PixelFormat pf = format.first.toPixelFormat(); > > > - if (pf.isValid()) > > > - deviceFormats[pf].emplace_back(sensorSize); > > > + if (pf.isValid()) { > > > + const SizeRange &ispSizes = format.second[0]; > > > + deviceFormats[pf].emplace_back(ispSizes.min, sensorSize, > > > + ispSizes.hStep, ispSizes.vStep); > > > + } > > > > Yes, looks fine to me. > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > Just one related question... I wonder if we should munge those sizes > > (and step values) so that applications only ever see values where the > > width and stride match. > > > > If I do that then cheese (which uses gstreamer) actually produces > > non-garbled images. Hurray (never seen that before)! Though trying to > > run cheese using v4l2-compat.so seems to fail whatever I try. *sigh* > > I think if you try to run cheese with the v4l2-compat layer, while you > have the gstreamer plugin available they'll conflict, as we can only > have one CameraManager and you'll end up with two ... > > Perhaps that could be fixed somehow in the future if they could both end > up mapping to the same one in the same process contexts ... > > This patch seems good though, I interpret it as exposing all the size > options available from the isp min, only up to the sensor resolution > which seems quite reasonable. Would you have a view on the alignment question? (i.e. only advertising sizes where the width happens to be such that the stride matches it exactly with no padding bytes) Thanks David > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > -- > Kieran > > > > > > David > > > > > } > > > } > > > > > > -- > > > 2.25.1 > > >
Hi David, Quoting David Plowman (2022-01-07 15:37:10) > Hi Kieran > > Thanks for the comments. > > On Fri, 7 Jan 2022 at 15:34, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Quoting David Plowman (2022-01-06 16:16:47) > > > Hi Naush > > > > > > Thanks for this patch! > > > > > > On Thu, 6 Jan 2022 at 11:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > > > The libcamerasrc gstreamer component does seem to not allow stream resolutions > > > > that are not advertised by PipelineHandler::generateConfiguration(). This has > > > > been raised in a bug report [1]. > > > > > > > > Fix this behavior by advertising a SizeRange from the minimum ISP resolution, up > > > > to the sensor resolution from PipelineHandlerRPi::generateConfiguration(). > > > > > > > > [1] https://bugs.libcamera.org/show_bug.cgi?id=105 > > > > > > > > Fixes: f16acb275c85 ("pipeline: raspberrypi: Restrict the advertised maximum ISP output resolution") > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 7 +++++-- > > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index b5c687da467f..220a5749c0d9 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -639,8 +639,11 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > > > */ > > > > for (const auto &format : fmts) { > > > > PixelFormat pf = format.first.toPixelFormat(); > > > > - if (pf.isValid()) > > > > - deviceFormats[pf].emplace_back(sensorSize); > > > > + if (pf.isValid()) { > > > > + const SizeRange &ispSizes = format.second[0]; > > > > + deviceFormats[pf].emplace_back(ispSizes.min, sensorSize, > > > > + ispSizes.hStep, ispSizes.vStep); > > > > + } > > > > > > Yes, looks fine to me. > > > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > > > Just one related question... I wonder if we should munge those sizes > > > (and step values) so that applications only ever see values where the > > > width and stride match. > > > > > > If I do that then cheese (which uses gstreamer) actually produces > > > non-garbled images. Hurray (never seen that before)! Though trying to > > > run cheese using v4l2-compat.so seems to fail whatever I try. *sigh* > > > > I think if you try to run cheese with the v4l2-compat layer, while you > > have the gstreamer plugin available they'll conflict, as we can only > > have one CameraManager and you'll end up with two ... > > > > Perhaps that could be fixed somehow in the future if they could both end > > up mapping to the same one in the same process contexts ... > > > > This patch seems good though, I interpret it as exposing all the size > > options available from the isp min, only up to the sensor resolution > > which seems quite reasonable. > > Would you have a view on the alignment question? (i.e. only > advertising sizes where the width happens to be such that the stride > matches it exactly with no padding bytes) I would say if there are known limitations, then it is the responsibility of the PH to ensure it only reports what is supported. Though in this case, it's a little bit dubious as the padding bytes are supported, but I think we're likely not conveying the stride correctly to the gstreamer element perhaps. So I think that's all worth some investigation, but on top. I'll create a bugzilla entry to track it, as I think this patch could be merged without waiting for that. https://bugs.libcamera.org/show_bug.cgi?id=108 Either we should correctly convey the stride information to gstreamer, and it should make use of it accordingly, or we should not support that size. (or if the limitation is on gstreamer, then it should not use a size it can't support). -- Kieran > > Thanks > David > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > -- > > Kieran > > > > > > > > > > David > > > > > > > } > > > > } > > > > > > > > -- > > > > 2.25.1 > > > >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index b5c687da467f..220a5749c0d9 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -639,8 +639,11 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, */ for (const auto &format : fmts) { PixelFormat pf = format.first.toPixelFormat(); - if (pf.isValid()) - deviceFormats[pf].emplace_back(sensorSize); + if (pf.isValid()) { + const SizeRange &ispSizes = format.second[0]; + deviceFormats[pf].emplace_back(ispSizes.min, sensorSize, + ispSizes.hStep, ispSizes.vStep); + } } }
The libcamerasrc gstreamer component does seem to not allow stream resolutions that are not advertised by PipelineHandler::generateConfiguration(). This has been raised in a bug report [1]. Fix this behavior by advertising a SizeRange from the minimum ISP resolution, up to the sensor resolution from PipelineHandlerRPi::generateConfiguration(). [1] https://bugs.libcamera.org/show_bug.cgi?id=105 Fixes: f16acb275c85 ("pipeline: raspberrypi: Restrict the advertised maximum ISP output resolution") Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)