Message ID | 20211027092803.3671096-7-naush@raspberrypi.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Quoting Naushir Patuck (2021-10-27 10:28:00) > Default to using CSI2 packed formats when setting up the Unicam image format, > but use an unpacked format if the user requests one through StreamConfiguration. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 48f561d31a31..1b78b5e74a63 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -96,14 +96,14 @@ BayerFormat mbusCodeToBayerFormat(unsigned int mbusCode) > return BayerFormat{}; > } > > -V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format) > +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format, BayerFormat::Packing packing) > { > V4L2DeviceFormat deviceFormat; > BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code); > > ASSERT(bayer.isValid()); > > - bayer.packing = BayerFormat::Packing::CSI2Packed; > + bayer.packing = packing; Ok, that really solidifies the answer to my earlier question ;-) > deviceFormat.fourcc = bayer.toV4L2PixelFormat(); > deviceFormat.size = format.size; > return deviceFormat; > @@ -413,7 +413,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > * the user request. > */ > V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size); > - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat); > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, > + BayerFormat::Packing::CSI2Packed); I believe ::Packing is optional if you want to reduce line length. But can also stay, it won't make a big difference to the line formatting. > int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat); > if (ret) > return Invalid; > @@ -631,6 +632,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > for (auto const stream : data->streams_) > stream->reset(); > > + BayerFormat::Packing packing = BayerFormat::Packing::CSI2Packed; Same. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Size maxSize, sensorSize; > unsigned int maxIndex = 0; > bool rawStream = false; > @@ -649,6 +651,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > */ > sensorSize = cfg.size; > rawStream = true; > + /* Check if the user has explicitly set an unpacked format. */ > + packing = BayerFormat::fromPixelFormat(cfg.pixelFormat).packing; > } else { > if (cfg.size > maxSize) { > maxSize = config->at(i).size; > @@ -667,7 +671,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > * Unicam image output format. The ISP input format gets set at start, > * just in case we have swapped bayer orders due to flips. > */ > - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat); > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, packing); > ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); > if (ret) > return ret; > -- > 2.25.1 >
Hi Kieran, Thank you for your feedback. On Wed, 27 Oct 2021 at 13:17, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck (2021-10-27 10:28:00) > > Default to using CSI2 packed formats when setting up the Unicam image > format, > > but use an unpacked format if the user requests one through > StreamConfiguration. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 48f561d31a31..1b78b5e74a63 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -96,14 +96,14 @@ BayerFormat mbusCodeToBayerFormat(unsigned int > mbusCode) > > return BayerFormat{}; > > } > > > > -V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format) > > +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format, > BayerFormat::Packing packing) > > { > > V4L2DeviceFormat deviceFormat; > > BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code); > > > > ASSERT(bayer.isValid()); > > > > - bayer.packing = BayerFormat::Packing::CSI2Packed; > > + bayer.packing = packing; > > Ok, that really solidifies the answer to my earlier question ;-) > > > > deviceFormat.fourcc = bayer.toV4L2PixelFormat(); > > deviceFormat.size = format.size; > > return deviceFormat; > > @@ -413,7 +413,8 @@ CameraConfiguration::Status > RPiCameraConfiguration::validate() > > * the user request. > > */ > > V4L2SubdeviceFormat sensorFormat = > findBestFormat(data_->sensorFormats_, cfg.size); > > - V4L2DeviceFormat unicamFormat = > toV4L2DeviceFormat(sensorFormat); > > + V4L2DeviceFormat unicamFormat = > toV4L2DeviceFormat(sensorFormat, > > + > BayerFormat::Packing::CSI2Packed); > > I believe ::Packing is optional if you want to reduce line length. But > can also stay, it won't make a big difference to the line formatting. > > > > int ret = > data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat); > > if (ret) > > return Invalid; > > @@ -631,6 +632,7 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > for (auto const stream : data->streams_) > > stream->reset(); > > > > + BayerFormat::Packing packing = BayerFormat::Packing::CSI2Packed; > > Same. > Yes, I can remove the extra scoping to be more consistent. Naush > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Size maxSize, sensorSize; > > unsigned int maxIndex = 0; > > bool rawStream = false; > > @@ -649,6 +651,8 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > */ > > sensorSize = cfg.size; > > rawStream = true; > > + /* Check if the user has explicitly set an > unpacked format. */ > > + packing = > BayerFormat::fromPixelFormat(cfg.pixelFormat).packing; > > } else { > > if (cfg.size > maxSize) { > > maxSize = config->at(i).size; > > @@ -667,7 +671,7 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > * Unicam image output format. The ISP input format gets set at > start, > > * just in case we have swapped bayer orders due to flips. > > */ > > - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat); > > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, > packing); > > ret = > data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); > > if (ret) > > return ret; > > -- > > 2.25.1 > > >
Quoting Naushir Patuck (2021-10-27 13:39:11) > Hi Kieran, > > Thank you for your feedback. > > On Wed, 27 Oct 2021 at 13:17, Kieran Bingham < > kieran.bingham@ideasonboard.com> wrote: > > > Quoting Naushir Patuck (2021-10-27 10:28:00) > > > Default to using CSI2 packed formats when setting up the Unicam image > > format, > > > but use an unpacked format if the user requests one through > > StreamConfiguration. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 48f561d31a31..1b78b5e74a63 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -96,14 +96,14 @@ BayerFormat mbusCodeToBayerFormat(unsigned int > > mbusCode) > > > return BayerFormat{}; > > > } > > > > > > -V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format) > > > +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format, > > BayerFormat::Packing packing) > > > { > > > V4L2DeviceFormat deviceFormat; > > > BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code); > > > > > > ASSERT(bayer.isValid()); > > > > > > - bayer.packing = BayerFormat::Packing::CSI2Packed; > > > + bayer.packing = packing; > > > > Ok, that really solidifies the answer to my earlier question ;-) > > > > > > > deviceFormat.fourcc = bayer.toV4L2PixelFormat(); > > > deviceFormat.size = format.size; > > > return deviceFormat; > > > @@ -413,7 +413,8 @@ CameraConfiguration::Status > > RPiCameraConfiguration::validate() > > > * the user request. > > > */ > > > V4L2SubdeviceFormat sensorFormat = > > findBestFormat(data_->sensorFormats_, cfg.size); > > > - V4L2DeviceFormat unicamFormat = > > toV4L2DeviceFormat(sensorFormat); > > > + V4L2DeviceFormat unicamFormat = > > toV4L2DeviceFormat(sensorFormat, > > > + > > BayerFormat::Packing::CSI2Packed); > > > > I believe ::Packing is optional if you want to reduce line length. But > > can also stay, it won't make a big difference to the line formatting. > > > > > > > int ret = > > data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat); > > > if (ret) > > > return Invalid; > > > @@ -631,6 +632,7 @@ int PipelineHandlerRPi::configure(Camera *camera, > > CameraConfiguration *config) > > > for (auto const stream : data->streams_) > > > stream->reset(); > > > > > > + BayerFormat::Packing packing = BayerFormat::Packing::CSI2Packed; > > > > Same. > > > > Yes, I can remove the extra scoping to be more consistent. > Erm... given Laurent's just submitted patch, I might ahve to take back my suggestions I'm afraid. Looks like this should be /with/ ::Packing:: -- Kieran > Naush > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > Size maxSize, sensorSize; > > > unsigned int maxIndex = 0; > > > bool rawStream = false; > > > @@ -649,6 +651,8 @@ int PipelineHandlerRPi::configure(Camera *camera, > > CameraConfiguration *config) > > > */ > > > sensorSize = cfg.size; > > > rawStream = true; > > > + /* Check if the user has explicitly set an > > unpacked format. */ > > > + packing = > > BayerFormat::fromPixelFormat(cfg.pixelFormat).packing; > > > } else { > > > if (cfg.size > maxSize) { > > > maxSize = config->at(i).size; > > > @@ -667,7 +671,7 @@ int PipelineHandlerRPi::configure(Camera *camera, > > CameraConfiguration *config) > > > * Unicam image output format. The ISP input format gets set at > > start, > > > * just in case we have swapped bayer orders due to flips. > > > */ > > > - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat); > > > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, > > packing); > > > ret = > > data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); > > > if (ret) > > > return ret; > > > -- > > > 2.25.1 > > > > >
On Wed, 27 Oct 2021 at 13:50, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck (2021-10-27 13:39:11) > > Hi Kieran, > > > > Thank you for your feedback. > > > > On Wed, 27 Oct 2021 at 13:17, Kieran Bingham < > > kieran.bingham@ideasonboard.com> wrote: > > > > > Quoting Naushir Patuck (2021-10-27 10:28:00) > > > > Default to using CSI2 packed formats when setting up the Unicam image > > > format, > > > > but use an unpacked format if the user requests one through > > > StreamConfiguration. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index 48f561d31a31..1b78b5e74a63 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -96,14 +96,14 @@ BayerFormat mbusCodeToBayerFormat(unsigned int > > > mbusCode) > > > > return BayerFormat{}; > > > > } > > > > > > > > -V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format) > > > > +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format, > > > BayerFormat::Packing packing) > > > > { > > > > V4L2DeviceFormat deviceFormat; > > > > BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code); > > > > > > > > ASSERT(bayer.isValid()); > > > > > > > > - bayer.packing = BayerFormat::Packing::CSI2Packed; > > > > + bayer.packing = packing; > > > > > > Ok, that really solidifies the answer to my earlier question ;-) > > > > > > > > > > deviceFormat.fourcc = bayer.toV4L2PixelFormat(); > > > > deviceFormat.size = format.size; > > > > return deviceFormat; > > > > @@ -413,7 +413,8 @@ CameraConfiguration::Status > > > RPiCameraConfiguration::validate() > > > > * the user request. > > > > */ > > > > V4L2SubdeviceFormat sensorFormat = > > > findBestFormat(data_->sensorFormats_, cfg.size); > > > > - V4L2DeviceFormat unicamFormat = > > > toV4L2DeviceFormat(sensorFormat); > > > > + V4L2DeviceFormat unicamFormat = > > > toV4L2DeviceFormat(sensorFormat, > > > > + > > > BayerFormat::Packing::CSI2Packed); > > > > > > I believe ::Packing is optional if you want to reduce line length. But > > > can also stay, it won't make a big difference to the line formatting. > > > > > > > > > > int ret = > > > data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat); > > > > if (ret) > > > > return Invalid; > > > > @@ -631,6 +632,7 @@ int PipelineHandlerRPi::configure(Camera *camera, > > > CameraConfiguration *config) > > > > for (auto const stream : data->streams_) > > > > stream->reset(); > > > > > > > > + BayerFormat::Packing packing = > BayerFormat::Packing::CSI2Packed; > > > > > > Same. > > > > > > > Yes, I can remove the extra scoping to be more consistent. > > > > Erm... given Laurent's just submitted patch, I might ahve to take back > my suggestions I'm afraid. > > Looks like this should be /with/ ::Packing:: > Yes, I saw that change in my inbox just after replying to your message... > > -- > Kieran > > > > > Naush > > > > > > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > Size maxSize, sensorSize; > > > > unsigned int maxIndex = 0; > > > > bool rawStream = false; > > > > @@ -649,6 +651,8 @@ int PipelineHandlerRPi::configure(Camera *camera, > > > CameraConfiguration *config) > > > > */ > > > > sensorSize = cfg.size; > > > > rawStream = true; > > > > + /* Check if the user has explicitly set an > > > unpacked format. */ > > > > + packing = > > > BayerFormat::fromPixelFormat(cfg.pixelFormat).packing; > > > > } else { > > > > if (cfg.size > maxSize) { > > > > maxSize = config->at(i).size; > > > > @@ -667,7 +671,7 @@ int PipelineHandlerRPi::configure(Camera *camera, > > > CameraConfiguration *config) > > > > * Unicam image output format. The ISP input format gets set > at > > > start, > > > > * just in case we have swapped bayer orders due to flips. > > > > */ > > > > - V4L2DeviceFormat unicamFormat = > toV4L2DeviceFormat(sensorFormat); > > > > + V4L2DeviceFormat unicamFormat = > toV4L2DeviceFormat(sensorFormat, > > > packing); > > > > ret = > > > data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); > > > > if (ret) > > > > return ret; > > > > -- > > > > 2.25.1 > > > > > > > >
Hi Naush, Thank you for the patch. On Wed, Oct 27, 2021 at 10:28:00AM +0100, Naushir Patuck wrote: > Default to using CSI2 packed formats when setting up the Unicam image format, > but use an unpacked format if the user requests one through StreamConfiguration. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 48f561d31a31..1b78b5e74a63 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -96,14 +96,14 @@ BayerFormat mbusCodeToBayerFormat(unsigned int mbusCode) > return BayerFormat{}; > } > > -V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format) > +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format, BayerFormat::Packing packing) > { > V4L2DeviceFormat deviceFormat; > BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code); > > ASSERT(bayer.isValid()); > > - bayer.packing = BayerFormat::Packing::CSI2Packed; > + bayer.packing = packing; > deviceFormat.fourcc = bayer.toV4L2PixelFormat(); Packing is handled explicitly, so going through BayerFormat here is fine I think. The other calls to mbusCodeToBayerFormat() in 5/9 however bother me, especially when calling .toPixelFormat() or .toV4L2PixelFormat() immediately after. It would be good to wrap those in helper functions that explicitly handle packing, to ensure that it will be considered everywhere. Have you tested both packed and unpacked formats by the way ? > deviceFormat.size = format.size; > return deviceFormat; > @@ -413,7 +413,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > * the user request. > */ > V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size); > - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat); > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, > + BayerFormat::Packing::CSI2Packed); > int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat); > if (ret) > return Invalid; > @@ -631,6 +632,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > for (auto const stream : data->streams_) > stream->reset(); > > + BayerFormat::Packing packing = BayerFormat::Packing::CSI2Packed; Could we default to packed formats when generating the configuration too ? > Size maxSize, sensorSize; > unsigned int maxIndex = 0; > bool rawStream = false; > @@ -649,6 +651,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > */ > sensorSize = cfg.size; > rawStream = true; > + /* Check if the user has explicitly set an unpacked format. */ > + packing = BayerFormat::fromPixelFormat(cfg.pixelFormat).packing; > } else { > if (cfg.size > maxSize) { > maxSize = config->at(i).size; > @@ -667,7 +671,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > * Unicam image output format. The ISP input format gets set at start, > * just in case we have swapped bayer orders due to flips. > */ > - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat); > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, packing); > ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); > if (ret) > return ret;
Hi Laurent, Thank you for the feedback for this and the other changes. On Wed, 27 Oct 2021 at 15:21, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > Thank you for the patch. > > On Wed, Oct 27, 2021 at 10:28:00AM +0100, Naushir Patuck wrote: > > Default to using CSI2 packed formats when setting up the Unicam image > format, > > but use an unpacked format if the user requests one through > StreamConfiguration. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 48f561d31a31..1b78b5e74a63 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -96,14 +96,14 @@ BayerFormat mbusCodeToBayerFormat(unsigned int > mbusCode) > > return BayerFormat{}; > > } > > > > -V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format) > > +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format, > BayerFormat::Packing packing) > > { > > V4L2DeviceFormat deviceFormat; > > BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code); > > > > ASSERT(bayer.isValid()); > > > > - bayer.packing = BayerFormat::Packing::CSI2Packed; > > + bayer.packing = packing; > > deviceFormat.fourcc = bayer.toV4L2PixelFormat(); > > Packing is handled explicitly, so going through BayerFormat here is fine > I think. The other calls to mbusCodeToBayerFormat() in 5/9 however > bother me, especially when calling .toPixelFormat() or > .toV4L2PixelFormat() immediately after. It would be good to wrap those > in helper functions that explicitly handle packing, to ensure that it > will be considered everywhere. > > Have you tested both packed and unpacked formats by the way ? > Yes, I see your point. As per your comment in the previous change, I will look to change the mbus -> Bayer format conversion to be mbus -> V4L2PixelFormat, and deal with the packing separately in a helper. With the changes in this version of the series Packing/unpacking works as expected, but you are correct in that the change in 5/9 to use .toPixelFormat() in generateConfiguration() will advertise an unpacked format to the application, which is not efficient I will rework that along with the mbus -> V4L2PixelFormat conversion. Thanks, Naush > > > deviceFormat.size = format.size; > > return deviceFormat; > > @@ -413,7 +413,8 @@ CameraConfiguration::Status > RPiCameraConfiguration::validate() > > * the user request. > > */ > > V4L2SubdeviceFormat sensorFormat = > findBestFormat(data_->sensorFormats_, cfg.size); > > - V4L2DeviceFormat unicamFormat = > toV4L2DeviceFormat(sensorFormat); > > + V4L2DeviceFormat unicamFormat = > toV4L2DeviceFormat(sensorFormat, > > + > BayerFormat::Packing::CSI2Packed); > > int ret = > data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat); > > if (ret) > > return Invalid; > > @@ -631,6 +632,7 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > for (auto const stream : data->streams_) > > stream->reset(); > > > > + BayerFormat::Packing packing = BayerFormat::Packing::CSI2Packed; > > Could we default to packed formats when generating the configuration > too ? > > > Size maxSize, sensorSize; > > unsigned int maxIndex = 0; > > bool rawStream = false; > > @@ -649,6 +651,8 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > */ > > sensorSize = cfg.size; > > rawStream = true; > > + /* Check if the user has explicitly set an > unpacked format. */ > > + packing = > BayerFormat::fromPixelFormat(cfg.pixelFormat).packing; > > } else { > > if (cfg.size > maxSize) { > > maxSize = config->at(i).size; > > @@ -667,7 +671,7 @@ int PipelineHandlerRPi::configure(Camera *camera, > CameraConfiguration *config) > > * Unicam image output format. The ISP input format gets set at > start, > > * just in case we have swapped bayer orders due to flips. > > */ > > - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat); > > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, > packing); > > ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); > > if (ret) > > return ret; > > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Wed, Oct 27, 2021 at 03:32:13PM +0100, Naushir Patuck wrote: > On Wed, 27 Oct 2021 at 15:21, Laurent Pinchart wrote: > > On Wed, Oct 27, 2021 at 10:28:00AM +0100, Naushir Patuck wrote: > > > Default to using CSI2 packed formats when setting up the Unicam image format, > > > but use an unpacked format if the user requests one through StreamConfiguration. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 48f561d31a31..1b78b5e74a63 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -96,14 +96,14 @@ BayerFormat mbusCodeToBayerFormat(unsigned int mbusCode) > > > return BayerFormat{}; > > > } > > > > > > -V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format) > > > +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format, BayerFormat::Packing packing) > > > { > > > V4L2DeviceFormat deviceFormat; > > > BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code); > > > > > > ASSERT(bayer.isValid()); > > > > > > - bayer.packing = BayerFormat::Packing::CSI2Packed; > > > + bayer.packing = packing; > > > deviceFormat.fourcc = bayer.toV4L2PixelFormat(); > > > > Packing is handled explicitly, so going through BayerFormat here is fine > > I think. The other calls to mbusCodeToBayerFormat() in 5/9 however > > bother me, especially when calling .toPixelFormat() or > > .toV4L2PixelFormat() immediately after. It would be good to wrap those > > in helper functions that explicitly handle packing, to ensure that it > > will be considered everywhere. > > > Have you tested both packed and unpacked formats by the way ? > > Yes, I see your point. As per your comment in the previous change, I will look to > change the mbus -> Bayer format conversion to be mbus -> V4L2PixelFormat, and > deal with the packing separately in a helper. To be perfecly clear, I don't object on using BayerFormat if it can help, as long as we handle packing explicitly. If a direct conversion ends up being simpler then let's go for that, but there's no strict need to drop BayerFormat usage if BayerFormat results in a cleaner implementation. > With the changes in this version of the series Packing/unpacking works as expected, > but you are correct in that the change in 5/9 to use .toPixelFormat() in generateConfiguration() > will advertise an unpacked format to the application, which is not efficient I will rework that > along with the mbus -> V4L2PixelFormat conversion. > > > > deviceFormat.size = format.size; > > > return deviceFormat; > > > @@ -413,7 +413,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > > > * the user request. > > > */ > > > V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size); > > > - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat); > > > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, > > > + BayerFormat::Packing::CSI2Packed); > > > int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat); > > > if (ret) > > > return Invalid; > > > @@ -631,6 +632,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > > > for (auto const stream : data->streams_) > > > stream->reset(); > > > > > > + BayerFormat::Packing packing = BayerFormat::Packing::CSI2Packed; > > Could we default to packed formats when generating the configuration too ? > > > > > Size maxSize, sensorSize; > > > unsigned int maxIndex = 0; > > > bool rawStream = false; > > > @@ -649,6 +651,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > > > */ > > > sensorSize = cfg.size; > > > rawStream = true; > > > + /* Check if the user has explicitly set an unpacked format. */ > > > + packing = BayerFormat::fromPixelFormat(cfg.pixelFormat).packing; > > > } else { > > > if (cfg.size > maxSize) { > > > maxSize = config->at(i).size; > > > @@ -667,7 +671,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > > > * Unicam image output format. The ISP input format gets set at start, > > > * just in case we have swapped bayer orders due to flips. > > > */ > > > - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat); > > > + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, packing); > > > ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); > > > if (ret) > > > return ret;
Hi Laurent, On Wed, 27 Oct 2021 at 15:59, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Wed, Oct 27, 2021 at 03:32:13PM +0100, Naushir Patuck wrote: > > On Wed, 27 Oct 2021 at 15:21, Laurent Pinchart wrote: > > > On Wed, Oct 27, 2021 at 10:28:00AM +0100, Naushir Patuck wrote: > > > > Default to using CSI2 packed formats when setting up the Unicam > image format, > > > > but use an unpacked format if the user requests one through > StreamConfiguration. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++---- > > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index 48f561d31a31..1b78b5e74a63 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -96,14 +96,14 @@ BayerFormat mbusCodeToBayerFormat(unsigned int > mbusCode) > > > > return BayerFormat{}; > > > > } > > > > > > > > -V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format) > > > > +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format, > BayerFormat::Packing packing) > > > > { > > > > V4L2DeviceFormat deviceFormat; > > > > BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code); > > > > > > > > ASSERT(bayer.isValid()); > > > > > > > > - bayer.packing = BayerFormat::Packing::CSI2Packed; > > > > + bayer.packing = packing; > > > > deviceFormat.fourcc = bayer.toV4L2PixelFormat(); > > > > > > Packing is handled explicitly, so going through BayerFormat here is > fine > > > I think. The other calls to mbusCodeToBayerFormat() in 5/9 however > > > bother me, especially when calling .toPixelFormat() or > > > .toV4L2PixelFormat() immediately after. It would be good to wrap those > > > in helper functions that explicitly handle packing, to ensure that it > > > will be considered everywhere. > > > > > Have you tested both packed and unpacked formats by the way ? > > > > Yes, I see your point. As per your comment in the previous change, I > will look to > > change the mbus -> Bayer format conversion to be mbus -> > V4L2PixelFormat, and > > deal with the packing separately in a helper. > > To be perfecly clear, I don't object on using BayerFormat if it can > help, as long as we handle packing explicitly. If a direct conversion > ends up being simpler then let's go for that, but there's no strict need > to drop BayerFormat usage if BayerFormat results in a cleaner > implementation. > After having prototyped a change with using V4L2DeviceFormat for the mbus conversion, I think it would be better to use BayerFormat in this case. BayerFormat makes things a bit easier to deal with packing/unpacking formats as it is not a separate enum, but just a property of the format. Given that we have no intention of supporting YUV sensor (this will come back to bite me :) in our pipeline handler, using BayerFormat should be ok for now. I will however, remove the table in the pipeline handler, and use the existing table in BayerFormat. In fact, this is what I had for v1 and v2. I'll also fix things so the existing mbusCodeToBayerFormat() will handle switching between packing/unpacking though a helper. Thanks, Naush > > > With the changes in this version of the series Packing/unpacking works > as expected, > > but you are correct in that the change in 5/9 to use .toPixelFormat() in > generateConfiguration() > > will advertise an unpacked format to the application, which is not > efficient I will rework that > > along with the mbus -> V4L2PixelFormat conversion. > > > > > > deviceFormat.size = format.size; > > > > return deviceFormat; > > > > @@ -413,7 +413,8 @@ CameraConfiguration::Status > RPiCameraConfiguration::validate() > > > > * the user request. > > > > */ > > > > V4L2SubdeviceFormat sensorFormat = > findBestFormat(data_->sensorFormats_, cfg.size); > > > > - V4L2DeviceFormat unicamFormat = > toV4L2DeviceFormat(sensorFormat); > > > > + V4L2DeviceFormat unicamFormat = > toV4L2DeviceFormat(sensorFormat, > > > > + BayerFormat::Packing::CSI2Packed); > > > > int ret = > data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat); > > > > if (ret) > > > > return Invalid; > > > > @@ -631,6 +632,7 @@ int PipelineHandlerRPi::configure(Camera > *camera, CameraConfiguration *config) > > > > for (auto const stream : data->streams_) > > > > stream->reset(); > > > > > > > > + BayerFormat::Packing packing = > BayerFormat::Packing::CSI2Packed; > > > Could we default to packed formats when generating the configuration > too ? > > > > > > > Size maxSize, sensorSize; > > > > unsigned int maxIndex = 0; > > > > bool rawStream = false; > > > > @@ -649,6 +651,8 @@ int PipelineHandlerRPi::configure(Camera > *camera, CameraConfiguration *config) > > > > */ > > > > sensorSize = cfg.size; > > > > rawStream = true; > > > > + /* Check if the user has explicitly set an > unpacked format. */ > > > > + packing = > BayerFormat::fromPixelFormat(cfg.pixelFormat).packing; > > > > } else { > > > > if (cfg.size > maxSize) { > > > > maxSize = config->at(i).size; > > > > @@ -667,7 +671,7 @@ int PipelineHandlerRPi::configure(Camera > *camera, CameraConfiguration *config) > > > > * Unicam image output format. The ISP input format gets set > at start, > > > > * just in case we have swapped bayer orders due to flips. > > > > */ > > > > - V4L2DeviceFormat unicamFormat = > toV4L2DeviceFormat(sensorFormat); > > > > + V4L2DeviceFormat unicamFormat = > toV4L2DeviceFormat(sensorFormat, packing); > > > > ret = > data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); > > > > if (ret) > > > > return ret; > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 48f561d31a31..1b78b5e74a63 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -96,14 +96,14 @@ BayerFormat mbusCodeToBayerFormat(unsigned int mbusCode) return BayerFormat{}; } -V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format) +V4L2DeviceFormat toV4L2DeviceFormat(V4L2SubdeviceFormat &format, BayerFormat::Packing packing) { V4L2DeviceFormat deviceFormat; BayerFormat bayer = mbusCodeToBayerFormat(format.mbus_code); ASSERT(bayer.isValid()); - bayer.packing = BayerFormat::Packing::CSI2Packed; + bayer.packing = packing; deviceFormat.fourcc = bayer.toV4L2PixelFormat(); deviceFormat.size = format.size; return deviceFormat; @@ -413,7 +413,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() * the user request. */ V4L2SubdeviceFormat sensorFormat = findBestFormat(data_->sensorFormats_, cfg.size); - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat); + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, + BayerFormat::Packing::CSI2Packed); int ret = data_->unicam_[Unicam::Image].dev()->tryFormat(&unicamFormat); if (ret) return Invalid; @@ -631,6 +632,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) for (auto const stream : data->streams_) stream->reset(); + BayerFormat::Packing packing = BayerFormat::Packing::CSI2Packed; Size maxSize, sensorSize; unsigned int maxIndex = 0; bool rawStream = false; @@ -649,6 +651,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) */ sensorSize = cfg.size; rawStream = true; + /* Check if the user has explicitly set an unpacked format. */ + packing = BayerFormat::fromPixelFormat(cfg.pixelFormat).packing; } else { if (cfg.size > maxSize) { maxSize = config->at(i).size; @@ -667,7 +671,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) * Unicam image output format. The ISP input format gets set at start, * just in case we have swapped bayer orders due to flips. */ - V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat); + V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, packing); ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat); if (ret) return ret;