Message ID | 20241203095217.2155153-1-chenghaoyang@chromium.org |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
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 >
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 > >
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); }
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(-)