| Message ID | 20221110144556.7858-5-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David On Thu, Nov 10, 2022 at 02:45:54PM +0000, David Plowman via libcamera-devel wrote: > Set the horizontal and vertical flip controls correctly by > calling the device's setTransform method now that this no longer > happens in CameraSensor::init(). > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 277465b7..4a891c23 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -209,6 +209,9 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config) > StreamConfiguration &cfg = config->at(0); > int ret; > > + /* Set up the video device's horizontal and vertical flips. */ > + data->video_->setTransform(config->transform); > + Do we need this ? The validate() function does if (transform != Transform::Identity) { transform = Transform::Identity; status = Adjusted; } Same for simple and rkisp1. This opens a different question. What happens if another application sets a flip before libcamera is started ? Should we here instead reset all flips to their default values ? Using the newly introduced setTrasform(Identity) wouldn't be enough as it would not clean up any externally set flip... What would the expected configuration from the application point of view ? Whatever flip is externally set is kept, or all flips are reset to their default ? Please note that "reset to their defaults" != "clear any flip flag" as most drivers (and we discussed this with Dave recently) do inizialize their flip controls to report what is actually programmed in registers, and quite often sensors are 180 degrees flipped by default: https://lore.kernel.org/all/CAPY8ntCCpMZtG-eJQ8cgd_GO06ZDP32oRK8mGrZKGMU2W401Dg@mail.gmail.com/ This is one more reason to remove the "set flips to 0" in init. Do we need a V4L2Device::resetFlips() function instead ? > V4L2DeviceFormat format; > format.fourcc = data->video_->toV4L2PixelFormat(cfg.pixelFormat); > format.size = cfg.size; > -- > 2.30.2 >
Hi Jacopo Thanks for the comments! On Thu, 17 Nov 2022 at 10:32, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi David > > On Thu, Nov 10, 2022 at 02:45:54PM +0000, David Plowman via libcamera-devel wrote: > > Set the horizontal and vertical flip controls correctly by > > calling the device's setTransform method now that this no longer > > happens in CameraSensor::init(). > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > index 277465b7..4a891c23 100644 > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > @@ -209,6 +209,9 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config) > > StreamConfiguration &cfg = config->at(0); > > int ret; > > > > + /* Set up the video device's horizontal and vertical flips. */ > > + data->video_->setTransform(config->transform); > > + > > Do we need this ? The validate() function does > > if (transform != Transform::Identity) { > transform = Transform::Identity; > status = Adjusted; > } > > Same for simple and rkisp1. > > This opens a different question. What happens if another application > sets a flip before libcamera is started ? Should we here instead > reset all flips to their default values ? Using the newly introduced > setTrasform(Identity) wouldn't be enough as it would not clean up any > externally set flip... > > What would the expected configuration from the application point of > view ? Whatever flip is externally set is kept, or all flips > are reset to their default ? Please note that "reset to their > defaults" != "clear any flip flag" as most drivers (and we discussed > this with Dave recently) do inizialize their flip controls to report > what is actually programmed in registers, and quite often sensors are > 180 degrees flipped by default: > https://lore.kernel.org/all/CAPY8ntCCpMZtG-eJQ8cgd_GO06ZDP32oRK8mGrZKGMU2W401Dg@mail.gmail.com/ > This is one more reason to remove the "set flips to 0" in init. > > Do we need a V4L2Device::resetFlips() function instead ? That's all interesting. Indeed I see there's a problem with devices that might have non-zero defaults, or where someone else has changed them without us being aware. As regards uvcvideo, maybe the best thing is then just to *do nothing*? At least that preserves the current behaviour. Although the pipeline handler doesn't support flips, someone could actually set them externally and they would be preserved. If the pipeline handler ever does support flips (should it one day?) we can figure out how it might work then. Probably the same argument holds for the simple PH too? I wonder if rkisp1 is different. That uses the CameraSensor class, so previously I assume it was clearing the flip bits. Therefore resetting the transform (it's always the Identity) might be correct? If a change of behaviour is necessary (actually handling the transforms would be ideal!), that's probably a separate piece of work. Thoughts? David > > > V4L2DeviceFormat format; > > format.fourcc = data->video_->toV4L2PixelFormat(cfg.pixelFormat); > > format.size = cfg.size; > > -- > > 2.30.2 > >
Hi David, On Thu, Nov 17, 2022 at 01:02:29PM +0000, David Plowman wrote: > Hi Jacopo > > Thanks for the comments! > > On Thu, 17 Nov 2022 at 10:32, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi David > > > > On Thu, Nov 10, 2022 at 02:45:54PM +0000, David Plowman via libcamera-devel wrote: > > > Set the horizontal and vertical flip controls correctly by > > > calling the device's setTransform method now that this no longer > > > happens in CameraSensor::init(). > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > index 277465b7..4a891c23 100644 > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > @@ -209,6 +209,9 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config) > > > StreamConfiguration &cfg = config->at(0); > > > int ret; > > > > > > + /* Set up the video device's horizontal and vertical flips. */ > > > + data->video_->setTransform(config->transform); > > > + > > > > Do we need this ? The validate() function does > > > > if (transform != Transform::Identity) { > > transform = Transform::Identity; > > status = Adjusted; > > } > > > > Same for simple and rkisp1. > > > > This opens a different question. What happens if another application > > sets a flip before libcamera is started ? Should we here instead > > reset all flips to their default values ? Using the newly introduced > > setTrasform(Identity) wouldn't be enough as it would not clean up any > > externally set flip... > > > > What would the expected configuration from the application point of > > view ? Whatever flip is externally set is kept, or all flips > > are reset to their default ? Please note that "reset to their > > defaults" != "clear any flip flag" as most drivers (and we discussed > > this with Dave recently) do inizialize their flip controls to report > > what is actually programmed in registers, and quite often sensors are > > 180 degrees flipped by default: > > https://lore.kernel.org/all/CAPY8ntCCpMZtG-eJQ8cgd_GO06ZDP32oRK8mGrZKGMU2W401Dg@mail.gmail.com/ > > This is one more reason to remove the "set flips to 0" in init. > > > > Do we need a V4L2Device::resetFlips() function instead ? > > That's all interesting. Indeed I see there's a problem with devices > that might have non-zero defaults, or where someone else has changed > them without us being aware. > > As regards uvcvideo, maybe the best thing is then just to *do > nothing*? At least that preserves the current behaviour. Although the > pipeline handler doesn't support flips, someone could actually set > them externally and they would be preserved. If the pipeline handler > ever does support flips (should it one day?) we can figure out how it On "should it be one day": I presume so, but I can't tell if there are uvc-specific reasons why this can't be made > might work then. I presume so. Again not a uvc expert > > Probably the same argument holds for the simple PH too? > > I wonder if rkisp1 is different. That uses the CameraSensor class, so > previously I assume it was clearing the flip bits. Therefore resetting > the transform (it's always the Identity) might be correct? If a change It seems to me Simple uses CameraSensor as well, but yes, it's mostly intended to work with YUV/RGB sensors. Nothing prevents to use RAW sensors though. The idea is to have one day a GPU-based ISP implementation that could be plugged to Simple, so RAW+Simple is a use case we care about.. My concern is only about RAW sensors. - Assume a sensor driver reports BGGR with both v4l2 flips set to 1 and both report V4L2_CTRL_FLAG_MODIFY_LAYOUT. - With your patch we transform it to RGGB and report it as properties::ColorFilterArrangement (should we rename it btw?). - If any other user changes on filp it changes the bayer components ordering too and applications might get confused ? Is something we should be concerned ? > of behaviour is necessary (actually handling the transforms would be > ideal!), that's probably a separate piece of work. > > Thoughts? > > David > > > > > > V4L2DeviceFormat format; > > > format.fourcc = data->video_->toV4L2PixelFormat(cfg.pixelFormat); > > > format.size = cfg.size; > > > -- > > > 2.30.2 > > >
Hi Jacopo On Thu, 17 Nov 2022 at 18:17, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi David, > > On Thu, Nov 17, 2022 at 01:02:29PM +0000, David Plowman wrote: > > Hi Jacopo > > > > Thanks for the comments! > > > > On Thu, 17 Nov 2022 at 10:32, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi David > > > > > > On Thu, Nov 10, 2022 at 02:45:54PM +0000, David Plowman via libcamera-devel wrote: > > > > Set the horizontal and vertical flip controls correctly by > > > > calling the device's setTransform method now that this no longer > > > > happens in CameraSensor::init(). > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > > index 277465b7..4a891c23 100644 > > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > > @@ -209,6 +209,9 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config) > > > > StreamConfiguration &cfg = config->at(0); > > > > int ret; > > > > > > > > + /* Set up the video device's horizontal and vertical flips. */ > > > > + data->video_->setTransform(config->transform); > > > > + > > > > > > Do we need this ? The validate() function does > > > > > > if (transform != Transform::Identity) { > > > transform = Transform::Identity; > > > status = Adjusted; > > > } > > > > > > Same for simple and rkisp1. > > > > > > This opens a different question. What happens if another application > > > sets a flip before libcamera is started ? Should we here instead > > > reset all flips to their default values ? Using the newly introduced > > > setTrasform(Identity) wouldn't be enough as it would not clean up any > > > externally set flip... > > > > > > What would the expected configuration from the application point of > > > view ? Whatever flip is externally set is kept, or all flips > > > are reset to their default ? Please note that "reset to their > > > defaults" != "clear any flip flag" as most drivers (and we discussed > > > this with Dave recently) do inizialize their flip controls to report > > > what is actually programmed in registers, and quite often sensors are > > > 180 degrees flipped by default: > > > https://lore.kernel.org/all/CAPY8ntCCpMZtG-eJQ8cgd_GO06ZDP32oRK8mGrZKGMU2W401Dg@mail.gmail.com/ > > > This is one more reason to remove the "set flips to 0" in init. > > > > > > Do we need a V4L2Device::resetFlips() function instead ? > > > > That's all interesting. Indeed I see there's a problem with devices > > that might have non-zero defaults, or where someone else has changed > > them without us being aware. > > > > As regards uvcvideo, maybe the best thing is then just to *do > > nothing*? At least that preserves the current behaviour. Although the > > pipeline handler doesn't support flips, someone could actually set > > them externally and they would be preserved. If the pipeline handler > > ever does support flips (should it one day?) we can figure out how it > > On "should it be one day": I presume so, but I can't tell if there are > uvc-specific reasons why this can't be made > > > might work then. > > I presume so. Again not a uvc expert > > > > > Probably the same argument holds for the simple PH too? > > > > I wonder if rkisp1 is different. That uses the CameraSensor class, so > > previously I assume it was clearing the flip bits. Therefore resetting > > the transform (it's always the Identity) might be correct? If a change > > It seems to me Simple uses CameraSensor as well, but yes, it's mostly > intended to work with YUV/RGB sensors. Nothing prevents to use RAW > sensors though. The idea is to have one day a GPU-based ISP > implementation that could be plugged to Simple, so RAW+Simple is a use > case we care about.. Perhaps we should clear the flips in the simple PH too, if that's what it was doing previously. It might be right or wrong, but it's not any more broken...! > > My concern is only about RAW sensors. > > - Assume a sensor driver reports BGGR with both v4l2 flips set to 1 and both > report V4L2_CTRL_FLAG_MODIFY_LAYOUT. > - With your patch we transform it to RGGB and report it as > properties::ColorFilterArrangement (should we rename it btw?). I don't mind ColorFilterArrangement... apart from the non-British spelling of "Color" of course! > - If any other user changes on filp it changes the bayer components > ordering too and applications might get confused ? > > Is something we should be concerned ? I think we should be concerned, yes. But perhaps for the moment we might be better off just preserving the previous behaviour and at some point someone will have to look again at them, perhaps when they have raw test cases. I'm feeling then that both rkisp1 and simple PHs should (possibly? perhaps? maybe?) clear the flips to be like they were before, and uvc should leave them alone (again, like before). Does that sound like enough for the next version...? Thanks! David > > > of behaviour is necessary (actually handling the transforms would be > > ideal!), that's probably a separate piece of work. > > > > Thoughts? > > > > David > > > > > > > > > V4L2DeviceFormat format; > > > > format.fourcc = data->video_->toV4L2PixelFormat(cfg.pixelFormat); > > > > format.size = cfg.size; > > > > -- > > > > 2.30.2 > > > >
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index 277465b7..4a891c23 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -209,6 +209,9 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config) StreamConfiguration &cfg = config->at(0); int ret; + /* Set up the video device's horizontal and vertical flips. */ + data->video_->setTransform(config->transform); + V4L2DeviceFormat format; format.fourcc = data->video_->toV4L2PixelFormat(cfg.pixelFormat); format.size = cfg.size;
Set the horizontal and vertical flip controls correctly by calling the device's setTransform method now that this no longer happens in CameraSensor::init(). Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 +++ 1 file changed, 3 insertions(+)