Message ID | 20250314174248.1015718-4-barnabas.pocze@ideasonboard.com |
---|---|
State | Deferred |
Headers | show |
Series |
|
Related | show |
Hi Barnabás On Fri, Mar 14, 2025 at 06:42:47PM +0100, Barnabás Pőcze wrote: > If `ExposureTimeMode` is supported, then retrieve the current value > of `V4L2_CID_EXPOSURE_AUTO` in `PipelineHandlerUVC::start()`, and > convert it to the appropriate `ExposureTimeModeEnum` value. If it > is not supported or the conversion fails, then fall back to assuming > that `ExposureTimeModeManual` is in effect. > > This is necessary to be able to properly handle the `ExposureTime` > control as its value is required to be ignored if `ExposureTimeMode` > is not manual. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 4ff79c291..7d882ebe1 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -62,6 +62,15 @@ public: > std::optional<v4l2_exposure_auto_type> autoExposureMode_; > std::optional<v4l2_exposure_auto_type> manualExposureMode_; > > + struct State { > + std::optional<controls::ExposureTimeModeEnum> exp; > + > + void reset() > + { > + exp.reset(); > + } > + } state_; Do you expect this to grow ? Otherwise there's no much difference than just using an optional<> directly. > + > private: > bool generateId(); > > @@ -303,6 +312,16 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList > return ret; > } > > + if (data->autoExposureMode_ || data->manualExposureMode_) { > + const auto &ctrls = data->video_->getControls({ V4L2_CID_EXPOSURE_AUTO }); > + const auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO); Is this safe in case there's no V4L2_CID_EXPOSURE_AUTO ? Isn't it safer to check if (!ctrls.empty()) { const auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO); data->state_.exp = v4l2ToExposureMode(value.get<int32_t>()); } > + if (!value.isNone()) > + data->state_.exp = v4l2ToExposureMode(value.get<int32_t>()); > + } > + > + if (!data->state_.exp) > + data->state_.exp = controls::ExposureTimeModeManual; > + I presume this is safer than just relying on the control default value, specifically in start-stop-start sessions (if controls are not reset in between ofc) > return 0; > } > > @@ -311,6 +330,7 @@ void PipelineHandlerUVC::stopDevice(Camera *camera) > UVCCameraData *data = cameraData(camera); > data->video_->streamOff(); > data->video_->releaseBuffers(); > + data->state_.reset(); > } > > int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *controls, > -- > 2.48.1 >
Hi 2025. 03. 21. 15:02 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Fri, Mar 14, 2025 at 06:42:47PM +0100, Barnabás Pőcze wrote: >> If `ExposureTimeMode` is supported, then retrieve the current value >> of `V4L2_CID_EXPOSURE_AUTO` in `PipelineHandlerUVC::start()`, and >> convert it to the appropriate `ExposureTimeModeEnum` value. If it >> is not supported or the conversion fails, then fall back to assuming >> that `ExposureTimeModeManual` is in effect. >> >> This is necessary to be able to properly handle the `ExposureTime` >> control as its value is required to be ignored if `ExposureTimeMode` >> is not manual. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> >> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp >> index 4ff79c291..7d882ebe1 100644 >> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp >> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp >> @@ -62,6 +62,15 @@ public: >> std::optional<v4l2_exposure_auto_type> autoExposureMode_; >> std::optional<v4l2_exposure_auto_type> manualExposureMode_; >> >> + struct State { >> + std::optional<controls::ExposureTimeModeEnum> exp; >> + >> + void reset() >> + { >> + exp.reset(); >> + } >> + } state_; > > Do you expect this to grow ? Otherwise there's no much difference than > just using an optional<> directly. No idea to be honest, but state objects are passed to functions, so it's certainly less typing. > >> + >> private: >> bool generateId(); >> >> @@ -303,6 +312,16 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList >> return ret; >> } >> >> + if (data->autoExposureMode_ || data->manualExposureMode_) { >> + const auto &ctrls = data->video_->getControls({ V4L2_CID_EXPOSURE_AUTO }); >> + const auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO); > > Is this safe in case there's no V4L2_CID_EXPOSURE_AUTO ? Isn't it > safer to check It is apparently not, documentation says undefined behaviour. I have added a `ctrls.contains(...)` check. > > if (!ctrls.empty()) { > const auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO); > data->state_.exp = v4l2ToExposureMode(value.get<int32_t>()); > > } >> + if (!value.isNone()) >> + data->state_.exp = v4l2ToExposureMode(value.get<int32_t>()); >> + } >> + >> + if (!data->state_.exp) >> + data->state_.exp = controls::ExposureTimeModeManual; >> + > > I presume this is safer than just relying on the control default > value, specifically in start-stop-start sessions (if controls are not > reset in between ofc) At this point I have different thoughts every time I look at this... This is a fallback path for very unlikely scenarios, but now I am thinking that the current value shouldn't even be retrieved for two reasons: * the current values of controls are not retrievable by the user application (with libcamera at least); * the user application will have to set `ExposureTimeMode=Manual` explicitly before setting `ExposureTime` at least once (otherwise correct operation is not guaranteed by the docs) and at that point up to date state information will be available. Or is there possibly an expectation that the controls will have their "default" values when streaming starts? That is surely not guaranteed by the UVC pipeline handler, do others guarantee that? Regards, Barnabás Pőcze > >> return 0; >> } >> >> @@ -311,6 +330,7 @@ void PipelineHandlerUVC::stopDevice(Camera *camera) >> UVCCameraData *data = cameraData(camera); >> data->video_->streamOff(); >> data->video_->releaseBuffers(); >> + data->state_.reset(); >> } >> >> int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *controls, >> -- >> 2.48.1 >>
Hi Barnabás On Fri, Mar 21, 2025 at 03:36:10PM +0100, Barnabás Pőcze wrote: > Hi > > 2025. 03. 21. 15:02 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Fri, Mar 14, 2025 at 06:42:47PM +0100, Barnabás Pőcze wrote: > > > If `ExposureTimeMode` is supported, then retrieve the current value > > > of `V4L2_CID_EXPOSURE_AUTO` in `PipelineHandlerUVC::start()`, and > > > convert it to the appropriate `ExposureTimeModeEnum` value. If it > > > is not supported or the conversion fails, then fall back to assuming > > > that `ExposureTimeModeManual` is in effect. > > > > > > This is necessary to be able to properly handle the `ExposureTime` > > > control as its value is required to be ignored if `ExposureTimeMode` > > > is not manual. > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 20 ++++++++++++++++++++ > > > 1 file changed, 20 insertions(+) > > > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > index 4ff79c291..7d882ebe1 100644 > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > @@ -62,6 +62,15 @@ public: > > > std::optional<v4l2_exposure_auto_type> autoExposureMode_; > > > std::optional<v4l2_exposure_auto_type> manualExposureMode_; > > > > > > + struct State { > > > + std::optional<controls::ExposureTimeModeEnum> exp; > > > + > > > + void reset() > > > + { > > > + exp.reset(); > > > + } > > > + } state_; > > > > Do you expect this to grow ? Otherwise there's no much difference than > > just using an optional<> directly. > > No idea to be honest, but state objects are passed to functions, so > it's certainly less typing. > True, and probably more tidy than just passing the optional<> around.. Let's make it clear this is about controls maybe naming the type ControlsState ? (uvc doesn't have an IPA, but in IPA-terms this would actually be the ActiveState..) > > > > > > + > > > private: > > > bool generateId(); > > > > > > @@ -303,6 +312,16 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList > > > return ret; > > > } > > > > > > + if (data->autoExposureMode_ || data->manualExposureMode_) { > > > + const auto &ctrls = data->video_->getControls({ V4L2_CID_EXPOSURE_AUTO }); > > > + const auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO); > > > > Is this safe in case there's no V4L2_CID_EXPOSURE_AUTO ? Isn't it > > safer to check > > It is apparently not, documentation says undefined behaviour. I have > added a `ctrls.contains(...)` check. > > > > > > if (!ctrls.empty()) { > > const auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO); > > data->state_.exp = v4l2ToExposureMode(value.get<int32_t>()); > > > > } > > > + if (!value.isNone()) > > > + data->state_.exp = v4l2ToExposureMode(value.get<int32_t>()); > > > + } > > > + > > > + if (!data->state_.exp) > > > + data->state_.exp = controls::ExposureTimeModeManual; > > > + > > > > I presume this is safer than just relying on the control default > > value, specifically in start-stop-start sessions (if controls are not > > reset in between ofc) > > At this point I have different thoughts every time I look at this... > This is a fallback path for very unlikely scenarios, but now I am > thinking that the current value shouldn't even be retrieved for > two reasons: > > * the current values of controls are not retrievable by the user application > (with libcamera at least); > * the user application will have to set `ExposureTimeMode=Manual` explicitly > before setting `ExposureTime` at least once (otherwise correct operation is > not guaranteed by the docs) and at that point up to date state information > will be available. > > Or is there possibly an expectation that the controls will have their "default" > values when streaming starts? That is surely not guaranteed by the UVC pipeline > handler, do others guarantee that? > > > Regards, > Barnabás Pőcze > > > > > > return 0; > > > } > > > > > > @@ -311,6 +330,7 @@ void PipelineHandlerUVC::stopDevice(Camera *camera) > > > UVCCameraData *data = cameraData(camera); > > > data->video_->streamOff(); > > > data->video_->releaseBuffers(); > > > + data->state_.reset(); > > > } > > > > > > int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *controls, > > > -- > > > 2.48.1 > > > >
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index 4ff79c291..7d882ebe1 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -62,6 +62,15 @@ public: std::optional<v4l2_exposure_auto_type> autoExposureMode_; std::optional<v4l2_exposure_auto_type> manualExposureMode_; + struct State { + std::optional<controls::ExposureTimeModeEnum> exp; + + void reset() + { + exp.reset(); + } + } state_; + private: bool generateId(); @@ -303,6 +312,16 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList return ret; } + if (data->autoExposureMode_ || data->manualExposureMode_) { + const auto &ctrls = data->video_->getControls({ V4L2_CID_EXPOSURE_AUTO }); + const auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO); + if (!value.isNone()) + data->state_.exp = v4l2ToExposureMode(value.get<int32_t>()); + } + + if (!data->state_.exp) + data->state_.exp = controls::ExposureTimeModeManual; + return 0; } @@ -311,6 +330,7 @@ void PipelineHandlerUVC::stopDevice(Camera *camera) UVCCameraData *data = cameraData(camera); data->video_->streamOff(); data->video_->releaseBuffers(); + data->state_.reset(); } int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *controls,
If `ExposureTimeMode` is supported, then retrieve the current value of `V4L2_CID_EXPOSURE_AUTO` in `PipelineHandlerUVC::start()`, and convert it to the appropriate `ExposureTimeModeEnum` value. If it is not supported or the conversion fails, then fall back to assuming that `ExposureTimeModeManual` is in effect. This is necessary to be able to properly handle the `ExposureTime` control as its value is required to be ignored if `ExposureTimeMode` is not manual. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)