| Message ID | 20260618123844.656396-2-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Thu, Jun 18, 2026 at 02:38:18PM +0200, Barnabás Pőcze wrote: > Remove code duplication by calling `sinkRelease()` instead. To me, saving 2 lines of code by calling into a function whose name is unrelated to reusing a request, it's not worth it. Let's see other people opinion, but I would rather drop this patch. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/apps/cam/camera_session.cpp | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp > index 17444a217b..ce8ee354f0 100644 > --- a/src/apps/cam/camera_session.cpp > +++ b/src/apps/cam/camera_session.cpp > @@ -542,11 +542,8 @@ void CameraSession::processRequest(Request *request) > * If the frame sink holds on the request, we'll requeue it later in the > * complete handler. > */ > - if (!requeue) > - return; > - > - request->reuse(Request::ReuseBuffers); > - queueRequest(request); > + if (requeue) > + sinkRelease(request); > } > > void CameraSession::sinkRelease(Request *request) > -- > 2.54.0 >
2026. 06. 22. 14:16 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Thu, Jun 18, 2026 at 02:38:18PM +0200, Barnabás Pőcze wrote: >> Remove code duplication by calling `sinkRelease()` instead. > > To me, saving 2 lines of code by calling into a function whose name is > unrelated to reusing a request, it's not worth it. The two lines will become more: for (const auto &[stream, buffer] : request->buffers()) camera_->addBuffer(stream, buffer); request->reuse(); for (const auto &cfg : *config_) request->enableStream(cfg.stream(), true); queueRequest(request); that's why I want to avoid the duplication. > > Let's see other people opinion, but I would rather drop this patch. > >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/apps/cam/camera_session.cpp | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp >> index 17444a217b..ce8ee354f0 100644 >> --- a/src/apps/cam/camera_session.cpp >> +++ b/src/apps/cam/camera_session.cpp >> @@ -542,11 +542,8 @@ void CameraSession::processRequest(Request *request) >> * If the frame sink holds on the request, we'll requeue it later in the >> * complete handler. >> */ >> - if (!requeue) >> - return; >> - >> - request->reuse(Request::ReuseBuffers); >> - queueRequest(request); >> + if (requeue) >> + sinkRelease(request); >> } >> >> void CameraSession::sinkRelease(Request *request) >> -- >> 2.54.0 >>
On Mon, Jun 22, 2026 at 02:41:58PM +0200, Barnabás Pőcze wrote: > 2026. 06. 22. 14:16 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Thu, Jun 18, 2026 at 02:38:18PM +0200, Barnabás Pőcze wrote: > >> Remove code duplication by calling `sinkRelease()` instead. > > > > To me, saving 2 lines of code by calling into a function whose name is > > unrelated to reusing a request, it's not worth it. > > The two lines will become more: > > for (const auto &[stream, buffer] : request->buffers()) > camera_->addBuffer(stream, buffer); > > request->reuse(); > > for (const auto &cfg : *config_) > request->enableStream(cfg.stream(), true); > > queueRequest(request); > > that's why I want to avoid the duplication. > > > Let's see other people opinion, but I would rather drop this patch. I agree with both of you. With Jacopo about the function name, and with Barnabás about this patch being useful preparation. A simple middleground could be to rename sinkRelease() to requeueRequest(). > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> --- > >> src/apps/cam/camera_session.cpp | 7 ++----- > >> 1 file changed, 2 insertions(+), 5 deletions(-) > >> > >> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp > >> index 17444a217b..ce8ee354f0 100644 > >> --- a/src/apps/cam/camera_session.cpp > >> +++ b/src/apps/cam/camera_session.cpp > >> @@ -542,11 +542,8 @@ void CameraSession::processRequest(Request *request) > >> * If the frame sink holds on the request, we'll requeue it later in the > >> * complete handler. > >> */ > >> - if (!requeue) > >> - return; > >> - > >> - request->reuse(Request::ReuseBuffers); > >> - queueRequest(request); > >> + if (requeue) > >> + sinkRelease(request); > >> } > >> > >> void CameraSession::sinkRelease(Request *request)
2026. 06. 23. 0:40 keltezéssel, Laurent Pinchart írta: > On Mon, Jun 22, 2026 at 02:41:58PM +0200, Barnabás Pőcze wrote: >> 2026. 06. 22. 14:16 keltezéssel, Jacopo Mondi írta: >>> Hi Barnabás >>> >>> On Thu, Jun 18, 2026 at 02:38:18PM +0200, Barnabás Pőcze wrote: >>>> Remove code duplication by calling `sinkRelease()` instead. >>> >>> To me, saving 2 lines of code by calling into a function whose name is >>> unrelated to reusing a request, it's not worth it. >> >> The two lines will become more: >> >> for (const auto &[stream, buffer] : request->buffers()) >> camera_->addBuffer(stream, buffer); >> >> request->reuse(); >> >> for (const auto &cfg : *config_) >> request->enableStream(cfg.stream(), true); >> >> queueRequest(request); >> >> that's why I want to avoid the duplication. >> >>> Let's see other people opinion, but I would rather drop this patch. > > I agree with both of you. With Jacopo about the function name, and with > Barnabás about this patch being useful preparation. > > A simple middleground could be to rename sinkRelease() to > requeueRequest(). I have made the change. > >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> --- >>>> src/apps/cam/camera_session.cpp | 7 ++----- >>>> 1 file changed, 2 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp >>>> index 17444a217b..ce8ee354f0 100644 >>>> --- a/src/apps/cam/camera_session.cpp >>>> +++ b/src/apps/cam/camera_session.cpp >>>> @@ -542,11 +542,8 @@ void CameraSession::processRequest(Request *request) >>>> * If the frame sink holds on the request, we'll requeue it later in the >>>> * complete handler. >>>> */ >>>> - if (!requeue) >>>> - return; >>>> - >>>> - request->reuse(Request::ReuseBuffers); >>>> - queueRequest(request); >>>> + if (requeue) >>>> + sinkRelease(request); >>>> } >>>> >>>> void CameraSession::sinkRelease(Request *request) >
diff --git a/src/apps/cam/camera_session.cpp b/src/apps/cam/camera_session.cpp index 17444a217b..ce8ee354f0 100644 --- a/src/apps/cam/camera_session.cpp +++ b/src/apps/cam/camera_session.cpp @@ -542,11 +542,8 @@ void CameraSession::processRequest(Request *request) * If the frame sink holds on the request, we'll requeue it later in the * complete handler. */ - if (!requeue) - return; - - request->reuse(Request::ReuseBuffers); - queueRequest(request); + if (requeue) + sinkRelease(request); } void CameraSession::sinkRelease(Request *request)
Remove code duplication by calling `sinkRelease()` instead. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/apps/cam/camera_session.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)