| Message ID | 20210111163359.43637-2-email@uajain.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 11/01/2021 16:33, Umang Jain wrote: > From: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Track the state of streamon/streamoff calls to simplify error paths. > > Ensuring that streamOff() can be called on non-streaming streams > facilitates simpler error code paths, where a set of devices can all > call streamOff regardless of their initialisation state. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Umang Jain <email@uajain.com> Thanks for re-vitalising this patch, I thought it wasn't useful back then, so it's nice to see it re-surface and has become useful again. > --- > include/libcamera/internal/v4l2_videodevice.h | 2 ++ > src/libcamera/v4l2_videodevice.cpp | 9 ++++++++- > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > index 529ca0e3..626dfbcd 100644 > --- a/include/libcamera/internal/v4l2_videodevice.h > +++ b/include/libcamera/internal/v4l2_videodevice.h > @@ -249,6 +249,8 @@ private: > std::map<unsigned int, FrameBuffer *> queuedBuffers_; > > EventNotifier *fdBufferNotifier_; > + > + bool streaming_; > }; > > class V4L2M2MDevice > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index baf683d6..164954f2 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -481,7 +481,8 @@ const std::string V4L2DeviceFormat::toString() const > * \param[in] deviceNode The file-system path to the video device node > */ > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode) > - : V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr) > + : V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr), > + streaming_(false) > { > /* > * We default to an MMAP based CAPTURE video device, however this will > @@ -1554,6 +1555,8 @@ int V4L2VideoDevice::streamOn() > return ret; > } > > + streaming_ = true; > + > return 0; > } > > @@ -1571,6 +1574,9 @@ int V4L2VideoDevice::streamOff() > { > int ret; > > + if (!streaming_ && queuedBuffers_.empty()) > + return 0; > + ah nice, I was worried that we were going to be modifying streaming_ when queuing buffers ... so that looks much nicer and doesn't detract from the meaning of streaming_ So Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Both Niklas and Laurent had reviewed the earlier incantation of this patch, but it has now changed with this line so I'll let them decide if they want to re-give tags. But Laurent's previous comment regarding documentation update still holds: > I forgot to mention that previously, I think the documentation should be > updated to state that stopping an already stopped device is a no-op. -- Kieran > ret = ioctl(VIDIOC_STREAMOFF, &bufferType_); > if (ret < 0) { > LOG(V4L2, Error) > @@ -1588,6 +1594,7 @@ int V4L2VideoDevice::streamOff() > > queuedBuffers_.clear(); > fdBufferNotifier_->setEnabled(false); > + streaming_ = false; > > return 0; > } >
diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h index 529ca0e3..626dfbcd 100644 --- a/include/libcamera/internal/v4l2_videodevice.h +++ b/include/libcamera/internal/v4l2_videodevice.h @@ -249,6 +249,8 @@ private: std::map<unsigned int, FrameBuffer *> queuedBuffers_; EventNotifier *fdBufferNotifier_; + + bool streaming_; }; class V4L2M2MDevice diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index baf683d6..164954f2 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -481,7 +481,8 @@ const std::string V4L2DeviceFormat::toString() const * \param[in] deviceNode The file-system path to the video device node */ V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode) - : V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr) + : V4L2Device(deviceNode), cache_(nullptr), fdBufferNotifier_(nullptr), + streaming_(false) { /* * We default to an MMAP based CAPTURE video device, however this will @@ -1554,6 +1555,8 @@ int V4L2VideoDevice::streamOn() return ret; } + streaming_ = true; + return 0; } @@ -1571,6 +1574,9 @@ int V4L2VideoDevice::streamOff() { int ret; + if (!streaming_ && queuedBuffers_.empty()) + return 0; + ret = ioctl(VIDIOC_STREAMOFF, &bufferType_); if (ret < 0) { LOG(V4L2, Error) @@ -1588,6 +1594,7 @@ int V4L2VideoDevice::streamOff() queuedBuffers_.clear(); fdBufferNotifier_->setEnabled(false); + streaming_ = false; return 0; }