| Message ID | 20260618123844.656396-5-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Thu, Jun 18, 2026 at 02:38:21PM +0200, Barnabás Pőcze wrote: > Match the behaviour of `completeBuffer()` by unsetting the "parent" `Request` > pointer of the `FrameBuffer`. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/libcamera/request.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index 163d8cf920..5f983e0ce6 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -143,6 +143,7 @@ void Request::Private::doCancelRequest() > for (FrameBuffer *buffer : pending_) { > buffer->_d()->cancel(); > camera_->bufferCompleted.emit(request, buffer); > + buffer->_d()->setRequest(nullptr); > } > > cancelled_ = true; > -- > 2.54.0 >
On Thu, Jun 18, 2026 at 02:38:21PM +0200, Barnabás Pőcze wrote: > Match the behaviour of `completeBuffer()` by unsetting the "parent" `Request` > pointer of the `FrameBuffer`. Should we extend unit tests to ensure FrameBuffer::request() returns null for all requests after Camera::stop() ? > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/request.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index 163d8cf920..5f983e0ce6 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -143,6 +143,7 @@ void Request::Private::doCancelRequest() > for (FrameBuffer *buffer : pending_) { > buffer->_d()->cancel(); > camera_->bufferCompleted.emit(request, buffer); > + buffer->_d()->setRequest(nullptr); > } > > cancelled_ = true;
2026. 06. 23. 0:49 keltezéssel, Laurent Pinchart írta: > On Thu, Jun 18, 2026 at 02:38:21PM +0200, Barnabás Pőcze wrote: >> Match the behaviour of `completeBuffer()` by unsetting the "parent" `Request` >> pointer of the `FrameBuffer`. > > Should we extend unit tests to ensure FrameBuffer::request() returns > null for all requests after Camera::stop() ? Possibly, I'm not sure, should I add it? > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- >> src/libcamera/request.cpp | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp >> index 163d8cf920..5f983e0ce6 100644 >> --- a/src/libcamera/request.cpp >> +++ b/src/libcamera/request.cpp >> @@ -143,6 +143,7 @@ void Request::Private::doCancelRequest() >> for (FrameBuffer *buffer : pending_) { >> buffer->_d()->cancel(); >> camera_->bufferCompleted.emit(request, buffer); >> + buffer->_d()->setRequest(nullptr); >> } >> >> cancelled_ = true; >
On Fri, Jun 26, 2026 at 09:25:33AM +0200, Barnabás Pőcze wrote: > 2026. 06. 23. 0:49 keltezéssel, Laurent Pinchart írta: > > On Thu, Jun 18, 2026 at 02:38:21PM +0200, Barnabás Pőcze wrote: > >> Match the behaviour of `completeBuffer()` by unsetting the "parent" `Request` > >> pointer of the `FrameBuffer`. > > > > Should we extend unit tests to ensure FrameBuffer::request() returns > > null for all requests after Camera::stop() ? > > Possibly, I'm not sure, should I add it? Yes please. If this is part of the API contract, it should be tested. If it's not part of the API contract, this patch isn't needed :-) > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> --- > >> src/libcamera/request.cpp | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > >> index 163d8cf920..5f983e0ce6 100644 > >> --- a/src/libcamera/request.cpp > >> +++ b/src/libcamera/request.cpp > >> @@ -143,6 +143,7 @@ void Request::Private::doCancelRequest() > >> for (FrameBuffer *buffer : pending_) { > >> buffer->_d()->cancel(); > >> camera_->bufferCompleted.emit(request, buffer); > >> + buffer->_d()->setRequest(nullptr); > >> } > >> > >> cancelled_ = true;
2026. 06. 26. 16:16 keltezéssel, Laurent Pinchart írta: > On Fri, Jun 26, 2026 at 09:25:33AM +0200, Barnabás Pőcze wrote: >> 2026. 06. 23. 0:49 keltezéssel, Laurent Pinchart írta: >>> On Thu, Jun 18, 2026 at 02:38:21PM +0200, Barnabás Pőcze wrote: >>>> Match the behaviour of `completeBuffer()` by unsetting the "parent" `Request` >>>> pointer of the `FrameBuffer`. >>> >>> Should we extend unit tests to ensure FrameBuffer::request() returns >>> null for all requests after Camera::stop() ? >> >> Possibly, I'm not sure, should I add it? > > Yes please. If this is part of the API contract, it should be tested. If > it's not part of the API contract, this patch isn't needed :-) Ahh, wait. The documentation does state "the association is valid until the buffer completes", so arguably a part of it. But I have just realized that the `request()` function is moved into the "Private" type by a later, not yet posted patch. I'm not seeing any test that accesses the private data with `->_d()`, so what's the opinion about that? > >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>>> --- >>>> src/libcamera/request.cpp | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp >>>> index 163d8cf920..5f983e0ce6 100644 >>>> --- a/src/libcamera/request.cpp >>>> +++ b/src/libcamera/request.cpp >>>> @@ -143,6 +143,7 @@ void Request::Private::doCancelRequest() >>>> for (FrameBuffer *buffer : pending_) { >>>> buffer->_d()->cancel(); >>>> camera_->bufferCompleted.emit(request, buffer); >>>> + buffer->_d()->setRequest(nullptr); >>>> } >>>> >>>> cancelled_ = true; >
diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index 163d8cf920..5f983e0ce6 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -143,6 +143,7 @@ void Request::Private::doCancelRequest() for (FrameBuffer *buffer : pending_) { buffer->_d()->cancel(); camera_->bufferCompleted.emit(request, buffer); + buffer->_d()->setRequest(nullptr); } cancelled_ = true;
Match the behaviour of `completeBuffer()` by unsetting the "parent" `Request` pointer of the `FrameBuffer`. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/libcamera/request.cpp | 1 + 1 file changed, 1 insertion(+)