| Message ID | 20250710144316.601640-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 1ef8981c3931f1df00e478b25d535306be1a2f6a |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, On Thu, 10 Jul 2025 at 15:43, Barnabás Pőcze <barnabas.pocze@ideasonboard.com> wrote: > > `SensorTimestamp` and `FrameWallClock` should always be available. However, > if that ever changes or they are not available for some unforeseen reason, > setting them to 0 is not ideal. That makes it more complicated for the > application to detect these cases (since they have to check the existence > either way), and if an application blindly assumes e.g. that `SensorTimestamp` > is monotonically increasing, then receiving a timestamp of 0 will likely > cause issues. > > So simply omit them from the request metadata if they are not available. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Looks good to me. Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index eafe94427..563df198e 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -1487,10 +1487,10 @@ void CameraData::checkRequestCompleted() > > void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request *request) > { > - request->metadata().set(controls::SensorTimestamp, > - bufferControls.get(controls::SensorTimestamp).value_or(0)); > - request->metadata().set(controls::FrameWallClock, > - bufferControls.get(controls::FrameWallClock).value_or(0)); > + if (auto x = bufferControls.get(controls::SensorTimestamp)) > + request->metadata().set(controls::SensorTimestamp, *x); > + if (auto x = bufferControls.get(controls::FrameWallClock)) > + request->metadata().set(controls::FrameWallClock, *x); > > if (cropParams_.size()) { > std::vector<Rectangle> crops; > -- > 2.50.0 >
Hi Barnabás, On 10/07/2025 15:43, Barnabás Pőcze wrote: > `SensorTimestamp` and `FrameWallClock` should always be available. However, > if that ever changes or they are not available for some unforeseen reason, > setting them to 0 is not ideal. That makes it more complicated for the > application to detect these cases (since they have to check the existence > either way), and if an application blindly assumes e.g. that `SensorTimestamp` > is monotonically increasing, then receiving a timestamp of 0 will likely > cause issues. > > So simply omit them from the request metadata if they are not available. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index eafe94427..563df198e 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -1487,10 +1487,10 @@ void CameraData::checkRequestCompleted() > > void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request *request) > { > - request->metadata().set(controls::SensorTimestamp, > - bufferControls.get(controls::SensorTimestamp).value_or(0)); > - request->metadata().set(controls::FrameWallClock, > - bufferControls.get(controls::FrameWallClock).value_or(0)); > + if (auto x = bufferControls.get(controls::SensorTimestamp)) > + request->metadata().set(controls::SensorTimestamp, *x); > + if (auto x = bufferControls.get(controls::FrameWallClock)) > + request->metadata().set(controls::FrameWallClock, *x); > > if (cropParams_.size()) { > std::vector<Rectangle> crops;
Hi Barnabás, Thank you for the patch. On Thu, Jul 10, 2025 at 04:43:16PM +0200, Barnabás Pőcze wrote: > `SensorTimestamp` and `FrameWallClock` should always be available. However, > if that ever changes or they are not available for some unforeseen reason, > setting them to 0 is not ideal. That makes it more complicated for the > application to detect these cases (since they have to check the existence > either way), and if an application blindly assumes e.g. that `SensorTimestamp` > is monotonically increasing, then receiving a timestamp of 0 will likely > cause issues. > > So simply omit them from the request metadata if they are not available. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > index eafe94427..563df198e 100644 > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp > @@ -1487,10 +1487,10 @@ void CameraData::checkRequestCompleted() > > void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request *request) > { > - request->metadata().set(controls::SensorTimestamp, > - bufferControls.get(controls::SensorTimestamp).value_or(0)); > - request->metadata().set(controls::FrameWallClock, > - bufferControls.get(controls::FrameWallClock).value_or(0)); > + if (auto x = bufferControls.get(controls::SensorTimestamp)) > + request->metadata().set(controls::SensorTimestamp, *x); Can it ever happen that we wouldn't have a timestamp, and if so, is there a way to fix that ? > + if (auto x = bufferControls.get(controls::FrameWallClock)) > + request->metadata().set(controls::FrameWallClock, *x); > > if (cropParams_.size()) { > std::vector<Rectangle> crops;
Hi 2025. 07. 15. 20:22 keltezéssel, Laurent Pinchart írta: > Hi Barnabás, > > Thank you for the patch. > > On Thu, Jul 10, 2025 at 04:43:16PM +0200, Barnabás Pőcze wrote: >> `SensorTimestamp` and `FrameWallClock` should always be available. However, >> if that ever changes or they are not available for some unforeseen reason, >> setting them to 0 is not ideal. That makes it more complicated for the >> application to detect these cases (since they have to check the existence >> either way), and if an application blindly assumes e.g. that `SensorTimestamp` >> is monotonically increasing, then receiving a timestamp of 0 will likely >> cause issues. >> >> So simply omit them from the request metadata if they are not available. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp >> index eafe94427..563df198e 100644 >> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp >> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp >> @@ -1487,10 +1487,10 @@ void CameraData::checkRequestCompleted() >> >> void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request *request) >> { >> - request->metadata().set(controls::SensorTimestamp, >> - bufferControls.get(controls::SensorTimestamp).value_or(0)); >> - request->metadata().set(controls::FrameWallClock, >> - bufferControls.get(controls::FrameWallClock).value_or(0)); >> + if (auto x = bufferControls.get(controls::SensorTimestamp)) >> + request->metadata().set(controls::SensorTimestamp, *x); > > Can it ever happen that we wouldn't have a timestamp, and if so, is > there a way to fix that ? At the moment I don't think so. But I believe one could go even further and store the two timestamps in the `CfeJob` / `BayerFrame` types. But that would be a bigger change, needing to reconcile the issue of having separate types but `CameraData::fillRequestMetadata()` being common. I can take a look at that if you want. I stopped here because I think it is still an improvement over the previous status quo. Regards, Barnabás Pőcze > >> + if (auto x = bufferControls.get(controls::FrameWallClock)) >> + request->metadata().set(controls::FrameWallClock, *x); >> >> if (cropParams_.size()) { >> std::vector<Rectangle> crops; >
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp index eafe94427..563df198e 100644 --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp @@ -1487,10 +1487,10 @@ void CameraData::checkRequestCompleted() void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request *request) { - request->metadata().set(controls::SensorTimestamp, - bufferControls.get(controls::SensorTimestamp).value_or(0)); - request->metadata().set(controls::FrameWallClock, - bufferControls.get(controls::FrameWallClock).value_or(0)); + if (auto x = bufferControls.get(controls::SensorTimestamp)) + request->metadata().set(controls::SensorTimestamp, *x); + if (auto x = bufferControls.get(controls::FrameWallClock)) + request->metadata().set(controls::FrameWallClock, *x); if (cropParams_.size()) { std::vector<Rectangle> crops;
`SensorTimestamp` and `FrameWallClock` should always be available. However, if that ever changes or they are not available for some unforeseen reason, setting them to 0 is not ideal. That makes it more complicated for the application to detect these cases (since they have to check the existence either way), and if an application blindly assumes e.g. that `SensorTimestamp` is monotonically increasing, then receiving a timestamp of 0 will likely cause issues. So simply omit them from the request metadata if they are not available. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/libcamera/pipeline/rpi/common/pipeline_base.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)