| Message ID | 20200721092306.823447-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | 1e8c91b65695449c5246d17ba7dc439c8058b781 |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for this, the fix looks good and works for me! David Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Tested-by: David Plowman <david.plowman@raspberrypi.com> On Tue, 21 Jul 2020 at 10:23, Naushir Patuck <naush@raspberrypi.com> wrote: > > The default sensor orientation must be set early on in match() to ensure > generateConfiguration() and configure() return out the correct Bayer > ordering to the application. This is particularly important for RAW > capture streams. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index bf1c7714..e9084afd 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > /* Initialize the camera properties. */ > data->properties_ = data->sensor_->properties(); > > + /* Configure the H/V flip controls based on the sensor rotation. */ > + ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls()); > + int32_t rotation = data->properties_.get(properties::Rotation); > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > + > /* > * List the available output streams. > * Currently cannot do Unicam streams! > @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA() > { V4L2_CID_EXPOSURE, result.data[1] } }); > sensorMetadata_ = result.data[2]; > } > - > - /* Configure the H/V flip controls based on the sensor rotation. */ > - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > - int32_t rotation = sensor_->properties().get(properties::Rotation); > - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > - unicam_[Unicam::Image].dev()->setControls(&ctrls); > } > > if (result.operation & RPI_IPA_CONFIG_SENSOR) { > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Naush, On 21/07/2020 10:23, Naushir Patuck wrote: > The default sensor orientation must be set early on in match() to ensure > generateConfiguration() and configure() return out the correct Bayer > ordering to the application. This is particularly important for RAW > capture streams. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index bf1c7714..e9084afd 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > /* Initialize the camera properties. */ > data->properties_ = data->sensor_->properties(); > > + /* Configure the H/V flip controls based on the sensor rotation. */ > + ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls()); > + int32_t rotation = data->properties_.get(properties::Rotation); > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > + I wonder if this should be done in some CameraData initialisation, but maybe that's just over abstracting and imagining supporting multiple cameras on this pipeline, which /aren't/ supported. So there's no reason for this to be Camera specific at the moment, but I do wonder when we'll see a video-mux hooked up to the RaspberryPi (Too much working with GMSL on my mind) So, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > /* > * List the available output streams. > * Currently cannot do Unicam streams! > @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA() > { V4L2_CID_EXPOSURE, result.data[1] } }); > sensorMetadata_ = result.data[2]; > } > - > - /* Configure the H/V flip controls based on the sensor rotation. */ > - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > - int32_t rotation = sensor_->properties().get(properties::Rotation); > - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > - unicam_[Unicam::Image].dev()->setControls(&ctrls); > } > > if (result.operation & RPI_IPA_CONFIG_SENSOR) { >
Hi Kieran, On Tue, 21 Jul 2020 at 12:35, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Naush, > > On 21/07/2020 10:23, Naushir Patuck wrote: > > The default sensor orientation must be set early on in match() to ensure > > generateConfiguration() and configure() return out the correct Bayer > > ordering to the application. This is particularly important for RAW > > capture streams. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index bf1c7714..e9084afd 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > /* Initialize the camera properties. */ > > data->properties_ = data->sensor_->properties(); > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > + ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls()); > > + int32_t rotation = data->properties_.get(properties::Rotation); > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > > + > > I wonder if this should be done in some CameraData initialisation, but > maybe that's just over abstracting and imagining supporting multiple > cameras on this pipeline, which /aren't/ supported. Yes, that's a good point. It won't make a difference now, but if we were to have multiple cameras, we would have to move this code to another place. Regards, Naush > > So there's no reason for this to be Camera specific at the moment, but I > do wonder when we'll see a video-mux hooked up to the RaspberryPi (Too > much working with GMSL on my mind) > > So, > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > /* > > * List the available output streams. > > * Currently cannot do Unicam streams! > > @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA() > > { V4L2_CID_EXPOSURE, result.data[1] } }); > > sensorMetadata_ = result.data[2]; > > } > > - > > - /* Configure the H/V flip controls based on the sensor rotation. */ > > - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > - int32_t rotation = sensor_->properties().get(properties::Rotation); > > - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > - unicam_[Unicam::Image].dev()->setControls(&ctrls); > > } > > > > if (result.operation & RPI_IPA_CONFIG_SENSOR) { > > > > -- > Regards > -- > Kieran
Hi Naush, On Tue, Jul 21, 2020 at 01:40:09PM +0100, Naushir Patuck wrote: > On Tue, 21 Jul 2020 at 12:35, Kieran Bingham wrote: > > On 21/07/2020 10:23, Naushir Patuck wrote: > > > The default sensor orientation must be set early on in match() to ensure > > > generateConfiguration() and configure() return out the correct Bayer > > > ordering to the application. This is particularly important for RAW > > > capture streams. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++------- > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index bf1c7714..e9084afd 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > > /* Initialize the camera properties. */ > > > data->properties_ = data->sensor_->properties(); > > > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > > + ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls()); > > > + int32_t rotation = data->properties_.get(properties::Rotation); > > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > + > > > > I wonder if this should be done in some CameraData initialisation, but > > maybe that's just over abstracting and imagining supporting multiple > > cameras on this pipeline, which /aren't/ supported. > > Yes, that's a good point. It won't make a difference now, but if we > were to have multiple cameras, we would have to move this code to > another place. If it's easy to do I'd rather have the code moved already. Otherwise, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> On a separate but related topic, when we'll implement support for transformations, the Bayer pattern will be affected. There are different options to deal with that, but before starting that discussion, I'd like to know if it would be feasible on Raspberry Pi platforms to adjust the crop rectangle transparently by one pixel to keep the Bayer pattern stable, or if that's not technically possible or not desired for particular reasons. > > So there's no reason for this to be Camera specific at the moment, but I > > do wonder when we'll see a video-mux hooked up to the RaspberryPi (Too > > much working with GMSL on my mind) > > > > So, > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > /* > > > * List the available output streams. > > > * Currently cannot do Unicam streams! > > > @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA() > > > { V4L2_CID_EXPOSURE, result.data[1] } }); > > > sensorMetadata_ = result.data[2]; > > > } > > > - > > > - /* Configure the H/V flip controls based on the sensor rotation. */ > > > - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > > - int32_t rotation = sensor_->properties().get(properties::Rotation); > > > - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > - unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > } > > > > > > if (result.operation & RPI_IPA_CONFIG_SENSOR) {
Hi Laurent, On Sat, 25 Jul 2020 at 01:20, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > On Tue, Jul 21, 2020 at 01:40:09PM +0100, Naushir Patuck wrote: > > On Tue, 21 Jul 2020 at 12:35, Kieran Bingham wrote: > > > On 21/07/2020 10:23, Naushir Patuck wrote: > > > > The default sensor orientation must be set early on in match() to ensure > > > > generateConfiguration() and configure() return out the correct Bayer > > > > ordering to the application. This is particularly important for RAW > > > > capture streams. > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > --- > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++------- > > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index bf1c7714..e9084afd 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > > > /* Initialize the camera properties. */ > > > > data->properties_ = data->sensor_->properties(); > > > > > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > > > + ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls()); > > > > + int32_t rotation = data->properties_.get(properties::Rotation); > > > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > > + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > > + > > > > > > I wonder if this should be done in some CameraData initialisation, but > > > maybe that's just over abstracting and imagining supporting multiple > > > cameras on this pipeline, which /aren't/ supported. > > > > Yes, that's a good point. It won't make a difference now, but if we > > were to have multiple cameras, we would have to move this code to > > another place. > > If it's easy to do I'd rather have the code moved already. Otherwise, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I could be wrong, but right now, I don't think there is another appropriate hook to place this in for multiple camera cases. Kieran, please do correct me if I am wrong. > > On a separate but related topic, when we'll implement support for > transformations, the Bayer pattern will be affected. There are different > options to deal with that, but before starting that discussion, I'd like > to know if it would be feasible on Raspberry Pi platforms to adjust the > crop rectangle transparently by one pixel to keep the Bayer pattern > stable, or if that's not technically possible or not desired for > particular reasons. The BCM2835 ISP should be able to crop on an odd pixel boundary, effectively modifying the bayer order. However, I have never tried this in anger, so I would fully expect it to have some underlying issues :-) Regards, Naush > > > > So there's no reason for this to be Camera specific at the moment, but I > > > do wonder when we'll see a video-mux hooked up to the RaspberryPi (Too > > > much working with GMSL on my mind) > > > > > > So, > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > /* > > > > * List the available output streams. > > > > * Currently cannot do Unicam streams! > > > > @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA() > > > > { V4L2_CID_EXPOSURE, result.data[1] } }); > > > > sensorMetadata_ = result.data[2]; > > > > } > > > > - > > > > - /* Configure the H/V flip controls based on the sensor rotation. */ > > > > - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > > > - int32_t rotation = sensor_->properties().get(properties::Rotation); > > > > - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > > - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > > - unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > > } > > > > > > > > if (result.operation & RPI_IPA_CONFIG_SENSOR) { > > -- > Regards, > > Laurent Pinchart
Hi everyone On Mon, 27 Jul 2020 at 09:07, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi Laurent, > > On Sat, 25 Jul 2020 at 01:20, Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Naush, > > > > On Tue, Jul 21, 2020 at 01:40:09PM +0100, Naushir Patuck wrote: > > > On Tue, 21 Jul 2020 at 12:35, Kieran Bingham wrote: > > > > On 21/07/2020 10:23, Naushir Patuck wrote: > > > > > The default sensor orientation must be set early on in match() to ensure > > > > > generateConfiguration() and configure() return out the correct Bayer > > > > > ordering to the application. This is particularly important for RAW > > > > > capture streams. > > > > > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > > --- > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++------- > > > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > index bf1c7714..e9084afd 100644 > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > > > > /* Initialize the camera properties. */ > > > > > data->properties_ = data->sensor_->properties(); > > > > > > > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > > > > + ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls()); > > > > > + int32_t rotation = data->properties_.get(properties::Rotation); > > > > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > > > + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > > > + > > > > > > > > I wonder if this should be done in some CameraData initialisation, but > > > > maybe that's just over abstracting and imagining supporting multiple > > > > cameras on this pipeline, which /aren't/ supported. > > > > > > Yes, that's a good point. It won't make a difference now, but if we > > > were to have multiple cameras, we would have to move this code to > > > another place. > > > > If it's easy to do I'd rather have the code moved already. Otherwise, > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I could be wrong, but right now, I don't think there is another > appropriate hook to place this in for multiple camera cases. Kieran, > please do correct me if I am wrong. > > > > > On a separate but related topic, when we'll implement support for > > transformations, the Bayer pattern will be affected. There are different > > options to deal with that, but before starting that discussion, I'd like > > to know if it would be feasible on Raspberry Pi platforms to adjust the > > crop rectangle transparently by one pixel to keep the Bayer pattern > > stable, or if that's not technically possible or not desired for > > particular reasons. > > The BCM2835 ISP should be able to crop on an odd pixel boundary, > effectively modifying the bayer order. However, I have never tried > this in anger, so I would fully expect it to have some underlying > issues :-) For what it's worth, and this is just a matter of personal taste, I'm not keen on the idea of applying teeny 1-pixel crops just to preserve the Bayer pattern. It means you can't rely on being able to read out the full native resolution of the sensor, which seems a bit unfortunate to me. Best regards David > > Regards, > Naush > > > > > > > > So there's no reason for this to be Camera specific at the moment, but I > > > > do wonder when we'll see a video-mux hooked up to the RaspberryPi (Too > > > > much working with GMSL on my mind) > > > > > > > > So, > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > /* > > > > > * List the available output streams. > > > > > * Currently cannot do Unicam streams! > > > > > @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA() > > > > > { V4L2_CID_EXPOSURE, result.data[1] } }); > > > > > sensorMetadata_ = result.data[2]; > > > > > } > > > > > - > > > > > - /* Configure the H/V flip controls based on the sensor rotation. */ > > > > > - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > > > > - int32_t rotation = sensor_->properties().get(properties::Rotation); > > > > > - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > > > - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > > > - unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > > > } > > > > > > > > > > if (result.operation & RPI_IPA_CONFIG_SENSOR) { > > > > -- > > Regards, > > > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi all, Just a gentle ping for the below change to be submitted when you can. Regards, Naush On Tue, 21 Jul 2020 at 10:23, Naushir Patuck <naush@raspberrypi.com> wrote: > > The default sensor orientation must be set early on in match() to ensure > generateConfiguration() and configure() return out the correct Bayer > ordering to the application. This is particularly important for RAW > capture streams. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index bf1c7714..e9084afd 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > /* Initialize the camera properties. */ > data->properties_ = data->sensor_->properties(); > > + /* Configure the H/V flip controls based on the sensor rotation. */ > + ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls()); > + int32_t rotation = data->properties_.get(properties::Rotation); > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > + > /* > * List the available output streams. > * Currently cannot do Unicam streams! > @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA() > { V4L2_CID_EXPOSURE, result.data[1] } }); > sensorMetadata_ = result.data[2]; > } > - > - /* Configure the H/V flip controls based on the sensor rotation. */ > - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > - int32_t rotation = sensor_->properties().get(properties::Rotation); > - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > - unicam_[Unicam::Image].dev()->setControls(&ctrls); > } > > if (result.operation & RPI_IPA_CONFIG_SENSOR) { > -- > 2.25.1 >
Hi Naush, On Fri, Aug 07, 2020 at 01:12:01PM +0100, Naushir Patuck wrote: > Hi all, > > Just a gentle ping for the below change to be submitted when you can. I know I have a fairly large backlog of RPi libcamera patches to review, I apologize about that, even more so as I will have very little time this week. I'll do my best and try to address as much of the backlog as possible on Thursday. > On Tue, 21 Jul 2020 at 10:23, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > The default sensor orientation must be set early on in match() to ensure > > generateConfiguration() and configure() return out the correct Bayer > > ordering to the application. This is particularly important for RAW > > capture streams. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index bf1c7714..e9084afd 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > /* Initialize the camera properties. */ > > data->properties_ = data->sensor_->properties(); > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > + ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls()); > > + int32_t rotation = data->properties_.get(properties::Rotation); > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > > + > > /* > > * List the available output streams. > > * Currently cannot do Unicam streams! > > @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA() > > { V4L2_CID_EXPOSURE, result.data[1] } }); > > sensorMetadata_ = result.data[2]; > > } > > - > > - /* Configure the H/V flip controls based on the sensor rotation. */ > > - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > - int32_t rotation = sensor_->properties().get(properties::Rotation); > > - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > - unicam_[Unicam::Image].dev()->setControls(&ctrls); > > } > > > > if (result.operation & RPI_IPA_CONFIG_SENSOR) {
Hi Laurent, On Mon, 10 Aug 2020 at 06:38, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > On Fri, Aug 07, 2020 at 01:12:01PM +0100, Naushir Patuck wrote: > > Hi all, > > > > Just a gentle ping for the below change to be submitted when you can. > > I know I have a fairly large backlog of RPi libcamera patches to review, > I apologize about that, even more so as I will have very little time > this week. I'll do my best and try to address as much of the backlog as > possible on Thursday. No problem at all :) Regards, Naush > > > On Tue, 21 Jul 2020 at 10:23, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > The default sensor orientation must be set early on in match() to ensure > > > generateConfiguration() and configure() return out the correct Bayer > > > ordering to the application. This is particularly important for RAW > > > capture streams. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++------- > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index bf1c7714..e9084afd 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > > /* Initialize the camera properties. */ > > > data->properties_ = data->sensor_->properties(); > > > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > > + ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls()); > > > + int32_t rotation = data->properties_.get(properties::Rotation); > > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > + > > > /* > > > * List the available output streams. > > > * Currently cannot do Unicam streams! > > > @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA() > > > { V4L2_CID_EXPOSURE, result.data[1] } }); > > > sensorMetadata_ = result.data[2]; > > > } > > > - > > > - /* Configure the H/V flip controls based on the sensor rotation. */ > > > - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > > - int32_t rotation = sensor_->properties().get(properties::Rotation); > > > - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > > - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > > - unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > } > > > > > > if (result.operation & RPI_IPA_CONFIG_SENSOR) { > > -- > Regards, > > Laurent Pinchart
Hi all, Another gentle ping for a review and submission of this patch please. Thanks, Naush On Fri, 7 Aug 2020 at 13:12, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi all, > > Just a gentle ping for the below change to be submitted when you can. > > Regards, > Naush > > On Tue, 21 Jul 2020 at 10:23, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > The default sensor orientation must be set early on in match() to ensure > > generateConfiguration() and configure() return out the correct Bayer > > ordering to the application. This is particularly important for RAW > > capture streams. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index bf1c7714..e9084afd 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > > /* Initialize the camera properties. */ > > data->properties_ = data->sensor_->properties(); > > > > + /* Configure the H/V flip controls based on the sensor rotation. */ > > + ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls()); > > + int32_t rotation = data->properties_.get(properties::Rotation); > > + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > > + > > /* > > * List the available output streams. > > * Currently cannot do Unicam streams! > > @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA() > > { V4L2_CID_EXPOSURE, result.data[1] } }); > > sensorMetadata_ = result.data[2]; > > } > > - > > - /* Configure the H/V flip controls based on the sensor rotation. */ > > - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > > - int32_t rotation = sensor_->properties().get(properties::Rotation); > > - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > > - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > > - unicam_[Unicam::Image].dev()->setControls(&ctrls); > > } > > > > if (result.operation & RPI_IPA_CONFIG_SENSOR) { > > -- > > 2.25.1 > >
Hi Naush, On 27/08/2020 09:30, Naushir Patuck wrote: > Hi all, > > Another gentle ping for a review and submission of this patch please. This has a tag from both me and Laurent, so I don't see any reason it can't already be in. We'll have to look at how to optimise the integration paths for contributions, as we of course just push our own patches after review. I wonder if it would help to make a pull-request (git-request-pull) when a series is ready. I don't want to add extra burden to you though, and I can't see any reason not to apply this now. Pushing ... I'll try to work through the status of the other raw patches now. -- Kieran > Thanks, > Naush > > On Fri, 7 Aug 2020 at 13:12, Naushir Patuck <naush@raspberrypi.com> wrote: >> >> Hi all, >> >> Just a gentle ping for the below change to be submitted when you can. >> >> Regards, >> Naush >> >> On Tue, 21 Jul 2020 at 10:23, Naushir Patuck <naush@raspberrypi.com> wrote: >>> >>> The default sensor orientation must be set early on in match() to ensure >>> generateConfiguration() and configure() return out the correct Bayer >>> ordering to the application. This is particularly important for RAW >>> capture streams. >>> >>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >>> --- >>> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++------- >>> 1 file changed, 7 insertions(+), 7 deletions(-) >>> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>> index bf1c7714..e9084afd 100644 >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>> @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) >>> /* Initialize the camera properties. */ >>> data->properties_ = data->sensor_->properties(); >>> >>> + /* Configure the H/V flip controls based on the sensor rotation. */ >>> + ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls()); >>> + int32_t rotation = data->properties_.get(properties::Rotation); >>> + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); >>> + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); >>> + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); >>> + >>> /* >>> * List the available output streams. >>> * Currently cannot do Unicam streams! >>> @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA() >>> { V4L2_CID_EXPOSURE, result.data[1] } }); >>> sensorMetadata_ = result.data[2]; >>> } >>> - >>> - /* Configure the H/V flip controls based on the sensor rotation. */ >>> - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); >>> - int32_t rotation = sensor_->properties().get(properties::Rotation); >>> - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); >>> - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); >>> - unicam_[Unicam::Image].dev()->setControls(&ctrls); >>> } >>> >>> if (result.operation & RPI_IPA_CONFIG_SENSOR) { >>> -- >>> 2.25.1 >>> > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Kieran, On Thu, 27 Aug 2020 at 10:57, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Naush, > > On 27/08/2020 09:30, Naushir Patuck wrote: > > Hi all, > > > > Another gentle ping for a review and submission of this patch please. > > This has a tag from both me and Laurent, so I don't see any reason it > can't already be in. Yes, this can go in as-is. Thanks! > > We'll have to look at how to optimise the integration paths for > contributions, as we of course just push our own patches after review. > > I wonder if it would help to make a pull-request (git-request-pull) when > a series is ready. > > I don't want to add extra burden to you though, and I can't see any > reason not to apply this now. > I do not have a problem doing an extra git-request-pull when a patch is ready if that helps the workflow for you and the team. I'm sure other external contributors would not have a problem either :) Let me know if this is the preferred way going forward. Regards, Naush > Pushing ... > > > I'll try to work through the status of the other raw patches now. > -- > Kieran > > > > Thanks, > > Naush > > > > On Fri, 7 Aug 2020 at 13:12, Naushir Patuck <naush@raspberrypi.com> wrote: > >> > >> Hi all, > >> > >> Just a gentle ping for the below change to be submitted when you can. > >> > >> Regards, > >> Naush > >> > >> On Tue, 21 Jul 2020 at 10:23, Naushir Patuck <naush@raspberrypi.com> wrote: > >>> > >>> The default sensor orientation must be set early on in match() to ensure > >>> generateConfiguration() and configure() return out the correct Bayer > >>> ordering to the application. This is particularly important for RAW > >>> capture streams. > >>> > >>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > >>> --- > >>> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++------- > >>> 1 file changed, 7 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> index bf1c7714..e9084afd 100644 > >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) > >>> /* Initialize the camera properties. */ > >>> data->properties_ = data->sensor_->properties(); > >>> > >>> + /* Configure the H/V flip controls based on the sensor rotation. */ > >>> + ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls()); > >>> + int32_t rotation = data->properties_.get(properties::Rotation); > >>> + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > >>> + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > >>> + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > >>> + > >>> /* > >>> * List the available output streams. > >>> * Currently cannot do Unicam streams! > >>> @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA() > >>> { V4L2_CID_EXPOSURE, result.data[1] } }); > >>> sensorMetadata_ = result.data[2]; > >>> } > >>> - > >>> - /* Configure the H/V flip controls based on the sensor rotation. */ > >>> - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); > >>> - int32_t rotation = sensor_->properties().get(properties::Rotation); > >>> - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); > >>> - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); > >>> - unicam_[Unicam::Image].dev()->setControls(&ctrls); > >>> } > >>> > >>> if (result.operation & RPI_IPA_CONFIG_SENSOR) { > >>> -- > >>> 2.25.1 > >>> > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards > -- > Kieran
Hi Naush, On 27/08/2020 11:17, Naushir Patuck wrote: > Hi Kieran, > > > On Thu, 27 Aug 2020 at 10:57, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: >> >> Hi Naush, >> >> On 27/08/2020 09:30, Naushir Patuck wrote: >>> Hi all, >>> >>> Another gentle ping for a review and submission of this patch please. >> >> This has a tag from both me and Laurent, so I don't see any reason it >> can't already be in. > > Yes, this can go in as-is. Thanks! > >> >> We'll have to look at how to optimise the integration paths for >> contributions, as we of course just push our own patches after review. >> >> I wonder if it would help to make a pull-request (git-request-pull) when >> a series is ready. >> >> I don't want to add extra burden to you though, and I can't see any >> reason not to apply this now. >> > > I do not have a problem doing an extra git-request-pull when a patch > is ready if that helps the workflow for you and the team. I'm sure > other external contributors would not have a problem either :) Let me > know if this is the preferred way going forward. For this one, I think it wasn't obvious at a glance it was ready to go in, so it got missed (it's in now). We have a patchwork instance running, but it still needs some more refinement to hook up the mail server correctly. I've also got a bot running against that instance to automate tasks - so I think perhaps that a good addition would be to have mail reminders from the bot if it sees a patch with sufficient RB tags, which is not yet integrated. -- Kieran > Regards, > Naush > >> Pushing ... >> >> >> I'll try to work through the status of the other raw patches now. >> -- >> Kieran >> >> >>> Thanks, >>> Naush >>> >>> On Fri, 7 Aug 2020 at 13:12, Naushir Patuck <naush@raspberrypi.com> wrote: >>>> >>>> Hi all, >>>> >>>> Just a gentle ping for the below change to be submitted when you can. >>>> >>>> Regards, >>>> Naush >>>> >>>> On Tue, 21 Jul 2020 at 10:23, Naushir Patuck <naush@raspberrypi.com> wrote: >>>>> >>>>> The default sensor orientation must be set early on in match() to ensure >>>>> generateConfiguration() and configure() return out the correct Bayer >>>>> ordering to the application. This is particularly important for RAW >>>>> capture streams. >>>>> >>>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >>>>> --- >>>>> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++------- >>>>> 1 file changed, 7 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>>>> index bf1c7714..e9084afd 100644 >>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>>>> @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) >>>>> /* Initialize the camera properties. */ >>>>> data->properties_ = data->sensor_->properties(); >>>>> >>>>> + /* Configure the H/V flip controls based on the sensor rotation. */ >>>>> + ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls()); >>>>> + int32_t rotation = data->properties_.get(properties::Rotation); >>>>> + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); >>>>> + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); >>>>> + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); >>>>> + >>>>> /* >>>>> * List the available output streams. >>>>> * Currently cannot do Unicam streams! >>>>> @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA() >>>>> { V4L2_CID_EXPOSURE, result.data[1] } }); >>>>> sensorMetadata_ = result.data[2]; >>>>> } >>>>> - >>>>> - /* Configure the H/V flip controls based on the sensor rotation. */ >>>>> - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); >>>>> - int32_t rotation = sensor_->properties().get(properties::Rotation); >>>>> - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); >>>>> - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); >>>>> - unicam_[Unicam::Image].dev()->setControls(&ctrls); >>>>> } >>>>> >>>>> if (result.operation & RPI_IPA_CONFIG_SENSOR) { >>>>> -- >>>>> 2.25.1 >>>>> >>> _______________________________________________ >>> libcamera-devel mailing list >>> libcamera-devel@lists.libcamera.org >>> https://lists.libcamera.org/listinfo/libcamera-devel >>> >> >> -- >> Regards >> -- >> Kieran
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index bf1c7714..e9084afd 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -962,6 +962,13 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) /* Initialize the camera properties. */ data->properties_ = data->sensor_->properties(); + /* Configure the H/V flip controls based on the sensor rotation. */ + ControlList ctrls(data->unicam_[Unicam::Image].dev()->controls()); + int32_t rotation = data->properties_.get(properties::Rotation); + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); + /* * List the available output streams. * Currently cannot do Unicam streams! @@ -1165,13 +1172,6 @@ int RPiCameraData::configureIPA() { V4L2_CID_EXPOSURE, result.data[1] } }); sensorMetadata_ = result.data[2]; } - - /* Configure the H/V flip controls based on the sensor rotation. */ - ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); - int32_t rotation = sensor_->properties().get(properties::Rotation); - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation)); - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation)); - unicam_[Unicam::Image].dev()->setControls(&ctrls); } if (result.operation & RPI_IPA_CONFIG_SENSOR) {
The default sensor orientation must be set early on in match() to ensure generateConfiguration() and configure() return out the correct Bayer ordering to the application. This is particularly important for RAW capture streams. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)