Quoting Laurent Pinchart (2022-03-23 10:05:02)
> On Wed, Mar 23, 2022 at 09:59:02AM +0000, Kieran Bingham via libcamera-devel wrote:
> > Quoting Naushir Patuck via libcamera-devel (2022-03-22 09:22:54)
> > > Replace the existing streaming_ state variable with an enum to track the
> > > following three state: Streaming, Stopping, and Stopped. The alternate states
> > > will be used in a subsequent commit.
> > >
> > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > ---
> > > include/libcamera/internal/v4l2_videodevice.h | 3 ++-
> > > src/libcamera/v4l2_videodevice.cpp | 10 ++++++----
> > > 2 files changed, 8 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> > > index 2d2ccc477c91..32e022543385 100644
> > > --- a/include/libcamera/internal/v4l2_videodevice.h
> > > +++ b/include/libcamera/internal/v4l2_videodevice.h
> > > @@ -258,7 +258,8 @@ private:
> > >
> > > EventNotifier *fdBufferNotifier_;
> > >
> > > - bool streaming_;
> > > + enum class State { Streaming, Stopping, Stopped };
> > > + State state_;
> > > };
> > >
> > > class V4L2M2MDevice
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index 5f36ee20710d..9cea6a608660 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -507,7 +507,7 @@ const std::string V4L2DeviceFormat::toString() const
> > > */
> > > V4L2VideoDevice::V4L2VideoDevice(const std::string &deviceNode)
> > > : V4L2Device(deviceNode), formatInfo_(nullptr), cache_(nullptr),
> > > - fdBufferNotifier_(nullptr), streaming_(false)
> > > + fdBufferNotifier_(nullptr), state_(State::Stopped)
> > > {
> > > /*
> > > * We default to an MMAP based CAPTURE video device, however this will
> > > @@ -1796,7 +1796,7 @@ int V4L2VideoDevice::streamOn()
> > > return ret;
> > > }
> > >
> > > - streaming_ = true;
> > > + state_ = State::Streaming;
> > >
> > > return 0;
> > > }
> > > @@ -1818,7 +1818,7 @@ int V4L2VideoDevice::streamOff()
> > > {
> > > int ret;
> > >
> > > - if (!streaming_ && queuedBuffers_.empty())
> > > + if (state_ != State::Streaming && queuedBuffers_.empty())
> > > return 0;
> > >
> > > ret = ioctl(VIDIOC_STREAMOFF, &bufferType_);
> > > @@ -1828,6 +1828,8 @@ int V4L2VideoDevice::streamOff()
> > > return ret;
> > > }
> > >
> > > + state_ = State::Stopping;
> > > +
> >
> > Should this be before the call for VIDIOC_STREAMOFF?
> > It may not make a difference in practice, or it might cause
> > V4L2VideoDevice to reject a buffer (/request) that is queued in parallel
> > ... but I think we already know the request can be queued while calling
> > streamOff ... so I 'think' it's safe to change the state before calling
> > the ioctl...
>
> As the V4L2VideoDevice class isn't thread-safe, it will indeed make no
> difference. I'd rather keep it here, or the state will stay Stopping
> forever if VIDIOC_STREAMOFF fails.
That's a good enough reason to keep it where it is indeed ;-)
--
Kieran
>
> > Which ever way you're happier with, or consensus goes to:
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >