| Message ID | 20210419131433.22920-11-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for your work. On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi <jacopo@jmondi.org> wrote: > Report the sensor's timestamp in the Request metadata by using the > Unicam::Image buffer timestamp as an initial approximation. > > The buffer's timestamp is recorded at DMA-transfer time, and it does not > theoretically matches the 'start of exposure' definition, but when used > to compare two consecutive frames it gives an acceptable estimation of > the sensor frame period duration. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index d1902bfc3393..7dc92acf3c4f 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1415,6 +1415,18 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer > *buffer) > << ", timestamp: " << buffer->metadata().timestamp; > > if (stream == &unicam_[Unicam::Image]) { > + /* > + * Record the sensor timestamp in the Request. > + * > + * \todo The sensor timestamp should be better estimated by > + * sampling the V4L2Device::frameStart signal. > + */ > Just a minor correction to this comment - the VB2 buffer timestamp that is provided by the device driver is actually sampled at the CSI2 frame start event, so is pretty much as close to the start of exposure as we can get without dead reckoning. This sample is also before the DMA to memory and V4L2Device::frameStart signal. Other than that, Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > + Request *request = requestQueue_.front(); > + ASSERT(request); > + > + request->metadata().set(controls::SensorTimestamp, > + buffer->metadata().timestamp); > + > /* > * Lookup the sensor controls used for this frame sequence > from > * DelayedControl and queue them along with the frame > buffer. > -- > 2.31.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Naush, thanks for review! On Mon, Apr 19, 2021 at 02:45:29PM +0100, Naushir Patuck wrote: > Hi Jacopo, > > Thank you for your work. > > On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > Report the sensor's timestamp in the Request metadata by using the > > Unicam::Image buffer timestamp as an initial approximation. > > > > The buffer's timestamp is recorded at DMA-transfer time, and it does not > > theoretically matches the 'start of exposure' definition, but when used > > to compare two consecutive frames it gives an acceptable estimation of > > the sensor frame period duration. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index d1902bfc3393..7dc92acf3c4f 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1415,6 +1415,18 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer > > *buffer) > > << ", timestamp: " << buffer->metadata().timestamp; > > > > if (stream == &unicam_[Unicam::Image]) { > > + /* > > + * Record the sensor timestamp in the Request. > > + * > > + * \todo The sensor timestamp should be better estimated by > > + * sampling the V4L2Device::frameStart signal. > > + */ > > > > Just a minor correction to this comment - the VB2 buffer timestamp that > is provided by the device driver is actually sampled at the CSI2 frame start > event, so is pretty much as close to the start of exposure as we can get > without dead reckoning. This sample is also before the DMA to memory > and V4L2Device::frameStart signal. Ah you see! That's great, I wrongly assumed the raw buffer timestamp was sampled at DMA transfer time end, as it happens on IPU3. It's great if I could drop that todo item! > > Other than that, > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > > + Request *request = requestQueue_.front(); > > + ASSERT(request); > > + > > + request->metadata().set(controls::SensorTimestamp, > > + buffer->metadata().timestamp); > > + > > /* > > * Lookup the sensor controls used for this frame sequence > > from > > * DelayedControl and queue them along with the frame > > buffer. > > -- > > 2.31.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > >
Hi Jacopo, Thank you for the patch. On Mon, Apr 19, 2021 at 03:55:39PM +0200, Jacopo Mondi wrote: > On Mon, Apr 19, 2021 at 02:45:29PM +0100, Naushir Patuck wrote: > > On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > Report the sensor's timestamp in the Request metadata by using the > > > Unicam::Image buffer timestamp as an initial approximation. > > > > > > The buffer's timestamp is recorded at DMA-transfer time, and it does not > > > theoretically matches the 'start of exposure' definition, but when used > > > to compare two consecutive frames it gives an acceptable estimation of > > > the sensor frame period duration. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index d1902bfc3393..7dc92acf3c4f 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -1415,6 +1415,18 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > << ", timestamp: " << buffer->metadata().timestamp; > > > > > > if (stream == &unicam_[Unicam::Image]) { > > > + /* > > > + * Record the sensor timestamp in the Request. > > > + * > > > + * \todo The sensor timestamp should be better estimated by > > > + * sampling the V4L2Device::frameStart signal. > > > + */ > > > > Just a minor correction to this comment - the VB2 buffer timestamp that > > is provided by the device driver is actually sampled at the CSI2 frame start > > event, so is pretty much as close to the start of exposure as we can get > > without dead reckoning. This sample is also before the DMA to memory > > and V4L2Device::frameStart signal. I wish more devices and drivers would do so :-) > Ah you see! That's great, I wrongly assumed the raw buffer timestamp > was sampled at DMA transfer time end, as it happens on IPU3. It's > great if I could drop that todo item! > > > Other than that, > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > > + Request *request = requestQueue_.front(); Don't we queue multiple buffers to Unicam ? Can't a buffer complete here while the ISP is busy processing the previous frame, wouldn't requestQueue_.front() then correspond to the frame processed by the ISP ? Maybe you could use buffer->request() instead (assuming the RPi pipeline handler calls setRequest()). > > > + ASSERT(request); > > > + > > > + request->metadata().set(controls::SensorTimestamp, > > > + buffer->metadata().timestamp); > > > + > > > /* > > > * Lookup the sensor controls used for this frame sequence from > > > * DelayedControl and queue them along with the frame buffer.
Hi Laurent, + Naush On Wed, Apr 21, 2021 at 12:58:21AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Mon, Apr 19, 2021 at 03:55:39PM +0200, Jacopo Mondi wrote: > > On Mon, Apr 19, 2021 at 02:45:29PM +0100, Naushir Patuck wrote: > > > On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Report the sensor's timestamp in the Request metadata by using the > > > > Unicam::Image buffer timestamp as an initial approximation. > > > > > > > > The buffer's timestamp is recorded at DMA-transfer time, and it does not > > > > theoretically matches the 'start of exposure' definition, but when used > > > > to compare two consecutive frames it gives an acceptable estimation of > > > > the sensor frame period duration. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index d1902bfc3393..7dc92acf3c4f 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -1415,6 +1415,18 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > > << ", timestamp: " << buffer->metadata().timestamp; > > > > > > > > if (stream == &unicam_[Unicam::Image]) { > > > > + /* > > > > + * Record the sensor timestamp in the Request. > > > > + * > > > > + * \todo The sensor timestamp should be better estimated by > > > > + * sampling the V4L2Device::frameStart signal. > > > > + */ > > > > > > Just a minor correction to this comment - the VB2 buffer timestamp that > > > is provided by the device driver is actually sampled at the CSI2 frame start > > > event, so is pretty much as close to the start of exposure as we can get > > > without dead reckoning. This sample is also before the DMA to memory > > > and V4L2Device::frameStart signal. > > I wish more devices and drivers would do so :-) > > > Ah you see! That's great, I wrongly assumed the raw buffer timestamp > > was sampled at DMA transfer time end, as it happens on IPU3. It's > > great if I could drop that todo item! > > > > > Other than that, > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > + Request *request = requestQueue_.front(); > > Don't we queue multiple buffers to Unicam ? Can't a buffer complete here > while the ISP is busy processing the previous frame, wouldn't > requestQueue_.front() then correspond to the frame processed by the ISP > ? Maybe you could use buffer->request() instead (assuming the RPi > pipeline handler calls setRequest()). > This has required me quite some mumbling, as I was a bit confused too on how better associate a buffer with a request in this pipeline. So, RPi does not use anything similar to FrameInfo, which I think we should bring to the core framework level soon, and have ph opt-in. It does not call setRequest() on buffers, and I failed to identify where to do so, given the ph design. The rest of the ph code when it has to retrieve the Request * goes and assume it's the first one (ie the oldest one) it's the one to chose. I have been wondering too if that's correct. Maybe I got lost into something trivial, Naush do you have opinions to share ? Thanks j > > > > + ASSERT(request); > > > > + > > > > + request->metadata().set(controls::SensorTimestamp, > > > > + buffer->metadata().timestamp); > > > > + > > > > /* > > > > * Lookup the sensor controls used for this frame sequence from > > > > * DelayedControl and queue them along with the frame buffer. > > -- > Regards, > > Laurent Pinchart
Hi Jacopo and Laurent, On Wed, 21 Apr 2021 at 08:49, Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Laurent, > + Naush > > On Wed, Apr 21, 2021 at 12:58:21AM +0300, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Mon, Apr 19, 2021 at 03:55:39PM +0200, Jacopo Mondi wrote: > > > On Mon, Apr 19, 2021 at 02:45:29PM +0100, Naushir Patuck wrote: > > > > On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi <jacopo@jmondi.org> > wrote: > > > > > Report the sensor's timestamp in the Request metadata by using the > > > > > Unicam::Image buffer timestamp as an initial approximation. > > > > > > > > > > The buffer's timestamp is recorded at DMA-transfer time, and it > does not > > > > > theoretically matches the 'start of exposure' definition, but when > used > > > > > to compare two consecutive frames it gives an acceptable > estimation of > > > > > the sensor frame period duration. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > --- > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 > ++++++++++++ > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > index d1902bfc3393..7dc92acf3c4f 100644 > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > @@ -1415,6 +1415,18 @@ void > RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > > > << ", timestamp: " << > buffer->metadata().timestamp; > > > > > > > > > > if (stream == &unicam_[Unicam::Image]) { > > > > > + /* > > > > > + * Record the sensor timestamp in the Request. > > > > > + * > > > > > + * \todo The sensor timestamp should be better > estimated by > > > > > + * sampling the V4L2Device::frameStart signal. > > > > > + */ > > > > > > > > Just a minor correction to this comment - the VB2 buffer timestamp > that > > > > is provided by the device driver is actually sampled at the CSI2 > frame start > > > > event, so is pretty much as close to the start of exposure as we can > get > > > > without dead reckoning. This sample is also before the DMA to memory > > > > and V4L2Device::frameStart signal. > > > > I wish more devices and drivers would do so :-) > > > > > Ah you see! That's great, I wrongly assumed the raw buffer timestamp > > > was sampled at DMA transfer time end, as it happens on IPU3. It's > > > great if I could drop that todo item! > > > > > > > Other than that, > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > > > + Request *request = requestQueue_.front(); > > > > Don't we queue multiple buffers to Unicam ? Can't a buffer complete here > > while the ISP is busy processing the previous frame, wouldn't > > requestQueue_.front() then correspond to the frame processed by the ISP > > ? Maybe you could use buffer->request() instead (assuming the RPi > > pipeline handler calls setRequest()). > > > Quite right, because of the queueing, there is a possibility that the top of the request queue may lag behind and will could return out a wrong timestamp with the above. > > This has required me quite some mumbling, as I was a bit confused too > on how better associate a buffer with a request in this pipeline. > > So, RPi does not use anything similar to FrameInfo, which I think we > should bring to the core framework level soon, and have ph opt-in. > It does not call setRequest() on buffers, and I failed to identify > where to do so, given the ph design. The rest of the ph code when > it has to retrieve the Request * goes and assume it's the first one > (ie the oldest one) it's the one to chose. I have been wondering too > if that's correct. Maybe I got lost into something trivial, Naush do > you have opinions to share ? > We don't use FrameInfo like the other pipeline handlers, but we do have a similar mechanism to store frame controls with a buffer. Coincidentally, I do have a patch that stores the timestamp for my IPA rate-control series [1]. This almost does what we need, the only thing missing is some code to transfer the timestamp from this ControlList to the request metadata. To get this change, we can do one of the following: 1) I can backport part of the change in [1] that we care about here, and add code to transfer to the request metadata. I can either provide a patch to add to this series or provide it on top of this series. 2) I can make this change in my patch series fairly easily as well. Given this seems to be related to Android, I doubt anybody would miss this feature from the Raspberry Pi platform until that series is merged. Let me know what you think. Regards, Naush [1] https://patchwork.libcamera.org/patch/12006/ > > Thanks > j > > > > > > + ASSERT(request); > > > > > + > > > > > + request->metadata().set(controls::SensorTimestamp, > > > > > + > buffer->metadata().timestamp); > > > > > + > > > > > /* > > > > > * Lookup the sensor controls used for this frame > sequence from > > > > > * DelayedControl and queue them along with the > frame buffer. > > > > -- > > Regards, > > > > Laurent Pinchart >
Hello, On Wed, Apr 21, 2021 at 10:14:57AM +0100, Naushir Patuck wrote: > On Wed, 21 Apr 2021 at 08:49, Jacopo Mondi wrote: > > On Wed, Apr 21, 2021 at 12:58:21AM +0300, Laurent Pinchart wrote: > > > On Mon, Apr 19, 2021 at 03:55:39PM +0200, Jacopo Mondi wrote: > > > > On Mon, Apr 19, 2021 at 02:45:29PM +0100, Naushir Patuck wrote: > > > > > On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi wrote: > > > > > > Report the sensor's timestamp in the Request metadata by using the > > > > > > Unicam::Image buffer timestamp as an initial approximation. > > > > > > > > > > > > The buffer's timestamp is recorded at DMA-transfer time, and it does not > > > > > > theoretically matches the 'start of exposure' definition, but when used > > > > > > to compare two consecutive frames it gives an acceptable estimation of > > > > > > the sensor frame period duration. > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > --- > > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++++ > > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > index d1902bfc3393..7dc92acf3c4f 100644 > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > @@ -1415,6 +1415,18 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > > > > << ", timestamp: " << buffer->metadata().timestamp; > > > > > > > > > > > > if (stream == &unicam_[Unicam::Image]) { > > > > > > + /* > > > > > > + * Record the sensor timestamp in the Request. > > > > > > + * > > > > > > + * \todo The sensor timestamp should be better estimated by > > > > > > + * sampling the V4L2Device::frameStart signal. > > > > > > + */ > > > > > > > > > > Just a minor correction to this comment - the VB2 buffer timestamp that > > > > > is provided by the device driver is actually sampled at the CSI2 frame start > > > > > event, so is pretty much as close to the start of exposure as we can get > > > > > without dead reckoning. This sample is also before the DMA to memory > > > > > and V4L2Device::frameStart signal. > > > > > > I wish more devices and drivers would do so :-) > > > > > > > Ah you see! That's great, I wrongly assumed the raw buffer timestamp > > > > was sampled at DMA transfer time end, as it happens on IPU3. It's > > > > great if I could drop that todo item! > > > > > > > > > Other than that, > > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > > > > > + Request *request = requestQueue_.front(); > > > > > > Don't we queue multiple buffers to Unicam ? Can't a buffer complete here > > > while the ISP is busy processing the previous frame, wouldn't > > > requestQueue_.front() then correspond to the frame processed by the ISP > > > ? Maybe you could use buffer->request() instead (assuming the RPi > > > pipeline handler calls setRequest()). > > Quite right, because of the queueing, there is a possibility that the top of > the request queue may lag behind and will could return out a wrong > timestamp with the above. > > > This has required me quite some mumbling, as I was a bit confused too > > on how better associate a buffer with a request in this pipeline. > > > > So, RPi does not use anything similar to FrameInfo, which I think we > > should bring to the core framework level soon, and have ph opt-in. > > It does not call setRequest() on buffers, and I failed to identify > > where to do so, given the ph design. The rest of the ph code when > > it has to retrieve the Request * goes and assume it's the first one > > (ie the oldest one) it's the one to chose. I have been wondering too > > if that's correct. Maybe I got lost into something trivial, Naush do > > you have opinions to share ? > > We don't use FrameInfo like the other pipeline handlers, but we do have > a similar mechanism to store frame controls with a buffer. > > Coincidentally, I do have a patch that stores the timestamp for my IPA > rate-control series [1]. This almost does what we need, the only thing > missing is some code to transfer the timestamp from this ControlList > to the request metadata. > > To get this change, we can do one of the following: > > 1) I can backport part of the change in [1] that we care about here, and > add code to transfer to the request metadata. I can either provide a patch > to add to this series or provide it on top of this series. > > 2) I can make this change in my patch series fairly easily as well. Given this > seems to be related to Android, I doubt anybody would miss this feature > from the Raspberry Pi platform until that series is merged. The timestamp metadata isn't limited to Android support, but nobody else uses it now, so the risk of breakage is fairly limited. Jacopo's series is nearly ready to get merged so I would avoid delaying it if possible. Jacopo, how about keeping this patch as-is with a \todo comment to tell that the front of the queue may not be the right buffer, and fixing it on top once both this series and Naush's series get merged ? Naush, would you be able to then submit a patch on top of the combination of both ? I'll review v5 of the rate limitation now. > Let me know what you think. > > [1] https://patchwork.libcamera.org/patch/12006/ > > > > > > > + ASSERT(request); > > > > > > + > > > > > > + request->metadata().set(controls::SensorTimestamp, > > > > > > + buffer->metadata().timestamp); > > > > > > + > > > > > > /* > > > > > > * Lookup the sensor controls used for this frame sequence from > > > > > > * DelayedControl and queue them along with the frame buffer.
On Wed, 21 Apr 2021 at 10:44, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hello, > > On Wed, Apr 21, 2021 at 10:14:57AM +0100, Naushir Patuck wrote: > > On Wed, 21 Apr 2021 at 08:49, Jacopo Mondi wrote: > > > On Wed, Apr 21, 2021 at 12:58:21AM +0300, Laurent Pinchart wrote: > > > > On Mon, Apr 19, 2021 at 03:55:39PM +0200, Jacopo Mondi wrote: > > > > > On Mon, Apr 19, 2021 at 02:45:29PM +0100, Naushir Patuck wrote: > > > > > > On Mon, 19 Apr 2021 at 14:14, Jacopo Mondi wrote: > > > > > > > Report the sensor's timestamp in the Request metadata by using > the > > > > > > > Unicam::Image buffer timestamp as an initial approximation. > > > > > > > > > > > > > > The buffer's timestamp is recorded at DMA-transfer time, and > it does not > > > > > > > theoretically matches the 'start of exposure' definition, but > when used > > > > > > > to compare two consecutive frames it gives an acceptable > estimation of > > > > > > > the sensor frame period duration. > > > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > --- > > > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 > ++++++++++++ > > > > > > > 1 file changed, 12 insertions(+) > > > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > > index d1902bfc3393..7dc92acf3c4f 100644 > > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > > > > @@ -1415,6 +1415,18 @@ void > RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > > > > > << ", timestamp: " << > buffer->metadata().timestamp; > > > > > > > > > > > > > > if (stream == &unicam_[Unicam::Image]) { > > > > > > > + /* > > > > > > > + * Record the sensor timestamp in the Request. > > > > > > > + * > > > > > > > + * \todo The sensor timestamp should be better > estimated by > > > > > > > + * sampling the V4L2Device::frameStart signal. > > > > > > > + */ > > > > > > > > > > > > Just a minor correction to this comment - the VB2 buffer > timestamp that > > > > > > is provided by the device driver is actually sampled at the CSI2 > frame start > > > > > > event, so is pretty much as close to the start of exposure as we > can get > > > > > > without dead reckoning. This sample is also before the DMA to > memory > > > > > > and V4L2Device::frameStart signal. > > > > > > > > I wish more devices and drivers would do so :-) > > > > > > > > > Ah you see! That's great, I wrongly assumed the raw buffer > timestamp > > > > > was sampled at DMA transfer time end, as it happens on IPU3. It's > > > > > great if I could drop that todo item! > > > > > > > > > > > Other than that, > > > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > > > > > > > + Request *request = requestQueue_.front(); > > > > > > > > Don't we queue multiple buffers to Unicam ? Can't a buffer complete > here > > > > while the ISP is busy processing the previous frame, wouldn't > > > > requestQueue_.front() then correspond to the frame processed by the > ISP > > > > ? Maybe you could use buffer->request() instead (assuming the RPi > > > > pipeline handler calls setRequest()). > > > > Quite right, because of the queueing, there is a possibility that the > top of > > the request queue may lag behind and will could return out a wrong > > timestamp with the above. > > > > > This has required me quite some mumbling, as I was a bit confused too > > > on how better associate a buffer with a request in this pipeline. > > > > > > So, RPi does not use anything similar to FrameInfo, which I think we > > > should bring to the core framework level soon, and have ph opt-in. > > > It does not call setRequest() on buffers, and I failed to identify > > > where to do so, given the ph design. The rest of the ph code when > > > it has to retrieve the Request * goes and assume it's the first one > > > (ie the oldest one) it's the one to chose. I have been wondering too > > > if that's correct. Maybe I got lost into something trivial, Naush do > > > you have opinions to share ? > > > > We don't use FrameInfo like the other pipeline handlers, but we do have > > a similar mechanism to store frame controls with a buffer. > > > > Coincidentally, I do have a patch that stores the timestamp for my IPA > > rate-control series [1]. This almost does what we need, the only thing > > missing is some code to transfer the timestamp from this ControlList > > to the request metadata. > > > > To get this change, we can do one of the following: > > > > 1) I can backport part of the change in [1] that we care about here, and > > add code to transfer to the request metadata. I can either provide a > patch > > to add to this series or provide it on top of this series. > > > > 2) I can make this change in my patch series fairly easily as well. > Given this > > seems to be related to Android, I doubt anybody would miss this feature > > from the Raspberry Pi platform until that series is merged. > > The timestamp metadata isn't limited to Android support, but nobody else > uses it now, so the risk of breakage is fairly limited. Jacopo's series > is nearly ready to get merged so I would avoid delaying it if possible. > Jacopo, how about keeping this patch as-is with a \todo comment to tell > that the front of the queue may not be the right buffer, and fixing it > on top once both this series and Naush's series get merged ? Naush, > would you be able to then submit a patch on top of the combination of > both ? I'll review v5 of the rate limitation now. > Sounds good, we fix this up once Jacopo's series and my rate-control have been merged. Note that my series is rebased on top of David's work at: https://patchwork.libcamera.org/patch/11730/) so I will also have to wait for that to be submitted. Regards, Naush > > > Let me know what you think. > > > > [1] https://patchwork.libcamera.org/patch/12006/ > > > > > > > > > + ASSERT(request); > > > > > > > + > > > > > > > + > request->metadata().set(controls::SensorTimestamp, > > > > > > > + > buffer->metadata().timestamp); > > > > > > > + > > > > > > > /* > > > > > > > * Lookup the sensor controls used for this > frame sequence from > > > > > > > * DelayedControl and queue them along with > the frame buffer. > > -- > Regards, > > Laurent Pinchart >
Hello On Wed, Apr 21, 2021 at 11:19:13AM +0100, Naushir Patuck wrote: [snip] > > > > > > > > > > > > > > > + Request *request = requestQueue_.front(); > > > > > > > > > > Don't we queue multiple buffers to Unicam ? Can't a buffer complete > > here > > > > > while the ISP is busy processing the previous frame, wouldn't > > > > > requestQueue_.front() then correspond to the frame processed by the > > ISP > > > > > ? Maybe you could use buffer->request() instead (assuming the RPi > > > > > pipeline handler calls setRequest()). > > > > > > Quite right, because of the queueing, there is a possibility that the > > top of > > > the request queue may lag behind and will could return out a wrong > > > timestamp with the above. > > > > > > > This has required me quite some mumbling, as I was a bit confused too > > > > on how better associate a buffer with a request in this pipeline. > > > > > > > > So, RPi does not use anything similar to FrameInfo, which I think we > > > > should bring to the core framework level soon, and have ph opt-in. > > > > It does not call setRequest() on buffers, and I failed to identify > > > > where to do so, given the ph design. The rest of the ph code when > > > > it has to retrieve the Request * goes and assume it's the first one > > > > (ie the oldest one) it's the one to chose. I have been wondering too > > > > if that's correct. Maybe I got lost into something trivial, Naush do > > > > you have opinions to share ? > > > > > > We don't use FrameInfo like the other pipeline handlers, but we do have > > > a similar mechanism to store frame controls with a buffer. > > > > > > Coincidentally, I do have a patch that stores the timestamp for my IPA > > > rate-control series [1]. This almost does what we need, the only thing > > > missing is some code to transfer the timestamp from this ControlList > > > to the request metadata. > > > > > > To get this change, we can do one of the following: > > > > > > 1) I can backport part of the change in [1] that we care about here, and > > > add code to transfer to the request metadata. I can either provide a > > patch > > > to add to this series or provide it on top of this series. > > > > > > 2) I can make this change in my patch series fairly easily as well. > > Given this > > > seems to be related to Android, I doubt anybody would miss this feature > > > from the Raspberry Pi platform until that series is merged. > > > > The timestamp metadata isn't limited to Android support, but nobody else > > uses it now, so the risk of breakage is fairly limited. Jacopo's series > > is nearly ready to get merged so I would avoid delaying it if possible. > > Jacopo, how about keeping this patch as-is with a \todo comment to tell > > that the front of the queue may not be the right buffer, and fixing it > > on top once both this series and Naush's series get merged ? Naush, > > would you be able to then submit a patch on top of the combination of > > both ? I'll review v5 of the rate limitation now. > > > > Sounds good, we fix this up once Jacopo's series and my rate-control have > been merged. Note that my series is rebased on top of David's work at: > https://patchwork.libcamera.org/patch/11730/) > > so I will also have to wait for that to be submitted. Great, I'll record that with a \todo then Thanks j > > Regards, > Naush > > > > > > > > Let me know what you think. > > > > > > [1] https://patchwork.libcamera.org/patch/12006/ > > > > > > > > > > > + ASSERT(request); > > > > > > > > + > > > > > > > > + > > request->metadata().set(controls::SensorTimestamp, > > > > > > > > + > > buffer->metadata().timestamp); > > > > > > > > + > > > > > > > > /* > > > > > > > > * Lookup the sensor controls used for this > > frame sequence from > > > > > > > > * DelayedControl and queue them along with > > the frame buffer. > > > > -- > > Regards, > > > > Laurent Pinchart > >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index d1902bfc3393..7dc92acf3c4f 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1415,6 +1415,18 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) << ", timestamp: " << buffer->metadata().timestamp; if (stream == &unicam_[Unicam::Image]) { + /* + * Record the sensor timestamp in the Request. + * + * \todo The sensor timestamp should be better estimated by + * sampling the V4L2Device::frameStart signal. + */ + Request *request = requestQueue_.front(); + ASSERT(request); + + request->metadata().set(controls::SensorTimestamp, + buffer->metadata().timestamp); + /* * Lookup the sensor controls used for this frame sequence from * DelayedControl and queue them along with the frame buffer.
Report the sensor's timestamp in the Request metadata by using the Unicam::Image buffer timestamp as an initial approximation. The buffer's timestamp is recorded at DMA-transfer time, and it does not theoretically matches the 'start of exposure' definition, but when used to compare two consecutive frames it gives an acceptable estimation of the sensor frame period duration. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+)