| Message ID | 20211108131350.130665-22-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jean-Michel Hautbois (2021-11-08 13:13:49) > Now that we have the exposure time calculated, pass it to the > controls::ExposureTime and don't use the pipeline handler for it > anymore. While at it, use the same line duration value for ExposureTime > and FrameDuration. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/ipu3.cpp | 9 +++++++-- > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 -- > 2 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index ca3f2417..24c77be8 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -691,12 +691,17 @@ void IPAIPU3::parseStatistics(unsigned int frame, > > setControls(frame); > > + double lineDuration = sensorInfo_.lineLength > + / (sensorInfo_.pixelRate / 1e6); Is this something that should be calculated / cached at configure time? > + > /* \todo Use VBlank value calculated from each frame exposure. */ > - int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) / > - (sensorInfo_.pixelRate / 1e6); > + int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) > + * lineDuration; > ctrls.set(controls::FrameDuration, frameDuration); > > ctrls.set(controls::ColourTemperature, context_.frameContext->awb.temperatureK); new line here if you're keeping the int32_t calculation split. > + int32_t exposureTime = context_.frameContext->agc.exposure * lineDuration; > + ctrls.set(controls::ExposureTime, exposureTime); Otherwise, it could go in one statement, particularly if we take a local alias pointer for the frameContext as it is likely to be accessed so much: IPAFrameContext *frameContext = context_.frameContext; ctrls.set(controls::ExposureTime, frameContext->agc.exposure * lineDuration); > > /* > * \todo We should be able to add 'anything' (with a Control) in here to > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 5d87f6e5..97003681 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -1322,8 +1322,6 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > pipe()->completeBuffer(request, buffer); > > request->metadata().set(controls::draft::PipelineDepth, 3); > - /* \todo Move the ExposureTime control to the IPA. */ > - request->metadata().set(controls::ExposureTime, exposureTime_); Does the pipeline handler still need an exposureTime_ ? Can it be removed? or is something else using it? > /* \todo Actually apply the scaler crop region to the ImgU. */ > if (request->controls().contains(controls::ScalerCrop)) > cropRegion_ = request->controls().get(controls::ScalerCrop); > -- > 2.32.0 >
Hi Kieran, On 08/11/2021 17:23, Kieran Bingham wrote: > Quoting Jean-Michel Hautbois (2021-11-08 13:13:49) >> Now that we have the exposure time calculated, pass it to the >> controls::ExposureTime and don't use the pipeline handler for it >> anymore. While at it, use the same line duration value for ExposureTime >> and FrameDuration. >> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> --- >> src/ipa/ipu3/ipu3.cpp | 9 +++++++-- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 -- >> 2 files changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp >> index ca3f2417..24c77be8 100644 >> --- a/src/ipa/ipu3/ipu3.cpp >> +++ b/src/ipa/ipu3/ipu3.cpp >> @@ -691,12 +691,17 @@ void IPAIPU3::parseStatistics(unsigned int frame, >> >> setControls(frame); >> >> + double lineDuration = sensorInfo_.lineLength >> + / (sensorInfo_.pixelRate / 1e6); > > Is this something that should be calculated / cached at configure time? It can, indeed ! >> + >> /* \todo Use VBlank value calculated from each frame exposure. */ >> - int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) / >> - (sensorInfo_.pixelRate / 1e6); >> + int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) >> + * lineDuration; >> ctrls.set(controls::FrameDuration, frameDuration); >> >> ctrls.set(controls::ColourTemperature, context_.frameContext->awb.temperatureK); > > new line here if you're keeping the int32_t calculation split. > >> + int32_t exposureTime = context_.frameContext->agc.exposure * lineDuration; >> + ctrls.set(controls::ExposureTime, exposureTime); > > Otherwise, it could go in one statement, particularly if we take a local > alias pointer for the frameContext as it is likely to be accessed so much: > > IPAFrameContext *frameContext = context_.frameContext; > > ctrls.set(controls::ExposureTime, frameContext->agc.exposure * lineDuration); > Sure. >> >> /* >> * \todo We should be able to add 'anything' (with a Control) in here to >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index 5d87f6e5..97003681 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -1322,8 +1322,6 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) >> pipe()->completeBuffer(request, buffer); >> >> request->metadata().set(controls::draft::PipelineDepth, 3); >> - /* \todo Move the ExposureTime control to the IPA. */ >> - request->metadata().set(controls::ExposureTime, exposureTime_); > > Does the pipeline handler still need an exposureTime_ ? Can it be > removed? or is something else using it? Good catch. It is set in PipelineHandlerIPU3::updateControls() and never used anymore, so it should be removed from the pipeline handler now :-). > > >> /* \todo Actually apply the scaler crop region to the ImgU. */ >> if (request->controls().contains(controls::ScalerCrop)) >> cropRegion_ = request->controls().get(controls::ScalerCrop); >> -- >> 2.32.0 >>
diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index ca3f2417..24c77be8 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -691,12 +691,17 @@ void IPAIPU3::parseStatistics(unsigned int frame, setControls(frame); + double lineDuration = sensorInfo_.lineLength + / (sensorInfo_.pixelRate / 1e6); + /* \todo Use VBlank value calculated from each frame exposure. */ - int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) / - (sensorInfo_.pixelRate / 1e6); + int64_t frameDuration = (defVBlank_ + sensorInfo_.outputSize.height) + * lineDuration; ctrls.set(controls::FrameDuration, frameDuration); ctrls.set(controls::ColourTemperature, context_.frameContext->awb.temperatureK); + int32_t exposureTime = context_.frameContext->agc.exposure * lineDuration; + ctrls.set(controls::ExposureTime, exposureTime); /* * \todo We should be able to add 'anything' (with a Control) in here to diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 5d87f6e5..97003681 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -1322,8 +1322,6 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) pipe()->completeBuffer(request, buffer); request->metadata().set(controls::draft::PipelineDepth, 3); - /* \todo Move the ExposureTime control to the IPA. */ - request->metadata().set(controls::ExposureTime, exposureTime_); /* \todo Actually apply the scaler crop region to the ImgU. */ if (request->controls().contains(controls::ScalerCrop)) cropRegion_ = request->controls().get(controls::ScalerCrop);
Now that we have the exposure time calculated, pass it to the controls::ExposureTime and don't use the pipeline handler for it anymore. While at it, use the same line duration value for ExposureTime and FrameDuration. Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> --- src/ipa/ipu3/ipu3.cpp | 9 +++++++-- src/libcamera/pipeline/ipu3/ipu3.cpp | 2 -- 2 files changed, 7 insertions(+), 4 deletions(-)