| Message ID | 20210616234422.24789-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 73b823b22009b87fc1d33a8000a870fe9223e7bb |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2021-06-17 02:44:22 +0300, Laurent Pinchart wrote: > Commit 922833f774f6 ("libcamera: simple: Report sensor timestamp") > unconditionally tries to access the request through the capture buffer > to store the capture timestamp in the metadata. This causes a null > pointer dereference when using a converter, as the capture buffers are > free-wheeling in that case, and not associated with a request. > > Fix this by getting the request from the user-facing buffer, which can > be the capture buffer when no converter is used. > > Fixes: 922833f774f6 ("libcamera: simple: Report sensor timestamp") > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 36fd9b852b33..e63d0bfd2fcb 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -1127,14 +1127,28 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) > } > > /* > - * Record the sensor's timestamp in the request metadata. > + * 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. > * > * \todo The sensor timestamp should be better estimated by connecting > * to the V4L2Device::frameStart signal if the platform provides it. > */ > Request *request = buffer->request(); > - request->metadata().set(controls::SensorTimestamp, > - buffer->metadata().timestamp); > + > + if (data->useConverter_ && !data->converterQueue_.empty()) { > + const std::map<unsigned int, FrameBuffer *> &outputs = > + data->converterQueue_.front(); > + if (!outputs.empty()) { > + FrameBuffer *outputBuffer = outputs.begin()->second; > + if (outputBuffer) > + request = outputBuffer->request(); > + } > + } > + > + if (request) > + request->metadata().set(controls::SensorTimestamp, > + buffer->metadata().timestamp); > > /* > * Queue the captured and the request buffer to the converter if format > -- > Regards, > > Laurent Pinchart >
Hi Laurent, On Thu, Jun 17, 2021 at 02:44:22AM +0300, Laurent Pinchart wrote: > Commit 922833f774f6 ("libcamera: simple: Report sensor timestamp") > unconditionally tries to access the request through the capture buffer > to store the capture timestamp in the metadata. This causes a null > pointer dereference when using a converter, as the capture buffers are > free-wheeling in that case, and not associated with a request. > > Fix this by getting the request from the user-facing buffer, which can > be the capture buffer when no converter is used. > > Fixes: 922833f774f6 ("libcamera: simple: Report sensor timestamp") > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> My bad, my understanding of the converters in the simple pipeline is limited. Thanks for fixing > --- > src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++++++++++--- > 1 file changed, 17 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 36fd9b852b33..e63d0bfd2fcb 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -1127,14 +1127,28 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) > } > > /* > - * Record the sensor's timestamp in the request metadata. > + * 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. > * > * \todo The sensor timestamp should be better estimated by connecting > * to the V4L2Device::frameStart signal if the platform provides it. > */ > Request *request = buffer->request(); > - request->metadata().set(controls::SensorTimestamp, > - buffer->metadata().timestamp); > + > + if (data->useConverter_ && !data->converterQueue_.empty()) { > + const std::map<unsigned int, FrameBuffer *> &outputs = > + data->converterQueue_.front(); > + if (!outputs.empty()) { Can outputs be empty if !data->converterQueue_.empty() ? > + FrameBuffer *outputBuffer = outputs.begin()->second; > + if (outputBuffer) > + request = outputBuffer->request(); > + } > + } > + > + if (request) > + request->metadata().set(controls::SensorTimestamp, > + buffer->metadata().timestamp); We could end up without a request Before 922833f774f6 the Request * was simply retreived with Request *request = buffer->request(); If we can now end up without a Request *, how can we complete it here below ? Thanks j > > /* > * Queue the captured and the request buffer to the converter if format > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Thu, Jun 17, 2021 at 10:10:21AM +0200, Jacopo Mondi wrote: > On Thu, Jun 17, 2021 at 02:44:22AM +0300, Laurent Pinchart wrote: > > Commit 922833f774f6 ("libcamera: simple: Report sensor timestamp") > > unconditionally tries to access the request through the capture buffer > > to store the capture timestamp in the metadata. This causes a null > > pointer dereference when using a converter, as the capture buffers are > > free-wheeling in that case, and not associated with a request. > > > > Fix this by getting the request from the user-facing buffer, which can > > be the capture buffer when no converter is used. > > > > Fixes: 922833f774f6 ("libcamera: simple: Report sensor timestamp") > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > My bad, my understanding of the converters in the simple pipeline is > limited. Thanks for fixing No worries. I missed it during review too :-) > > --- > > src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++++++++++--- > > 1 file changed, 17 insertions(+), 3 deletions(-) > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > index 36fd9b852b33..e63d0bfd2fcb 100644 > > --- a/src/libcamera/pipeline/simple/simple.cpp > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > @@ -1127,14 +1127,28 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) > > } > > > > /* > > - * Record the sensor's timestamp in the request metadata. > > + * 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. > > * > > * \todo The sensor timestamp should be better estimated by connecting > > * to the V4L2Device::frameStart signal if the platform provides it. > > */ > > Request *request = buffer->request(); > > - request->metadata().set(controls::SensorTimestamp, > > - buffer->metadata().timestamp); > > + > > + if (data->useConverter_ && !data->converterQueue_.empty()) { > > + const std::map<unsigned int, FrameBuffer *> &outputs = > > + data->converterQueue_.front(); > > + if (!outputs.empty()) { > > Can outputs be empty if !data->converterQueue_.empty() ? They shouldn't, but as there's the same sanity check in converter->queueBuffers(), which is called *after* this code block, I thought it would be best to be cautious. Eventually we should probably rework this to only queue buffers for capture when there are request queued, the same way te IPU3 pipeline handler does it, but that was a too big change for a fix. > > + FrameBuffer *outputBuffer = outputs.begin()->second; > > + if (outputBuffer) > > + request = outputBuffer->request(); > > + } > > + } > > + > > + if (request) > > + request->metadata().set(controls::SensorTimestamp, > > + buffer->metadata().timestamp); > > We could end up without a request > Before 922833f774f6 the Request * was simply retreived with > > Request *request = buffer->request(); > > If we can now end up without a Request *, how can we complete it here > below ? request can be null here if data->useConverter && data->converterQueue_.empty() In that case we'll take the `if (data->useConverter_)` branch, and won't reach the completeBuffer() call. It's a little bit convoluted, the other option was to deplicate the request->metadata().set() call, which I didn't like much. > > /* > > * Queue the captured and the request buffer to the converter if format
Hi Laurent, On Thu, Jun 17, 2021 at 12:12:28PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Thu, Jun 17, 2021 at 10:10:21AM +0200, Jacopo Mondi wrote: > > On Thu, Jun 17, 2021 at 02:44:22AM +0300, Laurent Pinchart wrote: > > > Commit 922833f774f6 ("libcamera: simple: Report sensor timestamp") > > > unconditionally tries to access the request through the capture buffer > > > to store the capture timestamp in the metadata. This causes a null > > > pointer dereference when using a converter, as the capture buffers are > > > free-wheeling in that case, and not associated with a request. > > > > > > Fix this by getting the request from the user-facing buffer, which can > > > be the capture buffer when no converter is used. > > > > > > Fixes: 922833f774f6 ("libcamera: simple: Report sensor timestamp") > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > My bad, my understanding of the converters in the simple pipeline is > > limited. Thanks for fixing > > No worries. I missed it during review too :-) > > > > --- > > > src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++++++++++--- > > > 1 file changed, 17 insertions(+), 3 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > > index 36fd9b852b33..e63d0bfd2fcb 100644 > > > --- a/src/libcamera/pipeline/simple/simple.cpp > > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > > @@ -1127,14 +1127,28 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) > > > } > > > > > > /* > > > - * Record the sensor's timestamp in the request metadata. > > > + * 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. > > > * > > > * \todo The sensor timestamp should be better estimated by connecting > > > * to the V4L2Device::frameStart signal if the platform provides it. > > > */ > > > Request *request = buffer->request(); > > > - request->metadata().set(controls::SensorTimestamp, > > > - buffer->metadata().timestamp); > > > + > > > + if (data->useConverter_ && !data->converterQueue_.empty()) { > > > + const std::map<unsigned int, FrameBuffer *> &outputs = > > > + data->converterQueue_.front(); > > > + if (!outputs.empty()) { > > > > Can outputs be empty if !data->converterQueue_.empty() ? > > They shouldn't, but as there's the same sanity check in > converter->queueBuffers(), which is called *after* this code block, I > thought it would be best to be cautious. Eventually we should probably > rework this to only queue buffers for capture when there are request > queued, the same way te IPU3 pipeline handler does it, but that was a > too big change for a fix. > > > > + FrameBuffer *outputBuffer = outputs.begin()->second; > > > + if (outputBuffer) > > > + request = outputBuffer->request(); > > > + } > > > + } > > > + > > > + if (request) > > > + request->metadata().set(controls::SensorTimestamp, > > > + buffer->metadata().timestamp); > > > > We could end up without a request > > Before 922833f774f6 the Request * was simply retreived with > > > > Request *request = buffer->request(); > > > > If we can now end up without a Request *, how can we complete it here > > below ? > > request can be null here if > > data->useConverter && data->converterQueue_.empty() > > In that case we'll take the `if (data->useConverter_)` branch, and won't > reach the completeBuffer() call. Oh right, I missed the condition check > > It's a little bit convoluted, the other option was to deplicate the > request->metadata().set() call, which I didn't like much. Yes, not easy to follow in full, thanks for the clarifications Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > > > /* > > > * Queue the captured and the request buffer to the converter if format > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 36fd9b852b33..e63d0bfd2fcb 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -1127,14 +1127,28 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) } /* - * Record the sensor's timestamp in the request metadata. + * 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. * * \todo The sensor timestamp should be better estimated by connecting * to the V4L2Device::frameStart signal if the platform provides it. */ Request *request = buffer->request(); - request->metadata().set(controls::SensorTimestamp, - buffer->metadata().timestamp); + + if (data->useConverter_ && !data->converterQueue_.empty()) { + const std::map<unsigned int, FrameBuffer *> &outputs = + data->converterQueue_.front(); + if (!outputs.empty()) { + FrameBuffer *outputBuffer = outputs.begin()->second; + if (outputBuffer) + request = outputBuffer->request(); + } + } + + if (request) + request->metadata().set(controls::SensorTimestamp, + buffer->metadata().timestamp); /* * Queue the captured and the request buffer to the converter if format
Commit 922833f774f6 ("libcamera: simple: Report sensor timestamp") unconditionally tries to access the request through the capture buffer to store the capture timestamp in the metadata. This causes a null pointer dereference when using a converter, as the capture buffers are free-wheeling in that case, and not associated with a request. Fix this by getting the request from the user-facing buffer, which can be the capture buffer when no converter is used. Fixes: 922833f774f6 ("libcamera: simple: Report sensor timestamp") Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)