| Message ID | 20200828144110.17303-3-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, On 28/08/2020 15:41, David Plowman wrote: > We set the sensor orientation (h and v flips) during validate as this > will in general affect the Bayer order output by the sensor. Doing it > here means that the correct raw format gets advertised in any raw > streams that the application requested. Eeep - I'm not sure if we could do this in validate(). Validation should not actually make any change to the hardware, but it should check that the configuration can be applied correctly, and make any changes that would be necessary to support a correct (and 'valid') configuration to be applied through the ->configure() > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 42c9caa..7aace71 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -400,6 +400,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > if (config_.empty()) > return Invalid; > > + /* > + * Configure the H/V flip controls based on the sensor rotation. We do > + * this here so that the sensor has the correct Bayer format that will > + * get advertised in the configuration of any raw streams. > + */ > + ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); > + int32_t rotation = data_->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)); > + data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); > + > unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0; > std::pair<int, Size> outSize[2]; > Size maxSize; > @@ -1164,13 +1175,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 Fri, 28 Aug 2020 at 16:02, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi David, > > On 28/08/2020 15:41, David Plowman wrote: > > We set the sensor orientation (h and v flips) during validate as this > > will in general affect the Bayer order output by the sensor. Doing it > > here means that the correct raw format gets advertised in any raw > > streams that the application requested. > > Eeep - I'm not sure if we could do this in validate(). > > Validation should not actually make any change to the hardware, but it > should check that the configuration can be applied correctly, and make > any changes that would be necessary to support a correct (and 'valid') > configuration to be applied through the ->configure() > Yes, that's an interesting one. The reason for doing it here is so that the Bayer format comes out correctly for any raw streams that were requested, and we're relying on the camera driver to give us the true Bayer order. Of course, the camera driver doesn't *have* to change the Bayer order when you transform it (it might do 1-pixel shits to maintain the original Bayer order), so the puzzle then is... how would you know? Another solution I toyed with - and indeed implemented first - was to do it in the configure() method, but then I had to dig around and find any raw stream configurations and update the pixelFormat post facto. This involves changing stream formats after validate(), which seemed bad to me too... but do we prefer it? David > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 18 +++++++++++------- > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 42c9caa..7aace71 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -400,6 +400,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > > if (config_.empty()) > > return Invalid; > > > > + /* > > + * Configure the H/V flip controls based on the sensor rotation. We do > > + * this here so that the sensor has the correct Bayer format that will > > + * get advertised in the configuration of any raw streams. > > + */ > > + ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); > > + int32_t rotation = data_->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)); > > + data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); > > + > > unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0; > > std::pair<int, Size> outSize[2]; > > Size maxSize; > > @@ -1164,13 +1175,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
Oops. On Fri, 28 Aug 2020 at 16:14, David Plowman <david.plowman@raspberrypi.com> wrote: > > Hi Kieran > > On Fri, 28 Aug 2020 at 16:02, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Hi David, > > > > On 28/08/2020 15:41, David Plowman wrote: > > > We set the sensor orientation (h and v flips) during validate as this > > > will in general affect the Bayer order output by the sensor. Doing it > > > here means that the correct raw format gets advertised in any raw > > > streams that the application requested. > > > > Eeep - I'm not sure if we could do this in validate(). > > > > Validation should not actually make any change to the hardware, but it > > should check that the configuration can be applied correctly, and make > > any changes that would be necessary to support a correct (and 'valid') > > configuration to be applied through the ->configure() > > > > Yes, that's an interesting one. The reason for doing it here is so > that the Bayer format comes out correctly for any raw streams that > were requested, and we're relying on the camera driver to give us the > true Bayer order. > > Of course, the camera driver doesn't *have* to change the Bayer order > when you transform it (it might do 1-pixel shits to maintain the shifts. oh dear... :( > original Bayer order), so the puzzle then is... how would you know? > > Another solution I toyed with - and indeed implemented first - was to > do it in the configure() method, but then I had to dig around and find > any raw stream configurations and update the pixelFormat post facto. > This involves changing stream formats after validate(), which seemed > bad to me too... but do we prefer it? > > David > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 18 +++++++++++------- > > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 42c9caa..7aace71 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -400,6 +400,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > > > if (config_.empty()) > > > return Invalid; > > > > > > + /* > > > + * Configure the H/V flip controls based on the sensor rotation. We do > > > + * this here so that the sensor has the correct Bayer format that will > > > + * get advertised in the configuration of any raw streams. > > > + */ > > > + ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); > > > + int32_t rotation = data_->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)); > > > + data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > + > > > unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0; > > > std::pair<int, Size> outSize[2]; > > > Size maxSize; > > > @@ -1164,13 +1175,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 David, On 28/08/2020 16:14, David Plowman wrote: > Hi Kieran > > On Fri, 28 Aug 2020 at 16:02, Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: >> >> Hi David, >> >> On 28/08/2020 15:41, David Plowman wrote: >>> We set the sensor orientation (h and v flips) during validate as this >>> will in general affect the Bayer order output by the sensor. Doing it >>> here means that the correct raw format gets advertised in any raw >>> streams that the application requested. >> >> Eeep - I'm not sure if we could do this in validate(). >> >> Validation should not actually make any change to the hardware, but it >> should check that the configuration can be applied correctly, and make >> any changes that would be necessary to support a correct (and 'valid') >> configuration to be applied through the ->configure() >> > > Yes, that's an interesting one. The reason for doing it here is so > that the Bayer format comes out correctly for any raw streams that > were requested, and we're relying on the camera driver to give us the > true Bayer order. > > Of course, the camera driver doesn't *have* to change the Bayer order > when you transform it (it might do 1-pixel shits to maintain the haha, I hadn't noticed the typo until your follow up post ;-) > original Bayer order), so the puzzle then is... how would you know? > > Another solution I toyed with - and indeed implemented first - was to > do it in the configure() method, but then I had to dig around and find > any raw stream configurations and update the pixelFormat post facto. > This involves changing stream formats after validate(), which seemed > bad to me too... but do we prefer it? Ayeee - no, we can't change a format after validate either. Oh dear have we deadlocked... So - is the issue that the rotation/transform affects the pixel format? Are we intending to support those transforms on a per-frame basis? or just a per-stream basis. As long as it's per-stream - Can we just store the state of the expected rotation in the RPiCameraConfiguration, so that we can use it at configure time appropriately? Or is it that we will need to determine what adjustment will be made to the pixelformat based on the transform. And if that's the case, I assume it's the kernel driver which is going to tell us what the new format will be. I fear we might have to duplicate the determination of the format up here, and report the adjustment. I guess what we're lacking is the ability to do a full atomic 'try' of the state of the device at the driver level ... -- Kieran > > David > >> >>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> >>> --- >>> .../pipeline/raspberrypi/raspberrypi.cpp | 18 +++++++++++------- >>> 1 file changed, 11 insertions(+), 7 deletions(-) >>> >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>> index 42c9caa..7aace71 100644 >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp >>> @@ -400,6 +400,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() >>> if (config_.empty()) >>> return Invalid; >>> >>> + /* >>> + * Configure the H/V flip controls based on the sensor rotation. We do >>> + * this here so that the sensor has the correct Bayer format that will >>> + * get advertised in the configuration of any raw streams. >>> + */ >>> + ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); >>> + int32_t rotation = data_->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)); >>> + data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); >>> + >>> unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0; >>> std::pair<int, Size> outSize[2]; >>> Size maxSize; >>> @@ -1164,13 +1175,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
Hello, On Fri, Aug 28, 2020 at 04:26:05PM +0100, Kieran Bingham wrote: > On 28/08/2020 16:14, David Plowman wrote: > > On Fri, 28 Aug 2020 at 16:02, Kieran Bingham wrote: > >> On 28/08/2020 15:41, David Plowman wrote: > >>> We set the sensor orientation (h and v flips) during validate as this > >>> will in general affect the Bayer order output by the sensor. Doing it > >>> here means that the correct raw format gets advertised in any raw > >>> streams that the application requested. > >> > >> Eeep - I'm not sure if we could do this in validate(). > >> > >> Validation should not actually make any change to the hardware, but it > >> should check that the configuration can be applied correctly, and make > >> any changes that would be necessary to support a correct (and 'valid') > >> configuration to be applied through the ->configure() > > > > Yes, that's an interesting one. The reason for doing it here is so > > that the Bayer format comes out correctly for any raw streams that > > were requested, and we're relying on the camera driver to give us the > > true Bayer order. > > > > Of course, the camera driver doesn't *have* to change the Bayer order > > when you transform it (it might do 1-pixel shits to maintain the > > haha, I hadn't noticed the typo until your follow up post ;-) > > > original Bayer order), so the puzzle then is... how would you know? > > > > Another solution I toyed with - and indeed implemented first - was to > > do it in the configure() method, but then I had to dig around and find > > any raw stream configurations and update the pixelFormat post facto. > > This involves changing stream formats after validate(), which seemed > > bad to me too... but do we prefer it? > > Ayeee - no, we can't change a format after validate either. > Oh dear have we deadlocked... > > So - is the issue that the rotation/transform affects the pixel format? > > Are we intending to support those transforms on a per-frame basis? or > just a per-stream basis. > > As long as it's per-stream - Can we just store the state of the expected > rotation in the RPiCameraConfiguration, so that we can use it at > configure time appropriately? Or is it that we will need to determine > what adjustment will be made to the pixelformat based on the transform. The latter. We need to report to the user which raw format will be used based on the transformation. > And if that's the case, I assume it's the kernel driver which is going > to tell us what the new format will be. I fear we might have to > duplicate the determination of the format up here, and report the > adjustment. > > I guess what we're lacking is the ability to do a full atomic 'try' of > the state of the device at the driver level ... Correct, and I doubt V4L2 will ever give us that. I think we need to adjust the format in the pipeline handler, instead of querying the device. We however need to know if h/v flip will affect the bayer pattern, or if the sensor will compensate by shifting the crop rectangle. One option is to query the sensor driver at match time, to see if the media bus format changes when the h/v flip controls are modified. A bit of a hassle, but shouldn't be too complicated. Another option would be to ignore all this and consider that a proper sensor driver will never shift the crop rectangle behind the scene. We have the option to establish a set of rules for a sensor driver to be supported by libcamera. > >>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > >>> --- > >>> .../pipeline/raspberrypi/raspberrypi.cpp | 18 +++++++++++------- > >>> 1 file changed, 11 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> index 42c9caa..7aace71 100644 > >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> @@ -400,6 +400,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > >>> if (config_.empty()) > >>> return Invalid; > >>> > >>> + /* > >>> + * Configure the H/V flip controls based on the sensor rotation. We do > >>> + * this here so that the sensor has the correct Bayer format that will > >>> + * get advertised in the configuration of any raw streams. > >>> + */ > >>> + ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); > >>> + int32_t rotation = data_->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)); > >>> + data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); > >>> + > >>> unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0; > >>> std::pair<int, Size> outSize[2]; > >>> Size maxSize; > >>> @@ -1164,13 +1175,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 Fri, 28 Aug 2020 at 16:26, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi David, > > On 28/08/2020 16:14, David Plowman wrote: > > Hi Kieran > > > > On Fri, 28 Aug 2020 at 16:02, Kieran Bingham > > <kieran.bingham@ideasonboard.com> wrote: > >> > >> Hi David, > >> > >> On 28/08/2020 15:41, David Plowman wrote: > >>> We set the sensor orientation (h and v flips) during validate as this > >>> will in general affect the Bayer order output by the sensor. Doing it > >>> here means that the correct raw format gets advertised in any raw > >>> streams that the application requested. > >> > >> Eeep - I'm not sure if we could do this in validate(). > >> > >> Validation should not actually make any change to the hardware, but it > >> should check that the configuration can be applied correctly, and make > >> any changes that would be necessary to support a correct (and 'valid') > >> configuration to be applied through the ->configure() > >> > > > > Yes, that's an interesting one. The reason for doing it here is so > > that the Bayer format comes out correctly for any raw streams that > > were requested, and we're relying on the camera driver to give us the > > true Bayer order. > > > > Of course, the camera driver doesn't *have* to change the Bayer order > > when you transform it (it might do 1-pixel shits to maintain the > > haha, I hadn't noticed the typo until your follow up post ;-) > > > > original Bayer order), so the puzzle then is... how would you know? > > > > Another solution I toyed with - and indeed implemented first - was to > > do it in the configure() method, but then I had to dig around and find > > any raw stream configurations and update the pixelFormat post facto. > > This involves changing stream formats after validate(), which seemed > > bad to me too... but do we prefer it? > > Ayeee - no, we can't change a format after validate either. > Oh dear have we deadlocked... > > > So - is the issue that the rotation/transform affects the pixel format? Correct. If you have an RGGB sensor and ask it to do h and v flips you end up with BGGR. > > Are we intending to support those transforms on a per-frame basis? or > just a per-stream basis. Per-stream. Actually, they're per-camera because that's where the transformations are being applied. There's no possibility to change any of this per frame. > > As long as it's per-stream - Can we just store the state of the expected > rotation in the RPiCameraConfiguration, so that we can use it at > configure time appropriately? Or is it that we will need to determine > what adjustment will be made to the pixelformat based on the transform. > > And if that's the case, I assume it's the kernel driver which is going > to tell us what the new format will be. I fear we might have to > duplicate the determination of the format up here, and report the > adjustment. I think the problem with this is that we don't know what the camera will do when we ask for a transform. I think all the drivers we've ever been involved with will change the Bayer order, but others might not. They might fiddle with some 1-pixel wide cropping to keep the Bayer order the same. > > I guess what we're lacking is the ability to do a full atomic 'try' of > the state of the device at the driver level ... Yes, I think this is the crux of the matter. David > > -- > Kieran > > > > > > > David > > > >> > >>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > >>> --- > >>> .../pipeline/raspberrypi/raspberrypi.cpp | 18 +++++++++++------- > >>> 1 file changed, 11 insertions(+), 7 deletions(-) > >>> > >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> index 42c9caa..7aace71 100644 > >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>> @@ -400,6 +400,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > >>> if (config_.empty()) > >>> return Invalid; > >>> > >>> + /* > >>> + * Configure the H/V flip controls based on the sensor rotation. We do > >>> + * this here so that the sensor has the correct Bayer format that will > >>> + * get advertised in the configuration of any raw streams. > >>> + */ > >>> + ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); > >>> + int32_t rotation = data_->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)); > >>> + data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); > >>> + > >>> unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0; > >>> std::pair<int, Size> outSize[2]; > >>> Size maxSize; > >>> @@ -1164,13 +1175,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 > > -- > Regards > -- > Kieran
Hi Guys On Fri, 28 Aug 2020 at 16:36, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > On Fri, Aug 28, 2020 at 04:26:05PM +0100, Kieran Bingham wrote: > > On 28/08/2020 16:14, David Plowman wrote: > > > On Fri, 28 Aug 2020 at 16:02, Kieran Bingham wrote: > > >> On 28/08/2020 15:41, David Plowman wrote: > > >>> We set the sensor orientation (h and v flips) during validate as this > > >>> will in general affect the Bayer order output by the sensor. Doing it > > >>> here means that the correct raw format gets advertised in any raw > > >>> streams that the application requested. > > >> > > >> Eeep - I'm not sure if we could do this in validate(). > > >> > > >> Validation should not actually make any change to the hardware, but it > > >> should check that the configuration can be applied correctly, and make > > >> any changes that would be necessary to support a correct (and 'valid') > > >> configuration to be applied through the ->configure() > > > > > > Yes, that's an interesting one. The reason for doing it here is so > > > that the Bayer format comes out correctly for any raw streams that > > > were requested, and we're relying on the camera driver to give us the > > > true Bayer order. > > > > > > Of course, the camera driver doesn't *have* to change the Bayer order > > > when you transform it (it might do 1-pixel shits to maintain the > > > > haha, I hadn't noticed the typo until your follow up post ;-) > > > > > original Bayer order), so the puzzle then is... how would you know? > > > > > > Another solution I toyed with - and indeed implemented first - was to > > > do it in the configure() method, but then I had to dig around and find > > > any raw stream configurations and update the pixelFormat post facto. > > > This involves changing stream formats after validate(), which seemed > > > bad to me too... but do we prefer it? > > > > Ayeee - no, we can't change a format after validate either. > > Oh dear have we deadlocked... > > > > So - is the issue that the rotation/transform affects the pixel format? > > > > Are we intending to support those transforms on a per-frame basis? or > > just a per-stream basis. > > > > As long as it's per-stream - Can we just store the state of the expected > > rotation in the RPiCameraConfiguration, so that we can use it at > > configure time appropriately? Or is it that we will need to determine > > what adjustment will be made to the pixelformat based on the transform. > > The latter. We need to report to the user which raw format will be used > based on the transformation. > > > And if that's the case, I assume it's the kernel driver which is going > > to tell us what the new format will be. I fear we might have to > > duplicate the determination of the format up here, and report the > > adjustment. > > > > I guess what we're lacking is the ability to do a full atomic 'try' of > > the state of the device at the driver level ... > > Correct, and I doubt V4L2 will ever give us that. > > I think we need to adjust the format in the pipeline handler, instead of > querying the device. We however need to know if h/v flip will affect the > bayer pattern, or if the sensor will compensate by shifting the crop > rectangle. > > One option is to query the sensor driver at match time, to see if the > media bus format changes when the h/v flip controls are modified. A bit > of a hassle, but shouldn't be too complicated. Another option would be > to ignore all this and consider that a proper sensor driver will never > shift the crop rectangle behind the scene. We have the option to > establish a set of rules for a sensor driver to be supported by > libcamera. You have a very good indicator of that from the V4L2_CTRL_FLAG_MODIFY_LAYOUT flag when querying controls. It would be fair to deduce from that flag on flips that it is going to change the bayer order. I've yet to see a Bayer sensor that offers transpose, but there may be some out there. IMX327 and IMX290 are two that don't alter the Bayer order when using the on-sensor flip controls. Reading the datasheet for IMX327 (page 58), it explicitly states for the full frame mode the active lines are "Line No during normal operation" 21 to 1100 "Line No during inverted operation" 1101 to 22 and "Horizontal pixel output image normal operation" 13 to1932 "Horizontal pixel output image inverted operation" 1933 to 14. I'm also aware of one further sensor (can't say what) that I know for definite doesn't change the Bayer order. Exactly how it achieves that one can only assume is by shifting the readout by one pixel. Should the crop reported by G_SELECTION be shifted in response to the flips then? Dave > > >>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > >>> --- > > >>> .../pipeline/raspberrypi/raspberrypi.cpp | 18 +++++++++++------- > > >>> 1 file changed, 11 insertions(+), 7 deletions(-) > > >>> > > >>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > >>> index 42c9caa..7aace71 100644 > > >>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > >>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > >>> @@ -400,6 +400,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > > >>> if (config_.empty()) > > >>> return Invalid; > > >>> > > >>> + /* > > >>> + * Configure the H/V flip controls based on the sensor rotation. We do > > >>> + * this here so that the sensor has the correct Bayer format that will > > >>> + * get advertised in the configuration of any raw streams. > > >>> + */ > > >>> + ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); > > >>> + int32_t rotation = data_->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)); > > >>> + data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); > > >>> + > > >>> unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0; > > >>> std::pair<int, Size> outSize[2]; > > >>> Size maxSize; > > >>> @@ -1164,13 +1175,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 Dave, On Fri, Aug 28, 2020 at 04:55:14PM +0100, Dave Stevenson wrote: > On Fri, 28 Aug 2020 at 16:36, Laurent Pinchart wrote: > > On Fri, Aug 28, 2020 at 04:26:05PM +0100, Kieran Bingham wrote: > >> On 28/08/2020 16:14, David Plowman wrote: > >>> On Fri, 28 Aug 2020 at 16:02, Kieran Bingham wrote: > >>>> On 28/08/2020 15:41, David Plowman wrote: > >>>>> We set the sensor orientation (h and v flips) during validate as this > >>>>> will in general affect the Bayer order output by the sensor. Doing it > >>>>> here means that the correct raw format gets advertised in any raw > >>>>> streams that the application requested. > >>>> > >>>> Eeep - I'm not sure if we could do this in validate(). > >>>> > >>>> Validation should not actually make any change to the hardware, but it > >>>> should check that the configuration can be applied correctly, and make > >>>> any changes that would be necessary to support a correct (and 'valid') > >>>> configuration to be applied through the ->configure() > >>> > >>> Yes, that's an interesting one. The reason for doing it here is so > >>> that the Bayer format comes out correctly for any raw streams that > >>> were requested, and we're relying on the camera driver to give us the > >>> true Bayer order. > >>> > >>> Of course, the camera driver doesn't *have* to change the Bayer order > >>> when you transform it (it might do 1-pixel shits to maintain the > >> > >> haha, I hadn't noticed the typo until your follow up post ;-) > >> > >>> original Bayer order), so the puzzle then is... how would you know? > >>> > >>> Another solution I toyed with - and indeed implemented first - was to > >>> do it in the configure() method, but then I had to dig around and find > >>> any raw stream configurations and update the pixelFormat post facto. > >>> This involves changing stream formats after validate(), which seemed > >>> bad to me too... but do we prefer it? > >> > >> Ayeee - no, we can't change a format after validate either. > >> Oh dear have we deadlocked... > >> > >> So - is the issue that the rotation/transform affects the pixel format? > >> > >> Are we intending to support those transforms on a per-frame basis? or > >> just a per-stream basis. > >> > >> As long as it's per-stream - Can we just store the state of the expected > >> rotation in the RPiCameraConfiguration, so that we can use it at > >> configure time appropriately? Or is it that we will need to determine > >> what adjustment will be made to the pixelformat based on the transform. > > > > The latter. We need to report to the user which raw format will be used > > based on the transformation. > > > >> And if that's the case, I assume it's the kernel driver which is going > >> to tell us what the new format will be. I fear we might have to > >> duplicate the determination of the format up here, and report the > >> adjustment. > >> > >> I guess what we're lacking is the ability to do a full atomic 'try' of > >> the state of the device at the driver level ... > > > > Correct, and I doubt V4L2 will ever give us that. > > > > I think we need to adjust the format in the pipeline handler, instead of > > querying the device. We however need to know if h/v flip will affect the > > bayer pattern, or if the sensor will compensate by shifting the crop > > rectangle. > > > > One option is to query the sensor driver at match time, to see if the > > media bus format changes when the h/v flip controls are modified. A bit > > of a hassle, but shouldn't be too complicated. Another option would be > > to ignore all this and consider that a proper sensor driver will never > > shift the crop rectangle behind the scene. We have the option to > > establish a set of rules for a sensor driver to be supported by > > libcamera. > > You have a very good indicator of that from the > V4L2_CTRL_FLAG_MODIFY_LAYOUT flag when querying controls. It would be > fair to deduce from that flag on flips that it is going to change the > bayer order. I've yet to see a Bayer sensor that offers transpose, but > there may be some out there. Good point, that's easier than changing the controls and checking the media bus format. > IMX327 and IMX290 are two that don't alter the Bayer order when using > the on-sensor flip controls. Reading the datasheet for IMX327 (page > 58), it explicitly states for the full frame mode the active lines are > "Line No during normal operation" 21 to 1100 > "Line No during inverted operation" 1101 to 22 > and > "Horizontal pixel output image normal operation" 13 to1932 > "Horizontal pixel output image inverted operation" 1933 to 14. > > I'm also aware of one further sensor (can't say what) that I know for > definite doesn't change the Bayer order. Exactly how it achieves that > one can only assume is by shifting the readout by one pixel. > > Should the crop reported by G_SELECTION be shifted in response to the > flips then? I think it should, yes. > >>>>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > >>>>> --- > >>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 18 +++++++++++------- > >>>>> 1 file changed, 11 insertions(+), 7 deletions(-) > >>>>> > >>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>>>> index 42c9caa..7aace71 100644 > >>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>>>> @@ -400,6 +400,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > >>>>> if (config_.empty()) > >>>>> return Invalid; > >>>>> > >>>>> + /* > >>>>> + * Configure the H/V flip controls based on the sensor rotation. We do > >>>>> + * this here so that the sensor has the correct Bayer format that will > >>>>> + * get advertised in the configuration of any raw streams. > >>>>> + */ > >>>>> + ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); > >>>>> + int32_t rotation = data_->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)); > >>>>> + data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); > >>>>> + > >>>>> unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0; > >>>>> std::pair<int, Size> outSize[2]; > >>>>> Size maxSize; > >>>>> @@ -1164,13 +1175,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) { > >>>>>
On Fri, 28 Aug 2020 at 17:03, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Dave, > > On Fri, Aug 28, 2020 at 04:55:14PM +0100, Dave Stevenson wrote: > > On Fri, 28 Aug 2020 at 16:36, Laurent Pinchart wrote: > > > On Fri, Aug 28, 2020 at 04:26:05PM +0100, Kieran Bingham wrote: > > >> On 28/08/2020 16:14, David Plowman wrote: > > >>> On Fri, 28 Aug 2020 at 16:02, Kieran Bingham wrote: > > >>>> On 28/08/2020 15:41, David Plowman wrote: > > >>>>> We set the sensor orientation (h and v flips) during validate as this > > >>>>> will in general affect the Bayer order output by the sensor. Doing it > > >>>>> here means that the correct raw format gets advertised in any raw > > >>>>> streams that the application requested. > > >>>> > > >>>> Eeep - I'm not sure if we could do this in validate(). > > >>>> > > >>>> Validation should not actually make any change to the hardware, but it > > >>>> should check that the configuration can be applied correctly, and make > > >>>> any changes that would be necessary to support a correct (and 'valid') > > >>>> configuration to be applied through the ->configure() > > >>> > > >>> Yes, that's an interesting one. The reason for doing it here is so > > >>> that the Bayer format comes out correctly for any raw streams that > > >>> were requested, and we're relying on the camera driver to give us the > > >>> true Bayer order. > > >>> > > >>> Of course, the camera driver doesn't *have* to change the Bayer order > > >>> when you transform it (it might do 1-pixel shits to maintain the > > >> > > >> haha, I hadn't noticed the typo until your follow up post ;-) > > >> > > >>> original Bayer order), so the puzzle then is... how would you know? > > >>> > > >>> Another solution I toyed with - and indeed implemented first - was to > > >>> do it in the configure() method, but then I had to dig around and find > > >>> any raw stream configurations and update the pixelFormat post facto. > > >>> This involves changing stream formats after validate(), which seemed > > >>> bad to me too... but do we prefer it? > > >> > > >> Ayeee - no, we can't change a format after validate either. > > >> Oh dear have we deadlocked... > > >> > > >> So - is the issue that the rotation/transform affects the pixel format? > > >> > > >> Are we intending to support those transforms on a per-frame basis? or > > >> just a per-stream basis. > > >> > > >> As long as it's per-stream - Can we just store the state of the expected > > >> rotation in the RPiCameraConfiguration, so that we can use it at > > >> configure time appropriately? Or is it that we will need to determine > > >> what adjustment will be made to the pixelformat based on the transform. > > > > > > The latter. We need to report to the user which raw format will be used > > > based on the transformation. > > > > > >> And if that's the case, I assume it's the kernel driver which is going > > >> to tell us what the new format will be. I fear we might have to > > >> duplicate the determination of the format up here, and report the > > >> adjustment. > > >> > > >> I guess what we're lacking is the ability to do a full atomic 'try' of > > >> the state of the device at the driver level ... > > > > > > Correct, and I doubt V4L2 will ever give us that. > > > > > > I think we need to adjust the format in the pipeline handler, instead of > > > querying the device. We however need to know if h/v flip will affect the > > > bayer pattern, or if the sensor will compensate by shifting the crop > > > rectangle. > > > > > > One option is to query the sensor driver at match time, to see if the > > > media bus format changes when the h/v flip controls are modified. A bit > > > of a hassle, but shouldn't be too complicated. Another option would be > > > to ignore all this and consider that a proper sensor driver will never > > > shift the crop rectangle behind the scene. We have the option to > > > establish a set of rules for a sensor driver to be supported by > > > libcamera. > > > > You have a very good indicator of that from the > > V4L2_CTRL_FLAG_MODIFY_LAYOUT flag when querying controls. It would be > > fair to deduce from that flag on flips that it is going to change the > > bayer order. I've yet to see a Bayer sensor that offers transpose, but > > there may be some out there. > > Good point, that's easier than changing the controls and checking the > media bus format. > > > IMX327 and IMX290 are two that don't alter the Bayer order when using > > the on-sensor flip controls. Reading the datasheet for IMX327 (page > > 58), it explicitly states for the full frame mode the active lines are > > "Line No during normal operation" 21 to 1100 > > "Line No during inverted operation" 1101 to 22 > > and > > "Horizontal pixel output image normal operation" 13 to1932 > > "Horizontal pixel output image inverted operation" 1933 to 14. > > > > I'm also aware of one further sensor (can't say what) that I know for > > definite doesn't change the Bayer order. Exactly how it achieves that > > one can only assume is by shifting the readout by one pixel. > > > > Should the crop reported by G_SELECTION be shifted in response to the > > flips then? > > I think it should, yes. You make writing sensor drivers such fun! ;-) At least with that one you don't have to fight the v4l2_ctrl framework. > > >>>>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > >>>>> --- > > >>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 18 +++++++++++------- > > >>>>> 1 file changed, 11 insertions(+), 7 deletions(-) > > >>>>> > > >>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > >>>>> index 42c9caa..7aace71 100644 > > >>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > >>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > >>>>> @@ -400,6 +400,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > > >>>>> if (config_.empty()) > > >>>>> return Invalid; > > >>>>> > > >>>>> + /* > > >>>>> + * Configure the H/V flip controls based on the sensor rotation. We do > > >>>>> + * this here so that the sensor has the correct Bayer format that will > > >>>>> + * get advertised in the configuration of any raw streams. > > >>>>> + */ > > >>>>> + ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); > > >>>>> + int32_t rotation = data_->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)); > > >>>>> + data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); > > >>>>> + > > >>>>> unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0; > > >>>>> std::pair<int, Size> outSize[2]; > > >>>>> Size maxSize; > > >>>>> @@ -1164,13 +1175,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
On Fri, Aug 28, 2020 at 05:13:09PM +0100, Dave Stevenson wrote: > On Fri, 28 Aug 2020 at 17:03, Laurent Pinchart wrote: > > On Fri, Aug 28, 2020 at 04:55:14PM +0100, Dave Stevenson wrote: > >> On Fri, 28 Aug 2020 at 16:36, Laurent Pinchart wrote: > >>> On Fri, Aug 28, 2020 at 04:26:05PM +0100, Kieran Bingham wrote: > >>>> On 28/08/2020 16:14, David Plowman wrote: > >>>>> On Fri, 28 Aug 2020 at 16:02, Kieran Bingham wrote: > >>>>>> On 28/08/2020 15:41, David Plowman wrote: > >>>>>>> We set the sensor orientation (h and v flips) during validate as this > >>>>>>> will in general affect the Bayer order output by the sensor. Doing it > >>>>>>> here means that the correct raw format gets advertised in any raw > >>>>>>> streams that the application requested. > >>>>>> > >>>>>> Eeep - I'm not sure if we could do this in validate(). > >>>>>> > >>>>>> Validation should not actually make any change to the hardware, but it > >>>>>> should check that the configuration can be applied correctly, and make > >>>>>> any changes that would be necessary to support a correct (and 'valid') > >>>>>> configuration to be applied through the ->configure() > >>>>> > >>>>> Yes, that's an interesting one. The reason for doing it here is so > >>>>> that the Bayer format comes out correctly for any raw streams that > >>>>> were requested, and we're relying on the camera driver to give us the > >>>>> true Bayer order. > >>>>> > >>>>> Of course, the camera driver doesn't *have* to change the Bayer order > >>>>> when you transform it (it might do 1-pixel shits to maintain the > >>>> > >>>> haha, I hadn't noticed the typo until your follow up post ;-) > >>>> > >>>>> original Bayer order), so the puzzle then is... how would you know? > >>>>> > >>>>> Another solution I toyed with - and indeed implemented first - was to > >>>>> do it in the configure() method, but then I had to dig around and find > >>>>> any raw stream configurations and update the pixelFormat post facto. > >>>>> This involves changing stream formats after validate(), which seemed > >>>>> bad to me too... but do we prefer it? > >>>> > >>>> Ayeee - no, we can't change a format after validate either. > >>>> Oh dear have we deadlocked... > >>>> > >>>> So - is the issue that the rotation/transform affects the pixel format? > >>>> > >>>> Are we intending to support those transforms on a per-frame basis? or > >>>> just a per-stream basis. > >>>> > >>>> As long as it's per-stream - Can we just store the state of the expected > >>>> rotation in the RPiCameraConfiguration, so that we can use it at > >>>> configure time appropriately? Or is it that we will need to determine > >>>> what adjustment will be made to the pixelformat based on the transform. > >>> > >>> The latter. We need to report to the user which raw format will be used > >>> based on the transformation. > >>> > >>>> And if that's the case, I assume it's the kernel driver which is going > >>>> to tell us what the new format will be. I fear we might have to > >>>> duplicate the determination of the format up here, and report the > >>>> adjustment. > >>>> > >>>> I guess what we're lacking is the ability to do a full atomic 'try' of > >>>> the state of the device at the driver level ... > >>> > >>> Correct, and I doubt V4L2 will ever give us that. > >>> > >>> I think we need to adjust the format in the pipeline handler, instead of > >>> querying the device. We however need to know if h/v flip will affect the > >>> bayer pattern, or if the sensor will compensate by shifting the crop > >>> rectangle. > >>> > >>> One option is to query the sensor driver at match time, to see if the > >>> media bus format changes when the h/v flip controls are modified. A bit > >>> of a hassle, but shouldn't be too complicated. Another option would be > >>> to ignore all this and consider that a proper sensor driver will never > >>> shift the crop rectangle behind the scene. We have the option to > >>> establish a set of rules for a sensor driver to be supported by > >>> libcamera. > >> > >> You have a very good indicator of that from the > >> V4L2_CTRL_FLAG_MODIFY_LAYOUT flag when querying controls. It would be > >> fair to deduce from that flag on flips that it is going to change the > >> bayer order. I've yet to see a Bayer sensor that offers transpose, but > >> there may be some out there. > > > > Good point, that's easier than changing the controls and checking the > > media bus format. > > > >> IMX327 and IMX290 are two that don't alter the Bayer order when using > >> the on-sensor flip controls. Reading the datasheet for IMX327 (page > >> 58), it explicitly states for the full frame mode the active lines are > >> "Line No during normal operation" 21 to 1100 > >> "Line No during inverted operation" 1101 to 22 > >> and > >> "Horizontal pixel output image normal operation" 13 to1932 > >> "Horizontal pixel output image inverted operation" 1933 to 14. > >> > >> I'm also aware of one further sensor (can't say what) that I know for > >> definite doesn't change the Bayer order. Exactly how it achieves that > >> one can only assume is by shifting the readout by one pixel. > >> > >> Should the crop reported by G_SELECTION be shifted in response to the > >> flips then? > > > > I think it should, yes. > > You make writing sensor drivers such fun! ;-) > At least with that one you don't have to fight the v4l2_ctrl framework. Does it show that v4l2_subdev hasn't been designed with a good enough understanding of camera sensors ? :-) Or, to be fairer, when previously unsupported features of sensors had to be supported, the API and how it was used by drivers never went through a coordinated design phase. > >>>>>>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > >>>>>>> --- > >>>>>>> .../pipeline/raspberrypi/raspberrypi.cpp | 18 +++++++++++------- > >>>>>>> 1 file changed, 11 insertions(+), 7 deletions(-) > >>>>>>> > >>>>>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>>>>>> index 42c9caa..7aace71 100644 > >>>>>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>>>>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > >>>>>>> @@ -400,6 +400,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() > >>>>>>> if (config_.empty()) > >>>>>>> return Invalid; > >>>>>>> > >>>>>>> + /* > >>>>>>> + * Configure the H/V flip controls based on the sensor rotation. We do > >>>>>>> + * this here so that the sensor has the correct Bayer format that will > >>>>>>> + * get advertised in the configuration of any raw streams. > >>>>>>> + */ > >>>>>>> + ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); > >>>>>>> + int32_t rotation = data_->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)); > >>>>>>> + data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); > >>>>>>> + > >>>>>>> unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0; > >>>>>>> std::pair<int, Size> outSize[2]; > >>>>>>> Size maxSize; > >>>>>>> @@ -1164,13 +1175,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) { > >>>>>>>
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 42c9caa..7aace71 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -400,6 +400,17 @@ CameraConfiguration::Status RPiCameraConfiguration::validate() if (config_.empty()) return Invalid; + /* + * Configure the H/V flip controls based on the sensor rotation. We do + * this here so that the sensor has the correct Bayer format that will + * get advertised in the configuration of any raw streams. + */ + ControlList ctrls(data_->unicam_[Unicam::Image].dev()->controls()); + int32_t rotation = data_->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)); + data_->unicam_[Unicam::Image].dev()->setControls(&ctrls); + unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0; std::pair<int, Size> outSize[2]; Size maxSize; @@ -1164,13 +1175,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) {
We set the sensor orientation (h and v flips) during validate as this will in general affect the Bayer order output by the sensor. Doing it here means that the correct raw format gets advertised in any raw streams that the application requested. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)