[PATCH] V4L2VideoDevice: Call FrameBuffer::Private::cancel() in streamOff()

[Kieran Bingham]

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 at 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 at 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
>