| Message ID | 20211206233948.1351206-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, On 12/7/21 5:09 AM, Kieran Bingham wrote: > The SensorSequence metadata provides the sequence number associated with > the image capture directly. > > While we already report sequence numbers in the metadata of the buffers, > this can lead to mis-leading parsing of those sequence numbers which may > be purely monotonic sequences from an ISP, and not represent the true > sequence counts from a capture device. > > Use the newly added SensorSequence metadata control and populate > completed requests with the sequence number from the appropriate stream. > For ISP based devices, such as the IPU3, RPi, and RkISP1, this comes > from the CSI2 receiver capture device, while for the Simple pipeline and > the UVC Video pipeline, this comes from the single video capture device. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +++- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 +++ > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +++- > src/libcamera/pipeline/simple/simple.cpp | 12 ++++++++---- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++ > 5 files changed, 19 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 313220624aff..a7f35840afbb 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -1372,13 +1372,15 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > } > > /* > - * Record the sensor's timestamp in the request metadata. > + * Record the sensor's timestamp and sequence in the request metadata. > * > * \todo The sensor timestamp should be better estimated by connecting > * to the V4L2Device::frameStart signal. > */ > request->metadata().set(controls::SensorTimestamp, > buffer->metadata().timestamp); > + request->metadata().set(controls::SensorSequence, > + buffer->metadata().sequence); I vaguely remember looking into consolidating SensorTimestamp with frameStart signal(turned out to be non-trivial), as mentioned in the \todo above. Does sequence number also seems a good candidate to be emitted from the signal, from that angle? If yes, I would add that in the \todo as well. For the patch Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence); > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 321b72adbbf7..f458587a8920 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1549,6 +1549,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > * as it does not receive the FrameBuffer object. > */ > ctrl.set(controls::SensorTimestamp, buffer->metadata().timestamp); > + ctrl.set(controls::SensorSequence, buffer->metadata().sequence); > bayerQueue_.push({ buffer, std::move(ctrl) }); > } else { > embeddedQueue_.push(buffer); > @@ -1786,6 +1787,8 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls, > { > request->metadata().set(controls::SensorTimestamp, > bufferControls.get(controls::SensorTimestamp)); > + request->metadata().set(controls::SensorSequence, > + bufferControls.get(controls::SensorSequence)); > > request->metadata().set(controls::ScalerCrop, scalerCrop_); > } > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 36ef6a02ae90..27592e473c14 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -1077,13 +1077,15 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) > Request *request = buffer->request(); > > /* > - * Record the sensor's timestamp in the request metadata. > + * Record the sensor's timestamp and sequence in the request metadata. > * > * \todo The sensor timestamp should be better estimated by connecting > * to the V4L2Device::frameStart signal. > */ > request->metadata().set(controls::SensorTimestamp, > buffer->metadata().timestamp); > + request->metadata().set(controls::SensorSequence, > + buffer->metadata().sequence); > > completeBuffer(request, buffer); > tryCompleteRequest(request); > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 701fb4be0b71..4c5d9c0b9919 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -667,9 +667,10 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) > } > > /* > - * Record the sensor's timestamp in the request metadata. The request > - * needs to be obtained from the user-facing buffer, as internal > - * buffers are free-wheeling and have no request associated with them. > + * Record the sensor's timestamp and sequence in the request metadata. > + * The request needs to be obtained from the user-facing buffer, as > + * internal buffers are free-wheeling and have no request associated > + * with them. > * > * \todo The sensor timestamp should be better estimated by connecting > * to the V4L2Device::frameStart signal if the platform provides it. > @@ -686,9 +687,12 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) > } > } > > - if (request) > + if (request) { > request->metadata().set(controls::SensorTimestamp, > buffer->metadata().timestamp); > + request->metadata().set(controls::SensorSequence, > + buffer->metadata().sequence); > + } > > /* > * Queue the captured and the request buffer to the converter if format > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 264f5370cf32..f5fab3b69e6d 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -666,6 +666,8 @@ void UVCCameraData::bufferReady(FrameBuffer *buffer) > /* \todo Use the UVC metadata to calculate a more precise timestamp */ > request->metadata().set(controls::SensorTimestamp, > buffer->metadata().timestamp); > + request->metadata().set(controls::SensorSequence, > + buffer->metadata().sequence); > > pipe()->completeBuffer(request, buffer); > pipe()->completeRequest(request);
Hi Kieran, Thank you for your patch. On Mon, 6 Dec 2021 at 23:39, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > The SensorSequence metadata provides the sequence number associated with > the image capture directly. > > While we already report sequence numbers in the metadata of the buffers, > this can lead to mis-leading parsing of those sequence numbers which may > be purely monotonic sequences from an ISP, and not represent the true > sequence counts from a capture device. > > Use the newly added SensorSequence metadata control and populate > completed requests with the sequence number from the appropriate stream. > For ISP based devices, such as the IPU3, RPi, and RkISP1, this comes > from the CSI2 receiver capture device, while for the Simple pipeline and > the UVC Video pipeline, this comes from the single video capture device. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +++- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 +++ > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +++- > src/libcamera/pipeline/simple/simple.cpp | 12 ++++++++---- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++ > 5 files changed, 19 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp > b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 313220624aff..a7f35840afbb 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -1372,13 +1372,15 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer > *buffer) > } > > /* > - * Record the sensor's timestamp in the request metadata. > + * Record the sensor's timestamp and sequence in the request > metadata. > * > * \todo The sensor timestamp should be better estimated by > connecting > * to the V4L2Device::frameStart signal. > */ > request->metadata().set(controls::SensorTimestamp, > buffer->metadata().timestamp); > + request->metadata().set(controls::SensorSequence, > + buffer->metadata().sequence); > > info->effectiveSensorControls = > delayedCtrls_->get(buffer->metadata().sequence); > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 321b72adbbf7..f458587a8920 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1549,6 +1549,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer > *buffer) > * as it does not receive the FrameBuffer object. > */ > ctrl.set(controls::SensorTimestamp, > buffer->metadata().timestamp); > + ctrl.set(controls::SensorSequence, > buffer->metadata().sequence); > bayerQueue_.push({ buffer, std::move(ctrl) }); > } else { > embeddedQueue_.push(buffer); > @@ -1786,6 +1787,8 @@ void RPiCameraData::fillRequestMetadata(const > ControlList &bufferControls, > { > request->metadata().set(controls::SensorTimestamp, > > bufferControls.get(controls::SensorTimestamp)); > + request->metadata().set(controls::SensorSequence, > + > bufferControls.get(controls::SensorSequence)); > > request->metadata().set(controls::ScalerCrop, scalerCrop_); > } > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 36ef6a02ae90..27592e473c14 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -1077,13 +1077,15 @@ void > PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) > Request *request = buffer->request(); > > /* > - * Record the sensor's timestamp in the request metadata. > + * Record the sensor's timestamp and sequence in the request > metadata. > * > * \todo The sensor timestamp should be better estimated by > connecting > * to the V4L2Device::frameStart signal. > */ > request->metadata().set(controls::SensorTimestamp, > buffer->metadata().timestamp); > + request->metadata().set(controls::SensorSequence, > + buffer->metadata().sequence); > > completeBuffer(request, buffer); > tryCompleteRequest(request); > diff --git a/src/libcamera/pipeline/simple/simple.cpp > b/src/libcamera/pipeline/simple/simple.cpp > index 701fb4be0b71..4c5d9c0b9919 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -667,9 +667,10 @@ void SimpleCameraData::bufferReady(FrameBuffer > *buffer) > } > > /* > - * Record the sensor's timestamp in the request metadata. The > request > - * needs to be obtained from the user-facing buffer, as internal > - * buffers are free-wheeling and have no request associated with > them. > + * Record the sensor's timestamp and sequence in the request > metadata. > + * The request needs to be obtained from the user-facing buffer, as > + * internal buffers are free-wheeling and have no request > associated > + * with them. > * > * \todo The sensor timestamp should be better estimated by > connecting > * to the V4L2Device::frameStart signal if the platform provides > it. > @@ -686,9 +687,12 @@ void SimpleCameraData::bufferReady(FrameBuffer > *buffer) > } > } > > - if (request) > + if (request) { > request->metadata().set(controls::SensorTimestamp, > buffer->metadata().timestamp); > + request->metadata().set(controls::SensorSequence, > + buffer->metadata().sequence); > + } > > /* > * Queue the captured and the request buffer to the converter if > format > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 264f5370cf32..f5fab3b69e6d 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -666,6 +666,8 @@ void UVCCameraData::bufferReady(FrameBuffer *buffer) > /* \todo Use the UVC metadata to calculate a more precise > timestamp */ > request->metadata().set(controls::SensorTimestamp, > buffer->metadata().timestamp); > + request->metadata().set(controls::SensorSequence, > + buffer->metadata().sequence); > > pipe()->completeBuffer(request, buffer); > pipe()->completeRequest(request); > -- > 2.30.2 > >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 313220624aff..a7f35840afbb 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -1372,13 +1372,15 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) } /* - * Record the sensor's timestamp in the request metadata. + * Record the sensor's timestamp and sequence in the request metadata. * * \todo The sensor timestamp should be better estimated by connecting * to the V4L2Device::frameStart signal. */ request->metadata().set(controls::SensorTimestamp, buffer->metadata().timestamp); + request->metadata().set(controls::SensorSequence, + buffer->metadata().sequence); info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence); diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 321b72adbbf7..f458587a8920 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1549,6 +1549,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) * as it does not receive the FrameBuffer object. */ ctrl.set(controls::SensorTimestamp, buffer->metadata().timestamp); + ctrl.set(controls::SensorSequence, buffer->metadata().sequence); bayerQueue_.push({ buffer, std::move(ctrl) }); } else { embeddedQueue_.push(buffer); @@ -1786,6 +1787,8 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls, { request->metadata().set(controls::SensorTimestamp, bufferControls.get(controls::SensorTimestamp)); + request->metadata().set(controls::SensorSequence, + bufferControls.get(controls::SensorSequence)); request->metadata().set(controls::ScalerCrop, scalerCrop_); } diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 36ef6a02ae90..27592e473c14 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -1077,13 +1077,15 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) Request *request = buffer->request(); /* - * Record the sensor's timestamp in the request metadata. + * Record the sensor's timestamp and sequence in the request metadata. * * \todo The sensor timestamp should be better estimated by connecting * to the V4L2Device::frameStart signal. */ request->metadata().set(controls::SensorTimestamp, buffer->metadata().timestamp); + request->metadata().set(controls::SensorSequence, + buffer->metadata().sequence); completeBuffer(request, buffer); tryCompleteRequest(request); diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 701fb4be0b71..4c5d9c0b9919 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -667,9 +667,10 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) } /* - * Record the sensor's timestamp in the request metadata. The request - * needs to be obtained from the user-facing buffer, as internal - * buffers are free-wheeling and have no request associated with them. + * Record the sensor's timestamp and sequence in the request metadata. + * The request needs to be obtained from the user-facing buffer, as + * internal buffers are free-wheeling and have no request associated + * with them. * * \todo The sensor timestamp should be better estimated by connecting * to the V4L2Device::frameStart signal if the platform provides it. @@ -686,9 +687,12 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer) } } - if (request) + if (request) { request->metadata().set(controls::SensorTimestamp, buffer->metadata().timestamp); + request->metadata().set(controls::SensorSequence, + buffer->metadata().sequence); + } /* * Queue the captured and the request buffer to the converter if format diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index 264f5370cf32..f5fab3b69e6d 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -666,6 +666,8 @@ void UVCCameraData::bufferReady(FrameBuffer *buffer) /* \todo Use the UVC metadata to calculate a more precise timestamp */ request->metadata().set(controls::SensorTimestamp, buffer->metadata().timestamp); + request->metadata().set(controls::SensorSequence, + buffer->metadata().sequence); pipe()->completeBuffer(request, buffer); pipe()->completeRequest(request);
The SensorSequence metadata provides the sequence number associated with the image capture directly. While we already report sequence numbers in the metadata of the buffers, this can lead to mis-leading parsing of those sequence numbers which may be purely monotonic sequences from an ISP, and not represent the true sequence counts from a capture device. Use the newly added SensorSequence metadata control and populate completed requests with the sequence number from the appropriate stream. For ISP based devices, such as the IPU3, RPi, and RkISP1, this comes from the CSI2 receiver capture device, while for the Simple pipeline and the UVC Video pipeline, this comes from the single video capture device. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 4 +++- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 3 +++ src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +++- src/libcamera/pipeline/simple/simple.cpp | 12 ++++++++---- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++ 5 files changed, 19 insertions(+), 6 deletions(-)