| Message ID | 20250626082235.1845316-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. On Thu, 26 Jun 2025 at 09:22, Barnabás Pőcze <barnabas.pocze@ideasonboard.com> wrote: > > Since 6cf9c4d34fbda5 ("pipeline: ipa: rpi: Split RPiCameraData::dropFrameCount_") > the initial n frames are not dropped anymore. So clearing the request metadata > should not be necessary anymore, remove it. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/libcamera/pipeline/rpi/pisp/pisp.cpp | 7 +------ > src/libcamera/pipeline/rpi/vc4/vc4.cpp | 7 +------ > 2 files changed, 2 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > index 2df91bacf..834b0e3f9 100644 > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > @@ -2313,16 +2313,11 @@ void PiSPCameraData::tryRunPipeline() > > /* Take the first request from the queue and action the IPA. */ > Request *request = requestQueue_.front(); > + ASSERT(request->metadata().empty()); I don't think the assert is needed here, a few lines above we check for requestQueue_.empty(). Ditto for the vc4.cpp change. Other than that, looks good! Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > /* See if a new ScalerCrop value needs to be applied. */ > applyScalerCrop(request->controls()); > > - /* > - * Clear the request metadata and fill it with some initial non-IPA > - * related controls. We clear it first because the request metadata > - * may have been populated if we have dropped the previous frame. > - */ > - request->metadata().clear(); > fillRequestMetadata(job.sensorControls, request); > > /* Set our state to say the pipeline is active. */ > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > index e99a7edf8..02e835b00 100644 > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > @@ -927,16 +927,11 @@ void Vc4CameraData::tryRunPipeline() > > /* Take the first request from the queue and action the IPA. */ > Request *request = requestQueue_.front(); > + ASSERT(request->metadata().empty()); > > /* See if a new ScalerCrop value needs to be applied. */ > applyScalerCrop(request->controls()); > > - /* > - * Clear the request metadata and fill it with some initial non-IPA > - * related controls. We clear it first because the request metadata > - * may have been populated if we have dropped the previous frame. > - */ > - request->metadata().clear(); > fillRequestMetadata(bayerFrame.controls, request); > > /* Set our state to say the pipeline is active. */ > -- > 2.50.0 >
Hi 2025. 06. 26. 10:47 keltezéssel, Naushir Patuck írta: > Hi Barnabás, > > Thank you for the patch. > > > On Thu, 26 Jun 2025 at 09:22, Barnabás Pőcze > <barnabas.pocze@ideasonboard.com> wrote: >> >> Since 6cf9c4d34fbda5 ("pipeline: ipa: rpi: Split RPiCameraData::dropFrameCount_") >> the initial n frames are not dropped anymore. So clearing the request metadata >> should not be necessary anymore, remove it. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/libcamera/pipeline/rpi/pisp/pisp.cpp | 7 +------ >> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 7 +------ >> 2 files changed, 2 insertions(+), 12 deletions(-) >> >> diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp >> index 2df91bacf..834b0e3f9 100644 >> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp >> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp >> @@ -2313,16 +2313,11 @@ void PiSPCameraData::tryRunPipeline() >> >> /* Take the first request from the queue and action the IPA. */ >> Request *request = requestQueue_.front(); >> + ASSERT(request->metadata().empty()); > > I don't think the assert is needed here, a few lines above we check > for requestQueue_.empty(). Ditto for the vc4.cpp change. Other than > that, looks good! Sorry, it's not clear to me why checking `requestQueue_.empty()` is important here? Can you clarify? Regards, Barnabás Pőcze > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > >> >> /* See if a new ScalerCrop value needs to be applied. */ >> applyScalerCrop(request->controls()); >> >> - /* >> - * Clear the request metadata and fill it with some initial non-IPA >> - * related controls. We clear it first because the request metadata >> - * may have been populated if we have dropped the previous frame. >> - */ >> - request->metadata().clear(); >> fillRequestMetadata(job.sensorControls, request); >> >> /* Set our state to say the pipeline is active. */ >> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp >> index e99a7edf8..02e835b00 100644 >> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp >> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp >> @@ -927,16 +927,11 @@ void Vc4CameraData::tryRunPipeline() >> >> /* Take the first request from the queue and action the IPA. */ >> Request *request = requestQueue_.front(); >> + ASSERT(request->metadata().empty()); >> >> /* See if a new ScalerCrop value needs to be applied. */ >> applyScalerCrop(request->controls()); >> >> - /* >> - * Clear the request metadata and fill it with some initial non-IPA >> - * related controls. We clear it first because the request metadata >> - * may have been populated if we have dropped the previous frame. >> - */ >> - request->metadata().clear(); >> fillRequestMetadata(bayerFrame.controls, request); >> >> /* Set our state to say the pipeline is active. */ >> -- >> 2.50.0 >>
Hi Barnabás, On Fri, 27 Jun 2025, 3:35 pm Barnabás Pőcze, < barnabas.pocze@ideasonboard.com> wrote: > Hi > > 2025. 06. 26. 10:47 keltezéssel, Naushir Patuck írta: > > Hi Barnabás, > > > > Thank you for the patch. > > > > > > On Thu, 26 Jun 2025 at 09:22, Barnabás Pőcze > > <barnabas.pocze@ideasonboard.com> wrote: > >> > >> Since 6cf9c4d34fbda5 ("pipeline: ipa: rpi: Split > RPiCameraData::dropFrameCount_") > >> the initial n frames are not dropped anymore. So clearing the request > metadata > >> should not be necessary anymore, remove it. > >> > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> --- > >> src/libcamera/pipeline/rpi/pisp/pisp.cpp | 7 +------ > >> src/libcamera/pipeline/rpi/vc4/vc4.cpp | 7 +------ > >> 2 files changed, 2 insertions(+), 12 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp > b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > >> index 2df91bacf..834b0e3f9 100644 > >> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp > >> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > >> @@ -2313,16 +2313,11 @@ void PiSPCameraData::tryRunPipeline() > >> > >> /* Take the first request from the queue and action the IPA. */ > >> Request *request = requestQueue_.front(); > >> + ASSERT(request->metadata().empty()); > > > > I don't think the assert is needed here, a few lines above we check > > for requestQueue_.empty(). Ditto for the vc4.cpp change. Other than > > that, looks good! > > Sorry, it's not clear to me why checking `requestQueue_.empty()` is > important here? Can you clarify? > Sorry my bad. I originally read that as test on the request queue, not request->metadata(). Naush > > > Regards, > Barnabás Pőcze > > > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > >> > >> /* See if a new ScalerCrop value needs to be applied. */ > >> applyScalerCrop(request->controls()); > >> > >> - /* > >> - * Clear the request metadata and fill it with some initial > non-IPA > >> - * related controls. We clear it first because the request > metadata > >> - * may have been populated if we have dropped the previous > frame. > >> - */ > >> - request->metadata().clear(); > >> fillRequestMetadata(job.sensorControls, request); > >> > >> /* Set our state to say the pipeline is active. */ > >> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > >> index e99a7edf8..02e835b00 100644 > >> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp > >> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp > >> @@ -927,16 +927,11 @@ void Vc4CameraData::tryRunPipeline() > >> > >> /* Take the first request from the queue and action the IPA. */ > >> Request *request = requestQueue_.front(); > >> + ASSERT(request->metadata().empty()); > >> > >> /* See if a new ScalerCrop value needs to be applied. */ > >> applyScalerCrop(request->controls()); > >> > >> - /* > >> - * Clear the request metadata and fill it with some initial > non-IPA > >> - * related controls. We clear it first because the request > metadata > >> - * may have been populated if we have dropped the previous > frame. > >> - */ > >> - request->metadata().clear(); > >> fillRequestMetadata(bayerFrame.controls, request); > >> > >> /* Set our state to say the pipeline is active. */ > >> -- > >> 2.50.0 > >> > >
diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp index 2df91bacf..834b0e3f9 100644 --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp @@ -2313,16 +2313,11 @@ void PiSPCameraData::tryRunPipeline() /* Take the first request from the queue and action the IPA. */ Request *request = requestQueue_.front(); + ASSERT(request->metadata().empty()); /* See if a new ScalerCrop value needs to be applied. */ applyScalerCrop(request->controls()); - /* - * Clear the request metadata and fill it with some initial non-IPA - * related controls. We clear it first because the request metadata - * may have been populated if we have dropped the previous frame. - */ - request->metadata().clear(); fillRequestMetadata(job.sensorControls, request); /* Set our state to say the pipeline is active. */ diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp index e99a7edf8..02e835b00 100644 --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp @@ -927,16 +927,11 @@ void Vc4CameraData::tryRunPipeline() /* Take the first request from the queue and action the IPA. */ Request *request = requestQueue_.front(); + ASSERT(request->metadata().empty()); /* See if a new ScalerCrop value needs to be applied. */ applyScalerCrop(request->controls()); - /* - * Clear the request metadata and fill it with some initial non-IPA - * related controls. We clear it first because the request metadata - * may have been populated if we have dropped the previous frame. - */ - request->metadata().clear(); fillRequestMetadata(bayerFrame.controls, request); /* Set our state to say the pipeline is active. */
Since 6cf9c4d34fbda5 ("pipeline: ipa: rpi: Split RPiCameraData::dropFrameCount_") the initial n frames are not dropped anymore. So clearing the request metadata should not be necessary anymore, remove it. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/libcamera/pipeline/rpi/pisp/pisp.cpp | 7 +------ src/libcamera/pipeline/rpi/vc4/vc4.cpp | 7 +------ 2 files changed, 2 insertions(+), 12 deletions(-)