V4L2VideoDevice: Call FrameBuffer::Private::cancel() in streamOff()
diff mbox series

Message ID 20241203095217.2155153-1-chenghaoyang@chromium.org
State New
Headers show
Series
  • V4L2VideoDevice: Call FrameBuffer::Private::cancel() in streamOff()
Related show

Commit Message

Cheng-Hao Yang Dec. 3, 2024, 9:52 a.m. UTC
At the moment `V4L2VideoDevice::streamOff()` sets
`FrameBuffer::Private`'s metadata directly, while that's equivalent to
calling `FrameBuffer::Private::cancel()`. To ease code tracing, this
patch replace the manual modification with the function call.

Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 src/libcamera/v4l2_videodevice.cpp | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Kieran Bingham Dec. 9, 2024, 3:01 p.m. UTC | #1
Quoting Harvey Yang (2024-12-03 09:52:13)
> At the moment `V4L2VideoDevice::streamOff()` sets
> `FrameBuffer::Private`'s metadata directly, while that's equivalent to
> calling `FrameBuffer::Private::cancel()`. To ease code tracing, this
> patch replace the manual modification with the function call.
> 
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  src/libcamera/v4l2_videodevice.cpp | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index a5cf67845..0558434cb 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -2007,10 +2007,9 @@ int V4L2VideoDevice::streamOff()
>         /* Send back all queued buffers. */
>         for (auto it : queuedBuffers_) {
>                 FrameBuffer *buffer = it.second;
> -               FrameMetadata &metadata = buffer->_d()->metadata();
> +               buffer->_d()->cancel();


buffer->_d()->cancel() indeed implements:

	void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }

so this looks good to me.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  
>                 cache_->put(it.first);
> -               metadata.status = FrameMetadata::FrameCancelled;

But you have changed "/where/" this is being done. Now the
metadata.status is being set /before/ cache_->put(it.first); which isn't
mentioned in the commit message. Is it an intentional change?

I believe it's fine though, but would have been note worthy to make sure
it was clear you had considered this and it's ok.

--
Kieran


>                 bufferReady.emit(buffer);
>         }
>  
> -- 
> 2.47.0.338.g60cca15819-goog
>
Cheng-Hao Yang Dec. 9, 2024, 4:12 p.m. UTC | #2
Hi Kieran,

On Mon, Dec 9, 2024 at 11:01 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Harvey Yang (2024-12-03 09:52:13)
> > At the moment `V4L2VideoDevice::streamOff()` sets
> > `FrameBuffer::Private`'s metadata directly, while that's equivalent to
> > calling `FrameBuffer::Private::cancel()`. To ease code tracing, this
> > patch replace the manual modification with the function call.
> >
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  src/libcamera/v4l2_videodevice.cpp | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index a5cf67845..0558434cb 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -2007,10 +2007,9 @@ int V4L2VideoDevice::streamOff()
> >         /* Send back all queued buffers. */
> >         for (auto it : queuedBuffers_) {
> >                 FrameBuffer *buffer = it.second;
> > -               FrameMetadata &metadata = buffer->_d()->metadata();
> > +               buffer->_d()->cancel();
>
>
> buffer->_d()->cancel() indeed implements:
>
>         void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }
>
> so this looks good to me.
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> >
> >                 cache_->put(it.first);
> > -               metadata.status = FrameMetadata::FrameCancelled;
>
> But you have changed "/where/" this is being done. Now the
> metadata.status is being set /before/ cache_->put(it.first); which isn't
> mentioned in the commit message. Is it an intentional change?
>
> I believe it's fine though, but would have been note worthy to make sure
> it was clear you had considered this and it's ok.

Right, I should set status after `cache_->put(it.first);` to avoid
unnecessary changes. Let me upload another version.

BR,
Harvey

>
> --
> Kieran
>
>
> >                 bufferReady.emit(buffer);
> >         }
> >
> > --
> > 2.47.0.338.g60cca15819-goog
> >

Patch
diff mbox series

diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index a5cf67845..0558434cb 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -2007,10 +2007,9 @@  int V4L2VideoDevice::streamOff()
 	/* Send back all queued buffers. */
 	for (auto it : queuedBuffers_) {
 		FrameBuffer *buffer = it.second;
-		FrameMetadata &metadata = buffer->_d()->metadata();
+		buffer->_d()->cancel();
 
 		cache_->put(it.first);
-		metadata.status = FrameMetadata::FrameCancelled;
 		bufferReady.emit(buffer);
 	}