| Message ID | 20241127092632.3145984-5-chenghaoyang@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Harevy On Wed, Nov 27, 2024 at 09:25:54AM +0000, Harvey Yang wrote: > Previously there's a potential issue in StreamBuffer::internalBuffer's > lifetime, when more than one streams depend on the same internal buffer. s/.// lifetime, when more than one streams depend on the same internal buffer for post-processing, the buffer is returned to the CameraStream pool as soon as the first post-processing request has completed. > > This patch fixes the issue by returning the buffer when the whole > Camera3RequestDescriptor is destructed. Actually it does more I guess, it allows to map multiple streams of type ::Mapped to the same libcamera::Stream, doesn't it ? > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > --- > src/android/camera_device.cpp | 63 +++++++++++++++++++--------------- > src/android/camera_request.cpp | 13 ++++--- > src/android/camera_request.h | 3 +- > 3 files changed, 46 insertions(+), 33 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 11613fac9..62f724041 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -967,9 +967,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > * to a libcamera stream. Streams of type Mapped will be handled later. > * > * Collect the CameraStream associated to each requested capture stream. > - * Since requestedStreams is an std:map<>, no duplications can happen. > + * Since requestedDirectBuffers is an std:map<>, no duplications can > + * happen. > */ > - std::map<CameraStream *, libcamera::FrameBuffer *> requestedBuffers; > + std::map<CameraStream *, libcamera::FrameBuffer *> requestedDirectBuffers; Do you actually need to rename ? > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > CameraStream *cameraStream = buffer.stream; > camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); > @@ -1009,7 +1010,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > frameBuffer = buffer.frameBuffer.get(); > acquireFence = std::move(buffer.fence); > > - requestedBuffers[cameraStream] = frameBuffer; > + requestedDirectBuffers[cameraStream] = frameBuffer; > LOG(HAL, Debug) << ss.str() << " (direct)"; > break; > > @@ -1018,14 +1019,17 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > * Get the frame buffer from the CameraStream internal > * buffer pool. > * > - * The buffer has to be returned to the CameraStream > - * once it has been processed. > + * The buffer will be returned to the CameraStream when > + * the request is destructed. or 'destroyed' > */ > frameBuffer = cameraStream->getBuffer(); > - buffer.internalBuffer = frameBuffer; > buffer.srcBuffer = frameBuffer; > > - requestedBuffers[cameraStream] = frameBuffer; > + /* > + * Track the allocated internal buffers, which will be > + * recycled when the descriptor destroyed. > + * */ Stray * */ > + descriptor->internalBuffers_[cameraStream] = frameBuffer; > LOG(HAL, Debug) << ss.str() << " (internal)"; > > descriptor->pendingStreamsToProcess_.insert( > @@ -1079,24 +1083,41 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > * post-processing. No need to recycle the buffer since it's > * owned by Android. > */ > - auto iterDirectBuffer = requestedBuffers.find(sourceStream); > - if (iterDirectBuffer != requestedBuffers.end()) { > + auto iterDirectBuffer = requestedDirectBuffers.find(sourceStream); > + if (iterDirectBuffer != requestedDirectBuffers.end()) { > buffer.srcBuffer = iterDirectBuffer->second; > continue; > } > > /* > - * If that's not the case, we need to add a buffer to the request > - * for this stream. > + * If that's not the case, we use an internal buffer allocated > + * from the source stream. > + * > + * If an internal buffer has been requested for the source > + * stream before, we should reuse it. > + */ > + auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream); > + if (iterInternalBuffer != descriptor->internalBuffers_.end()) { > + buffer.srcBuffer = iterInternalBuffer->second; > + continue; > + } > + > + /* > + * Otherwise, we need to create an internal buffer to the > + * request for the source stream. Get the frame buffer from the > + * source stream's internal buffer pool. > + * > + * The buffer will be returned to the CameraStream when the > + * request is destructed. > */ > - FrameBuffer *frameBuffer = cameraStream->getBuffer(); > - buffer.internalBuffer = frameBuffer; > + FrameBuffer *frameBuffer = sourceStream->getBuffer(); Here I don't see why we're using the sourceStream's pool instead of the cameraStream's pool However, I think this goes in the direction of making the HAL capable of more things, and I'm looking at the diff of this and the previous patch combined and I think you should squash the two and make a commit about "Enabling multiple processed streams to map to the same internal buffer". Looking at the combined diff, it feels to me you could populate buffer.src for the Direct case in the switch() and keep using an std::set for requestedDirectBuffers (or requestedStreams). This would make the diff smaller probably. > buffer.srcBuffer = frameBuffer; > > descriptor->request_->addBuffer(sourceStream->stream(), > frameBuffer, nullptr); > > - requestedBuffers[sourceStream] = frameBuffer; > + /* Track the allocated internal buffer. */ > + descriptor->internalBuffers_[sourceStream] = frameBuffer; > } > > /* > @@ -1256,13 +1277,6 @@ void CameraDevice::requestComplete(Request *request) > if (ret) { > setBufferStatus(*buffer, StreamBuffer::Status::Error); > descriptor->pendingStreamsToProcess_.erase(stream); > - > - /* > - * If the framebuffer is internal to CameraStream return > - * it back now that we're done processing it. > - */ > - if (buffer->internalBuffer) > - stream->putBuffer(buffer->internalBuffer); > } > } > > @@ -1381,13 +1395,6 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer, > { > setBufferStatus(*streamBuffer, status); > > - /* > - * If the framebuffer is internal to CameraStream return it back now > - * that we're done processing it. > - */ > - if (streamBuffer->internalBuffer) > - streamBuffer->stream->putBuffer(streamBuffer->internalBuffer); > - > Camera3RequestDescriptor *request = streamBuffer->request; > > { > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp > index 52a3ac1f7..a9240a83c 100644 > --- a/src/android/camera_request.cpp > +++ b/src/android/camera_request.cpp > @@ -10,6 +10,7 @@ > #include <libcamera/base/span.h> > > #include "camera_buffer.h" > +#include "camera_stream.h" > > using namespace libcamera; > > @@ -138,7 +139,14 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( > request_ = camera->createRequest(reinterpret_cast<uint64_t>(this)); > } > > -Camera3RequestDescriptor::~Camera3RequestDescriptor() = default; > +Camera3RequestDescriptor::~Camera3RequestDescriptor() > +{ > + /* > + * Recycle the allocated internal buffer back to its source stream. > + */ Fits in one line most probably > + for (auto &[sourceStream, frameBuffer] : internalBuffers_) > + sourceStream->putBuffer(frameBuffer); > +} > > /** > * \class StreamBuffer > @@ -166,9 +174,6 @@ Camera3RequestDescriptor::~Camera3RequestDescriptor() = default; > * \var StreamBuffer::status > * \brief Track the status of the buffer > * > - * \var StreamBuffer::internalBuffer > - * \brief Pointer to a buffer internally handled by CameraStream (if any) > - * > * \var StreamBuffer::srcBuffer > * \brief Pointer to the source frame buffer used for post-processing > * > diff --git a/src/android/camera_request.h b/src/android/camera_request.h > index 335f1985d..6b2a00795 100644 > --- a/src/android/camera_request.h > +++ b/src/android/camera_request.h > @@ -49,7 +49,6 @@ public: > std::unique_ptr<HALFrameBuffer> frameBuffer; > libcamera::UniqueFD fence; > Status status = Status::Success; > - libcamera::FrameBuffer *internalBuffer = nullptr; > const libcamera::FrameBuffer *srcBuffer = nullptr; > std::unique_ptr<CameraBuffer> dstBuffer; > Camera3RequestDescriptor *request; > @@ -85,6 +84,8 @@ public: > std::unique_ptr<libcamera::Request> request_; > std::unique_ptr<CameraMetadata> resultMetadata_; > > + std::map<CameraStream *, libcamera::FrameBuffer *> internalBuffers_; > + > bool complete_ = false; > Status status_ = Status::Success; > > -- > 2.47.0.338.g60cca15819-goog >
On Thu, Nov 28, 2024 at 03:24:44PM +0100, Jacopo Mondi wrote: > Hi Harevy > > On Wed, Nov 27, 2024 at 09:25:54AM +0000, Harvey Yang wrote: > > Previously there's a potential issue in StreamBuffer::internalBuffer's > > lifetime, when more than one streams depend on the same internal buffer. > > s/.// > > lifetime, when more than one streams depend on the same internal buffer > for post-processing, the buffer is returned to the CameraStream pool > as soon as the first post-processing request has completed. > > > > > This patch fixes the issue by returning the buffer when the whole > > Camera3RequestDescriptor is destructed. > > Actually it does more I guess, it allows to map multiple streams of > type ::Mapped to the same libcamera::Stream, doesn't it ? > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > --- > > src/android/camera_device.cpp | 63 +++++++++++++++++++--------------- > > src/android/camera_request.cpp | 13 ++++--- > > src/android/camera_request.h | 3 +- > > 3 files changed, 46 insertions(+), 33 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 11613fac9..62f724041 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -967,9 +967,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > * to a libcamera stream. Streams of type Mapped will be handled later. > > * > > * Collect the CameraStream associated to each requested capture stream. > > - * Since requestedStreams is an std:map<>, no duplications can happen. > > + * Since requestedDirectBuffers is an std:map<>, no duplications can > > + * happen. > > */ > > - std::map<CameraStream *, libcamera::FrameBuffer *> requestedBuffers; > > + std::map<CameraStream *, libcamera::FrameBuffer *> requestedDirectBuffers; > > Do you actually need to rename ? > > > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > > CameraStream *cameraStream = buffer.stream; > > camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); > > @@ -1009,7 +1010,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > frameBuffer = buffer.frameBuffer.get(); > > acquireFence = std::move(buffer.fence); > > > > - requestedBuffers[cameraStream] = frameBuffer; > > + requestedDirectBuffers[cameraStream] = frameBuffer; > > LOG(HAL, Debug) << ss.str() << " (direct)"; > > break; > > > > @@ -1018,14 +1019,17 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > * Get the frame buffer from the CameraStream internal > > * buffer pool. > > * > > - * The buffer has to be returned to the CameraStream > > - * once it has been processed. > > + * The buffer will be returned to the CameraStream when > > + * the request is destructed. > > or 'destroyed' > > > */ > > frameBuffer = cameraStream->getBuffer(); > > - buffer.internalBuffer = frameBuffer; > > buffer.srcBuffer = frameBuffer; > > > > - requestedBuffers[cameraStream] = frameBuffer; > > + /* > > + * Track the allocated internal buffers, which will be > > + * recycled when the descriptor destroyed. > > + * */ > > Stray * */ > > > + descriptor->internalBuffers_[cameraStream] = frameBuffer; > > LOG(HAL, Debug) << ss.str() << " (internal)"; > > > > descriptor->pendingStreamsToProcess_.insert( > > @@ -1079,24 +1083,41 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > * post-processing. No need to recycle the buffer since it's > > * owned by Android. > > */ > > - auto iterDirectBuffer = requestedBuffers.find(sourceStream); > > - if (iterDirectBuffer != requestedBuffers.end()) { > > + auto iterDirectBuffer = requestedDirectBuffers.find(sourceStream); > > + if (iterDirectBuffer != requestedDirectBuffers.end()) { > > buffer.srcBuffer = iterDirectBuffer->second; > > continue; > > } > > > > /* > > - * If that's not the case, we need to add a buffer to the request > > - * for this stream. > > + * If that's not the case, we use an internal buffer allocated > > + * from the source stream. > > + * > > + * If an internal buffer has been requested for the source > > + * stream before, we should reuse it. > > + */ > > + auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream); > > + if (iterInternalBuffer != descriptor->internalBuffers_.end()) { > > + buffer.srcBuffer = iterInternalBuffer->second; > > + continue; > > + } > > + > > + /* > > + * Otherwise, we need to create an internal buffer to the > > + * request for the source stream. Get the frame buffer from the > > + * source stream's internal buffer pool. > > + * > > + * The buffer will be returned to the CameraStream when the > > + * request is destructed. > > */ > > - FrameBuffer *frameBuffer = cameraStream->getBuffer(); > > - buffer.internalBuffer = frameBuffer; > > + FrameBuffer *frameBuffer = sourceStream->getBuffer(); > > Here I don't see why we're using the sourceStream's pool instead > of the cameraStream's pool > > However, I think this goes in the direction of making the HAL capable > of more things, and I'm looking at the diff of this and the previous > patch combined and I think you should squash the two and make a commit > about "Enabling multiple processed streams to map to the same internal > buffer". Processed streams are expensive, is there enough CPU power to make that possible ? I suppose it wouldn't be an issue when producing, for instance, a processed JPEG stream with a hardware encoder and a scaled YUV stream with libyuv from the same stream. This is interesting, but isn't mentioned at all in the cover letter, and goes well beyond the subject of the series. Is is an intentional change ? > Looking at the combined diff, it feels to me you could populate > buffer.src for the Direct case in the switch() and keep using an > std::set for requestedDirectBuffers (or requestedStreams). This would > make the diff smaller probably. > > > > buffer.srcBuffer = frameBuffer; > > > > descriptor->request_->addBuffer(sourceStream->stream(), > > frameBuffer, nullptr); > > > > - requestedBuffers[sourceStream] = frameBuffer; > > + /* Track the allocated internal buffer. */ > > + descriptor->internalBuffers_[sourceStream] = frameBuffer; > > } > > > > /* > > @@ -1256,13 +1277,6 @@ void CameraDevice::requestComplete(Request *request) > > if (ret) { > > setBufferStatus(*buffer, StreamBuffer::Status::Error); > > descriptor->pendingStreamsToProcess_.erase(stream); > > - > > - /* > > - * If the framebuffer is internal to CameraStream return > > - * it back now that we're done processing it. > > - */ > > - if (buffer->internalBuffer) > > - stream->putBuffer(buffer->internalBuffer); > > } > > } > > > > @@ -1381,13 +1395,6 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer, > > { > > setBufferStatus(*streamBuffer, status); > > > > - /* > > - * If the framebuffer is internal to CameraStream return it back now > > - * that we're done processing it. > > - */ > > - if (streamBuffer->internalBuffer) > > - streamBuffer->stream->putBuffer(streamBuffer->internalBuffer); > > - > > Camera3RequestDescriptor *request = streamBuffer->request; > > > > { > > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp > > index 52a3ac1f7..a9240a83c 100644 > > --- a/src/android/camera_request.cpp > > +++ b/src/android/camera_request.cpp > > @@ -10,6 +10,7 @@ > > #include <libcamera/base/span.h> > > > > #include "camera_buffer.h" > > +#include "camera_stream.h" > > > > using namespace libcamera; > > > > @@ -138,7 +139,14 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( > > request_ = camera->createRequest(reinterpret_cast<uint64_t>(this)); > > } > > > > -Camera3RequestDescriptor::~Camera3RequestDescriptor() = default; > > +Camera3RequestDescriptor::~Camera3RequestDescriptor() > > +{ > > + /* > > + * Recycle the allocated internal buffer back to its source stream. > > + */ > > Fits in one line most probably > > > + for (auto &[sourceStream, frameBuffer] : internalBuffers_) > > + sourceStream->putBuffer(frameBuffer); > > +} > > > > /** > > * \class StreamBuffer > > @@ -166,9 +174,6 @@ Camera3RequestDescriptor::~Camera3RequestDescriptor() = default; > > * \var StreamBuffer::status > > * \brief Track the status of the buffer > > * > > - * \var StreamBuffer::internalBuffer > > - * \brief Pointer to a buffer internally handled by CameraStream (if any) > > - * > > * \var StreamBuffer::srcBuffer > > * \brief Pointer to the source frame buffer used for post-processing > > * > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h > > index 335f1985d..6b2a00795 100644 > > --- a/src/android/camera_request.h > > +++ b/src/android/camera_request.h > > @@ -49,7 +49,6 @@ public: > > std::unique_ptr<HALFrameBuffer> frameBuffer; > > libcamera::UniqueFD fence; > > Status status = Status::Success; > > - libcamera::FrameBuffer *internalBuffer = nullptr; > > const libcamera::FrameBuffer *srcBuffer = nullptr; > > std::unique_ptr<CameraBuffer> dstBuffer; > > Camera3RequestDescriptor *request; > > @@ -85,6 +84,8 @@ public: > > std::unique_ptr<libcamera::Request> request_; > > std::unique_ptr<CameraMetadata> resultMetadata_; > > > > + std::map<CameraStream *, libcamera::FrameBuffer *> internalBuffers_; > > + > > bool complete_ = false; > > Status status_ = Status::Success; > >
Hi Jacopo and Laurent, On Thu, Nov 28, 2024 at 10:32 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Thu, Nov 28, 2024 at 03:24:44PM +0100, Jacopo Mondi wrote: > > Hi Harevy > > > > On Wed, Nov 27, 2024 at 09:25:54AM +0000, Harvey Yang wrote: > > > Previously there's a potential issue in StreamBuffer::internalBuffer's > > > lifetime, when more than one streams depend on the same internal buffer. > > > > s/.// > > > > lifetime, when more than one streams depend on the same internal buffer > > for post-processing, the buffer is returned to the CameraStream pool > > as soon as the first post-processing request has completed. Updated. > > > > > > > > This patch fixes the issue by returning the buffer when the whole > > > Camera3RequestDescriptor is destructed. > > > > Actually it does more I guess, it allows to map multiple streams of > > type ::Mapped to the same libcamera::Stream, doesn't it ? According to the previous implementation, I'd say that's already allowed, just not correctly implemented yet. Looking into [1], the condition doesn't care if the sourceStream has been mapped by another mapped stream or not. Unless you mean that because there's such a bug, it's not allowed... I'd say that we should have a FATAL/ERROR log at least, or even block such a configuration. [1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1071 > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > --- > > > src/android/camera_device.cpp | 63 +++++++++++++++++++--------------- > > > src/android/camera_request.cpp | 13 ++++--- > > > src/android/camera_request.h | 3 +- > > > 3 files changed, 46 insertions(+), 33 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 11613fac9..62f724041 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -967,9 +967,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > * to a libcamera stream. Streams of type Mapped will be handled later. > > > * > > > * Collect the CameraStream associated to each requested capture stream. > > > - * Since requestedStreams is an std:map<>, no duplications can happen. > > > + * Since requestedDirectBuffers is an std:map<>, no duplications can > > > + * happen. > > > */ > > > - std::map<CameraStream *, libcamera::FrameBuffer *> requestedBuffers; > > > + std::map<CameraStream *, libcamera::FrameBuffer *> requestedDirectBuffers; > > > > Do you actually need to rename ? In this patch, this map only includes buffers from direct type. The internal type buffers are included by `Camera3RequestDescriptor::internalBuffers_` instead. If we don't exclude the internal ones in this map, we don't need to rename it as well. WDYT? > > > > > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > > > CameraStream *cameraStream = buffer.stream; > > > camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); > > > @@ -1009,7 +1010,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > frameBuffer = buffer.frameBuffer.get(); > > > acquireFence = std::move(buffer.fence); > > > > > > - requestedBuffers[cameraStream] = frameBuffer; > > > + requestedDirectBuffers[cameraStream] = frameBuffer; > > > LOG(HAL, Debug) << ss.str() << " (direct)"; > > > break; > > > > > > @@ -1018,14 +1019,17 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > * Get the frame buffer from the CameraStream internal > > > * buffer pool. > > > * > > > - * The buffer has to be returned to the CameraStream > > > - * once it has been processed. > > > + * The buffer will be returned to the CameraStream when > > > + * the request is destructed. > > > > or 'destroyed' Done > > > > > */ > > > frameBuffer = cameraStream->getBuffer(); > > > - buffer.internalBuffer = frameBuffer; > > > buffer.srcBuffer = frameBuffer; > > > > > > - requestedBuffers[cameraStream] = frameBuffer; > > > + /* > > > + * Track the allocated internal buffers, which will be > > > + * recycled when the descriptor destroyed. > > > + * */ > > > > Stray * */ Updated > > > > > + descriptor->internalBuffers_[cameraStream] = frameBuffer; > > > LOG(HAL, Debug) << ss.str() << " (internal)"; > > > > > > descriptor->pendingStreamsToProcess_.insert( > > > @@ -1079,24 +1083,41 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > * post-processing. No need to recycle the buffer since it's > > > * owned by Android. > > > */ > > > - auto iterDirectBuffer = requestedBuffers.find(sourceStream); > > > - if (iterDirectBuffer != requestedBuffers.end()) { > > > + auto iterDirectBuffer = requestedDirectBuffers.find(sourceStream); > > > + if (iterDirectBuffer != requestedDirectBuffers.end()) { > > > buffer.srcBuffer = iterDirectBuffer->second; > > > continue; > > > } > > > > > > /* > > > - * If that's not the case, we need to add a buffer to the request > > > - * for this stream. > > > + * If that's not the case, we use an internal buffer allocated > > > + * from the source stream. > > > + * > > > + * If an internal buffer has been requested for the source > > > + * stream before, we should reuse it. > > > + */ > > > + auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream); > > > + if (iterInternalBuffer != descriptor->internalBuffers_.end()) { > > > + buffer.srcBuffer = iterInternalBuffer->second; > > > + continue; > > > + } > > > + > > > + /* > > > + * Otherwise, we need to create an internal buffer to the > > > + * request for the source stream. Get the frame buffer from the > > > + * source stream's internal buffer pool. > > > + * > > > + * The buffer will be returned to the CameraStream when the > > > + * request is destructed. > > > */ > > > - FrameBuffer *frameBuffer = cameraStream->getBuffer(); > > > - buffer.internalBuffer = frameBuffer; > > > + FrameBuffer *frameBuffer = sourceStream->getBuffer(); > > > > Here I don't see why we're using the sourceStream's pool instead > > of the cameraStream's pool Because the buffer recycle process is different. In this patch, it'll be returned to the stream set to `internalBuffers_`. sourceStream is the correct one after this patch. > > > > However, I think this goes in the direction of making the HAL capable > > of more things, and I'm looking at the diff of this and the previous > > patch combined and I think you should squash the two and make a commit > > about "Enabling multiple processed streams to map to the same internal > > buffer". Hmm, as I stated above. I think multiple processed streams to map to the same internal buffer is already allowed, just that it's not handled properly. Do you still think that we should squash the two into one? > > Processed streams are expensive, is there enough CPU power to make that > possible ? I suppose it wouldn't be an issue when producing, for > instance, a processed JPEG stream with a hardware encoder and a scaled > YUV stream with libyuv from the same stream. > > This is interesting, but isn't mentioned at all in the cover letter, and > goes well beyond the subject of the series. Is is an intentional change > ? Therefore, I took this as a fix of an issue, instead of adding a feature. Let me know if you prefer to merge this as a separate series. > > > Looking at the combined diff, it feels to me you could populate > > buffer.src for the Direct case in the switch() and keep using an Sorry, I don't get it. Mapped streams need to be handled the last, so that other streams and `requestedDirectBuffers` and `internalBuffers_` are setup, IIUC. > > std::set for requestedDirectBuffers (or requestedStreams). This would > > make the diff smaller probably. > > > > > > > buffer.srcBuffer = frameBuffer; > > > > > > descriptor->request_->addBuffer(sourceStream->stream(), > > > frameBuffer, nullptr); > > > > > > - requestedBuffers[sourceStream] = frameBuffer; > > > + /* Track the allocated internal buffer. */ > > > + descriptor->internalBuffers_[sourceStream] = frameBuffer; > > > } > > > > > > /* > > > @@ -1256,13 +1277,6 @@ void CameraDevice::requestComplete(Request *request) > > > if (ret) { > > > setBufferStatus(*buffer, StreamBuffer::Status::Error); > > > descriptor->pendingStreamsToProcess_.erase(stream); > > > - > > > - /* > > > - * If the framebuffer is internal to CameraStream return > > > - * it back now that we're done processing it. > > > - */ > > > - if (buffer->internalBuffer) > > > - stream->putBuffer(buffer->internalBuffer); > > > } > > > } > > > > > > @@ -1381,13 +1395,6 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer, > > > { > > > setBufferStatus(*streamBuffer, status); > > > > > > - /* > > > - * If the framebuffer is internal to CameraStream return it back now > > > - * that we're done processing it. > > > - */ > > > - if (streamBuffer->internalBuffer) > > > - streamBuffer->stream->putBuffer(streamBuffer->internalBuffer); > > > - > > > Camera3RequestDescriptor *request = streamBuffer->request; > > > > > > { > > > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp > > > index 52a3ac1f7..a9240a83c 100644 > > > --- a/src/android/camera_request.cpp > > > +++ b/src/android/camera_request.cpp > > > @@ -10,6 +10,7 @@ > > > #include <libcamera/base/span.h> > > > > > > #include "camera_buffer.h" > > > +#include "camera_stream.h" > > > > > > using namespace libcamera; > > > > > > @@ -138,7 +139,14 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( > > > request_ = camera->createRequest(reinterpret_cast<uint64_t>(this)); > > > } > > > > > > -Camera3RequestDescriptor::~Camera3RequestDescriptor() = default; > > > +Camera3RequestDescriptor::~Camera3RequestDescriptor() > > > +{ > > > + /* > > > + * Recycle the allocated internal buffer back to its source stream. > > > + */ > > > > Fits in one line most probably Done BR, Harvey > > > > > + for (auto &[sourceStream, frameBuffer] : internalBuffers_) > > > + sourceStream->putBuffer(frameBuffer); > > > +} > > > > > > /** > > > * \class StreamBuffer > > > @@ -166,9 +174,6 @@ Camera3RequestDescriptor::~Camera3RequestDescriptor() = default; > > > * \var StreamBuffer::status > > > * \brief Track the status of the buffer > > > * > > > - * \var StreamBuffer::internalBuffer > > > - * \brief Pointer to a buffer internally handled by CameraStream (if any) > > > - * > > > * \var StreamBuffer::srcBuffer > > > * \brief Pointer to the source frame buffer used for post-processing > > > * > > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h > > > index 335f1985d..6b2a00795 100644 > > > --- a/src/android/camera_request.h > > > +++ b/src/android/camera_request.h > > > @@ -49,7 +49,6 @@ public: > > > std::unique_ptr<HALFrameBuffer> frameBuffer; > > > libcamera::UniqueFD fence; > > > Status status = Status::Success; > > > - libcamera::FrameBuffer *internalBuffer = nullptr; > > > const libcamera::FrameBuffer *srcBuffer = nullptr; > > > std::unique_ptr<CameraBuffer> dstBuffer; > > > Camera3RequestDescriptor *request; > > > @@ -85,6 +84,8 @@ public: > > > std::unique_ptr<libcamera::Request> request_; > > > std::unique_ptr<CameraMetadata> resultMetadata_; > > > > > > + std::map<CameraStream *, libcamera::FrameBuffer *> internalBuffers_; > > > + > > > bool complete_ = false; > > > Status status_ = Status::Success; > > > > > -- > Regards, > > Laurent Pinchart
Hi Harvey On Fri, Nov 29, 2024 at 03:51:39PM +0800, Cheng-Hao Yang wrote: > Hi Jacopo and Laurent, > > On Thu, Nov 28, 2024 at 10:32 PM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > On Thu, Nov 28, 2024 at 03:24:44PM +0100, Jacopo Mondi wrote: > > > Hi Harevy > > > > > > On Wed, Nov 27, 2024 at 09:25:54AM +0000, Harvey Yang wrote: > > > > Previously there's a potential issue in StreamBuffer::internalBuffer's > > > > lifetime, when more than one streams depend on the same internal buffer. > > > > > > s/.// > > > > > > lifetime, when more than one streams depend on the same internal buffer > > > for post-processing, the buffer is returned to the CameraStream pool > > > as soon as the first post-processing request has completed. > > Updated. > > > > > > > > > > > > This patch fixes the issue by returning the buffer when the whole > > > > Camera3RequestDescriptor is destructed. > > > > > > Actually it does more I guess, it allows to map multiple streams of > > > type ::Mapped to the same libcamera::Stream, doesn't it ? > > According to the previous implementation, I'd say that's already > allowed, just not correctly implemented yet. > Looking into [1], the condition doesn't care if the sourceStream > has been mapped by another mapped stream or not. No but as you can see it is not supported as we immediately return the source stream buffer once post-processing is done. However, something tells me this was by design as we allow a single (Mapped) JPEG stream /* Defer handling of MJPEG streams until all others are known. */ if (stream->format == HAL_PIXEL_FORMAT_BLOB) { if (jpegStream) { LOG(HAL, Error) << "Multiple JPEG streams are not supported"; return -EINVAL; } jpegStream = stream; continue; } What is the use case you have for mapping multiple Mapped streams to the same Direct|Internal one ? Multiple streams produced using libyuv conversion ? (afaict that's the only other post-processing case we support) > > Unless you mean that because there's such a bug, it's not allowed... > I'd say that we should have a FATAL/ERROR log at least, or even > block such a configuration. > Well, this patch fixes it, doesn't it ? > [1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1071 > > > > > > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > > --- > > > > src/android/camera_device.cpp | 63 +++++++++++++++++++--------------- > > > > src/android/camera_request.cpp | 13 ++++--- > > > > src/android/camera_request.h | 3 +- > > > > 3 files changed, 46 insertions(+), 33 deletions(-) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > index 11613fac9..62f724041 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -967,9 +967,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > > * to a libcamera stream. Streams of type Mapped will be handled later. > > > > * > > > > * Collect the CameraStream associated to each requested capture stream. > > > > - * Since requestedStreams is an std:map<>, no duplications can happen. > > > > + * Since requestedDirectBuffers is an std:map<>, no duplications can > > > > + * happen. > > > > */ > > > > - std::map<CameraStream *, libcamera::FrameBuffer *> requestedBuffers; > > > > + std::map<CameraStream *, libcamera::FrameBuffer *> requestedDirectBuffers; > > > > > > Do you actually need to rename ? > > In this patch, this map only includes buffers from direct type. > The internal type buffers are included by > `Camera3RequestDescriptor::internalBuffers_` instead. > let's call it directBuffers as a middle ground > If we don't exclude the internal ones in this map, we don't > need to rename it as well. WDYT? > > > > > > > > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > > > > CameraStream *cameraStream = buffer.stream; > > > > camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); > > > > @@ -1009,7 +1010,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > > frameBuffer = buffer.frameBuffer.get(); > > > > acquireFence = std::move(buffer.fence); > > > > > > > > - requestedBuffers[cameraStream] = frameBuffer; > > > > + requestedDirectBuffers[cameraStream] = frameBuffer; > > > > LOG(HAL, Debug) << ss.str() << " (direct)"; > > > > break; > > > > > > > > @@ -1018,14 +1019,17 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > > * Get the frame buffer from the CameraStream internal > > > > * buffer pool. > > > > * > > > > - * The buffer has to be returned to the CameraStream > > > > - * once it has been processed. > > > > + * The buffer will be returned to the CameraStream when > > > > + * the request is destructed. > > > > > > or 'destroyed' > > Done > > > > > > > > */ > > > > frameBuffer = cameraStream->getBuffer(); > > > > - buffer.internalBuffer = frameBuffer; > > > > buffer.srcBuffer = frameBuffer; > > > > > > > > - requestedBuffers[cameraStream] = frameBuffer; > > > > + /* > > > > + * Track the allocated internal buffers, which will be > > > > + * recycled when the descriptor destroyed. > > > > + * */ > > > > > > Stray * */ > > Updated > > > > > > > > + descriptor->internalBuffers_[cameraStream] = frameBuffer; > > > > LOG(HAL, Debug) << ss.str() << " (internal)"; > > > > > > > > descriptor->pendingStreamsToProcess_.insert( > > > > @@ -1079,24 +1083,41 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > > * post-processing. No need to recycle the buffer since it's > > > > * owned by Android. > > > > */ > > > > - auto iterDirectBuffer = requestedBuffers.find(sourceStream); > > > > - if (iterDirectBuffer != requestedBuffers.end()) { > > > > + auto iterDirectBuffer = requestedDirectBuffers.find(sourceStream); > > > > + if (iterDirectBuffer != requestedDirectBuffers.end()) { > > > > buffer.srcBuffer = iterDirectBuffer->second; > > > > continue; > > > > } > > > > > > > > /* > > > > - * If that's not the case, we need to add a buffer to the request > > > > - * for this stream. > > > > + * If that's not the case, we use an internal buffer allocated > > > > + * from the source stream. > > > > + * Could you break this comment in two ? The first part apply to the rest of the for() loop, the second part only to the following if() /* * If that's not the case, we use an internal buffer allocated * from the source stream. */ /* * If an internal buffer has been requested for the source * stream before, we should reuse it. */ auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream); if (iterInternalBuffer != descriptor->internalBuffers_.end()) { buffer.srcBuffer = iterInternalBuffer->second; continue; } /* * Otherwise, we need to create an internal buffer to the * request for the source stream. Get the frame buffer from the * source stream's internal buffer pool. * * The buffer will be returned to the CameraStream when the * request is destructed. */ FrameBuffer *frameBuffer = sourceStream->getBuffer(); buffer.srcBuffer = frameBuffer; descriptor->request_->addBuffer(sourceStream->stream(), frameBuffer, nullptr); > > > > + * If an internal buffer has been requested for the source > > > > + * stream before, we should reuse it. > > > > + */ > > > > + auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream); > > > > + if (iterInternalBuffer != descriptor->internalBuffers_.end()) { > > > > + buffer.srcBuffer = iterInternalBuffer->second; > > > > + continue; > > > > + } > > > > + > > > > + /* > > > > + * Otherwise, we need to create an internal buffer to the > > > > + * request for the source stream. Get the frame buffer from the > > > > + * source stream's internal buffer pool. > > > > + * > > > > + * The buffer will be returned to the CameraStream when the > > > > + * request is destructed. > > > > */ > > > > - FrameBuffer *frameBuffer = cameraStream->getBuffer(); > > > > - buffer.internalBuffer = frameBuffer; > > > > + FrameBuffer *frameBuffer = sourceStream->getBuffer(); > > > > > > Here I don't see why we're using the sourceStream's pool instead > > > of the cameraStream's pool > > Because the buffer recycle process is different. In this patch, it'll be > returned to the stream set to `internalBuffers_`. sourceStream is the > correct one after this patch. > Ok. As I understand this it is because, now that we allow multiple Mapped stream to be ... mapped ... on a Direct stream. If we fall here it's because the Direct stream its not part of the capture_request, so we have to create a buffer from a pool and map multiple processed streams on it. So yes the pool to use is the sourceStream one. > > > > > > However, I think this goes in the direction of making the HAL capable > > > of more things, and I'm looking at the diff of this and the previous > > > patch combined and I think you should squash the two and make a commit > > > about "Enabling multiple processed streams to map to the same internal > > > buffer". > > Hmm, as I stated above. I think multiple processed streams to map to the same > internal buffer is already allowed, just that it's not handled properly. > Do you still think that we should squash the two into one? > Looking at the combined diff it seems easier to make a single commit along the lines of ------------------------------------------------------------------------------- android: Correctly support multiple Mapped streams The Android camera HAL supports creating streams for the Android framework by post-processing streams produced by libcamera. However at the moment a single Mapped stream can be associated with a Direct stream because, after the first post-processing, the data from the internal stream are returned preventing further post-processing passes. Fix this by storing the list of Direct stream buffers used as the post-processing source in an Camera3RequestDescriptor::internalBuffers_ map to replace the single internalBuffer_ pointer and return the internal buffers when the capture request is deleted. ------------------------------------------------------------------------------- I'm sure this can be written a 1000 times better than this, but at least, this is the level of detail and description a commit message should provide > > > > Processed streams are expensive, is there enough CPU power to make that > > possible ? I suppose it wouldn't be an issue when producing, for > > instance, a processed JPEG stream with a hardware encoder and a scaled > > YUV stream with libyuv from the same stream. > > > > This is interesting, but isn't mentioned at all in the cover letter, and > > goes well beyond the subject of the series. Is is an intentional change > > ? > > Therefore, I took this as a fix of an issue, instead of adding a feature. > > Let me know if you prefer to merge this as a separate series. > > > > > > Looking at the combined diff, it feels to me you could populate > > > buffer.src for the Direct case in the switch() and keep using an > > Sorry, I don't get it. Mapped streams need to be handled the last, > so that other streams and `requestedDirectBuffers` and > `internalBuffers_` are setup, IIUC. > No you're right, there's no buffer.srcBuffer to populate for Direct Thanks j > > > std::set for requestedDirectBuffers (or requestedStreams). This would > > > make the diff smaller probably. > > > > > > > > > > buffer.srcBuffer = frameBuffer; > > > > > > > > descriptor->request_->addBuffer(sourceStream->stream(), > > > > frameBuffer, nullptr); > > > > > > > > - requestedBuffers[sourceStream] = frameBuffer; > > > > + /* Track the allocated internal buffer. */ > > > > + descriptor->internalBuffers_[sourceStream] = frameBuffer; > > > > } > > > > > > > > /* > > > > @@ -1256,13 +1277,6 @@ void CameraDevice::requestComplete(Request *request) > > > > if (ret) { > > > > setBufferStatus(*buffer, StreamBuffer::Status::Error); > > > > descriptor->pendingStreamsToProcess_.erase(stream); > > > > - > > > > - /* > > > > - * If the framebuffer is internal to CameraStream return > > > > - * it back now that we're done processing it. > > > > - */ > > > > - if (buffer->internalBuffer) > > > > - stream->putBuffer(buffer->internalBuffer); > > > > } > > > > } > > > > > > > > @@ -1381,13 +1395,6 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer, > > > > { > > > > setBufferStatus(*streamBuffer, status); > > > > > > > > - /* > > > > - * If the framebuffer is internal to CameraStream return it back now > > > > - * that we're done processing it. > > > > - */ > > > > - if (streamBuffer->internalBuffer) > > > > - streamBuffer->stream->putBuffer(streamBuffer->internalBuffer); > > > > - > > > > Camera3RequestDescriptor *request = streamBuffer->request; > > > > > > > > { > > > > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp > > > > index 52a3ac1f7..a9240a83c 100644 > > > > --- a/src/android/camera_request.cpp > > > > +++ b/src/android/camera_request.cpp > > > > @@ -10,6 +10,7 @@ > > > > #include <libcamera/base/span.h> > > > > > > > > #include "camera_buffer.h" > > > > +#include "camera_stream.h" > > > > > > > > using namespace libcamera; > > > > > > > > @@ -138,7 +139,14 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( > > > > request_ = camera->createRequest(reinterpret_cast<uint64_t>(this)); > > > > } > > > > > > > > -Camera3RequestDescriptor::~Camera3RequestDescriptor() = default; > > > > +Camera3RequestDescriptor::~Camera3RequestDescriptor() > > > > +{ > > > > + /* > > > > + * Recycle the allocated internal buffer back to its source stream. > > > > + */ > > > > > > Fits in one line most probably > > Done > > BR, > Harvey > > > > > > > > + for (auto &[sourceStream, frameBuffer] : internalBuffers_) > > > > + sourceStream->putBuffer(frameBuffer); > > > > +} > > > > > > > > /** > > > > * \class StreamBuffer > > > > @@ -166,9 +174,6 @@ Camera3RequestDescriptor::~Camera3RequestDescriptor() = default; > > > > * \var StreamBuffer::status > > > > * \brief Track the status of the buffer > > > > * > > > > - * \var StreamBuffer::internalBuffer > > > > - * \brief Pointer to a buffer internally handled by CameraStream (if any) > > > > - * > > > > * \var StreamBuffer::srcBuffer > > > > * \brief Pointer to the source frame buffer used for post-processing > > > > * > > > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h > > > > index 335f1985d..6b2a00795 100644 > > > > --- a/src/android/camera_request.h > > > > +++ b/src/android/camera_request.h > > > > @@ -49,7 +49,6 @@ public: > > > > std::unique_ptr<HALFrameBuffer> frameBuffer; > > > > libcamera::UniqueFD fence; > > > > Status status = Status::Success; > > > > - libcamera::FrameBuffer *internalBuffer = nullptr; > > > > const libcamera::FrameBuffer *srcBuffer = nullptr; > > > > std::unique_ptr<CameraBuffer> dstBuffer; > > > > Camera3RequestDescriptor *request; > > > > @@ -85,6 +84,8 @@ public: > > > > std::unique_ptr<libcamera::Request> request_; > > > > std::unique_ptr<CameraMetadata> resultMetadata_; > > > > > > > > + std::map<CameraStream *, libcamera::FrameBuffer *> internalBuffers_; > > > > + > > > > bool complete_ = false; > > > > Status status_ = Status::Success; > > > > > > > > -- > > Regards, > > > > Laurent Pinchart
Hi Jacopo, On Mon, Dec 2, 2024 at 11:39 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Harvey > > On Fri, Nov 29, 2024 at 03:51:39PM +0800, Cheng-Hao Yang wrote: > > Hi Jacopo and Laurent, > > > > On Thu, Nov 28, 2024 at 10:32 PM Laurent Pinchart > > <laurent.pinchart@ideasonboard.com> wrote: > > > > > > On Thu, Nov 28, 2024 at 03:24:44PM +0100, Jacopo Mondi wrote: > > > > Hi Harevy > > > > > > > > On Wed, Nov 27, 2024 at 09:25:54AM +0000, Harvey Yang wrote: > > > > > Previously there's a potential issue in StreamBuffer::internalBuffer's > > > > > lifetime, when more than one streams depend on the same internal buffer. > > > > > > > > s/.// > > > > > > > > lifetime, when more than one streams depend on the same internal buffer > > > > for post-processing, the buffer is returned to the CameraStream pool > > > > as soon as the first post-processing request has completed. > > > > Updated. > > > > > > > > > > > > > > > > This patch fixes the issue by returning the buffer when the whole > > > > > Camera3RequestDescriptor is destructed. > > > > > > > > Actually it does more I guess, it allows to map multiple streams of > > > > type ::Mapped to the same libcamera::Stream, doesn't it ? > > > > According to the previous implementation, I'd say that's already > > allowed, just not correctly implemented yet. > > Looking into [1], the condition doesn't care if the sourceStream > > has been mapped by another mapped stream or not. > > No but as you can see it is not supported as we immediately return the > source stream buffer once post-processing is done. > > However, something tells me this was by design as we allow a single > (Mapped) JPEG stream > > /* Defer handling of MJPEG streams until all others are known. */ > if (stream->format == HAL_PIXEL_FORMAT_BLOB) { > if (jpegStream) { > LOG(HAL, Error) > << "Multiple JPEG streams are not supported"; > return -EINVAL; > } > > jpegStream = stream; > continue; > } > > What is the use case you have for mapping multiple Mapped streams to > the same Direct|Internal one ? Multiple streams produced using libyuv > conversion ? (afaict that's the only other post-processing case we > support) Yes, and yes, we would expect at most one Internal stream and multiple Mapped streams with libyuv conversion I think. It's still allowed (to be configured) in the tot version, and has the issue of internalBuffer_ lifetime, right? That's why I think this patch is a pure issue fix. > > > > > Unless you mean that because there's such a bug, it's not allowed... > > I'd say that we should have a FATAL/ERROR log at least, or even > > block such a configuration. > > > > Well, this patch fixes it, doesn't it ? Yes :) > > > [1]: https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n1071 > > > > > > > > > > > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org> > > > > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > > > > > --- > > > > > src/android/camera_device.cpp | 63 +++++++++++++++++++--------------- > > > > > src/android/camera_request.cpp | 13 ++++--- > > > > > src/android/camera_request.h | 3 +- > > > > > 3 files changed, 46 insertions(+), 33 deletions(-) > > > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > > index 11613fac9..62f724041 100644 > > > > > --- a/src/android/camera_device.cpp > > > > > +++ b/src/android/camera_device.cpp > > > > > @@ -967,9 +967,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > > > * to a libcamera stream. Streams of type Mapped will be handled later. > > > > > * > > > > > * Collect the CameraStream associated to each requested capture stream. > > > > > - * Since requestedStreams is an std:map<>, no duplications can happen. > > > > > + * Since requestedDirectBuffers is an std:map<>, no duplications can > > > > > + * happen. > > > > > */ > > > > > - std::map<CameraStream *, libcamera::FrameBuffer *> requestedBuffers; > > > > > + std::map<CameraStream *, libcamera::FrameBuffer *> requestedDirectBuffers; > > > > > > > > Do you actually need to rename ? > > > > In this patch, this map only includes buffers from direct type. > > The internal type buffers are included by > > `Camera3RequestDescriptor::internalBuffers_` instead. > > > > let's call it directBuffers as a middle ground Sure, done. > > > If we don't exclude the internal ones in this map, we don't > > need to rename it as well. WDYT? > > > > > > > > > > > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > > > > > CameraStream *cameraStream = buffer.stream; > > > > > camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); > > > > > @@ -1009,7 +1010,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > > > frameBuffer = buffer.frameBuffer.get(); > > > > > acquireFence = std::move(buffer.fence); > > > > > > > > > > - requestedBuffers[cameraStream] = frameBuffer; > > > > > + requestedDirectBuffers[cameraStream] = frameBuffer; > > > > > LOG(HAL, Debug) << ss.str() << " (direct)"; > > > > > break; > > > > > > > > > > @@ -1018,14 +1019,17 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > > > * Get the frame buffer from the CameraStream internal > > > > > * buffer pool. > > > > > * > > > > > - * The buffer has to be returned to the CameraStream > > > > > - * once it has been processed. > > > > > + * The buffer will be returned to the CameraStream when > > > > > + * the request is destructed. > > > > > > > > or 'destroyed' > > > > Done > > > > > > > > > > > */ > > > > > frameBuffer = cameraStream->getBuffer(); > > > > > - buffer.internalBuffer = frameBuffer; > > > > > buffer.srcBuffer = frameBuffer; > > > > > > > > > > - requestedBuffers[cameraStream] = frameBuffer; > > > > > + /* > > > > > + * Track the allocated internal buffers, which will be > > > > > + * recycled when the descriptor destroyed. > > > > > + * */ > > > > > > > > Stray * */ > > > > Updated > > > > > > > > > > > + descriptor->internalBuffers_[cameraStream] = frameBuffer; > > > > > LOG(HAL, Debug) << ss.str() << " (internal)"; > > > > > > > > > > descriptor->pendingStreamsToProcess_.insert( > > > > > @@ -1079,24 +1083,41 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > > > * post-processing. No need to recycle the buffer since it's > > > > > * owned by Android. > > > > > */ > > > > > - auto iterDirectBuffer = requestedBuffers.find(sourceStream); > > > > > - if (iterDirectBuffer != requestedBuffers.end()) { > > > > > + auto iterDirectBuffer = requestedDirectBuffers.find(sourceStream); > > > > > + if (iterDirectBuffer != requestedDirectBuffers.end()) { > > > > > buffer.srcBuffer = iterDirectBuffer->second; > > > > > continue; > > > > > } > > > > > > > > > > /* > > > > > - * If that's not the case, we need to add a buffer to the request > > > > > - * for this stream. > > > > > + * If that's not the case, we use an internal buffer allocated > > > > > + * from the source stream. > > > > > + * > > Could you break this comment in two ? The first part apply to the rest > of the for() loop, the second part only to the following if() > > /* > * If that's not the case, we use an internal buffer allocated > * from the source stream. > */ > > /* > * If an internal buffer has been requested for the source > * stream before, we should reuse it. > */ > auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream); > if (iterInternalBuffer != descriptor->internalBuffers_.end()) { > buffer.srcBuffer = iterInternalBuffer->second; > continue; > } > > /* > * Otherwis`e, we need to create an internal buffer to the > * request for the source stream. Get the frame buffer from the > * source stream's internal buffer pool. > * > * The buffer will be returned to the CameraStream when the > * request is destructed. > */ > FrameBuffer *frameBuffer = sourceStream->getBuffer(); > buffer.srcBuffer = frameBuffer; > > descriptor->request_->addBuffer(sourceStream->stream(), > frameBuffer, nullptr); > Sure, done. > > > > > + * If an internal buffer has been requested for the source > > > > > + * stream before, we should reuse it. > > > > > + */ > > > > > + auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream); > > > > > + if (iterInternalBuffer != descriptor->internalBuffers_.end()) { > > > > > + buffer.srcBuffer = iterInternalBuffer->second; > > > > > + continue; > > > > > + } > > > > > + > > > > > + /* > > > > > + * Otherwise, we need to create an internal buffer to the > > > > > + * request for the source stream. Get the frame buffer from the > > > > > + * source stream's internal buffer pool. > > > > > + * > > > > > + * The buffer will be returned to the CameraStream when the > > > > > + * request is destructed. > > > > > */ > > > > > - FrameBuffer *frameBuffer = cameraStream->getBuffer(); > > > > > - buffer.internalBuffer = frameBuffer; > > > > > + FrameBuffer *frameBuffer = sourceStream->getBuffer(); > > > > > > > > Here I don't see why we're using the sourceStream's pool instead > > > > of the cameraStream's pool > > > > Because the buffer recycle process is different. In this patch, it'll be > > returned to the stream set to `internalBuffers_`. sourceStream is the > > correct one after this patch. > > > > Ok. As I understand this it is because, now that we allow multiple > Mapped stream to be ... mapped ... on a Direct stream. If we fall here > it's because the Direct stream its not part of the capture_request, so > we have to create a buffer from a pool and map multiple processed > streams on it. So yes the pool to use is the sourceStream one. Exactly :) > > > > > > > > > However, I think this goes in the direction of making the HAL capable > > > > of more things, and I'm looking at the diff of this and the previous > > > > patch combined and I think you should squash the two and make a commit > > > > about "Enabling multiple processed streams to map to the same internal > > > > buffer". > > > > Hmm, as I stated above. I think multiple processed streams to map to the same > > internal buffer is already allowed, just that it's not handled properly. > > Do you still think that we should squash the two into one? > > > > Looking at the combined diff it seems easier to make a single commit > along the lines of > > ------------------------------------------------------------------------------- > android: Correctly support multiple Mapped streams > > The Android camera HAL supports creating streams for the Android > framework by post-processing streams produced by libcamera. > > However at the moment a single Mapped stream can be associated with a > Direct stream because, after the first post-processing, the data from > the internal stream are returned preventing further post-processing > passes. > > Fix this by storing the list of Direct stream buffers used as the > post-processing source in an Camera3RequestDescriptor::internalBuffers_ > map to replace the single internalBuffer_ pointer and return the > internal buffers when the capture request is deleted. > ------------------------------------------------------------------------------- > > I'm sure this can be written a 1000 times better than this, but > at least, this is the level of detail and description a commit message > should provide Thanks, adopted. I'll try to provide more details next time... BR, Harvey > > > > > > > Processed streams are expensive, is there enough CPU power to make that > > > possible ? I suppose it wouldn't be an issue when producing, for > > > instance, a processed JPEG stream with a hardware encoder and a scaled > > > YUV stream with libyuv from the same stream. > > > > > > This is interesting, but isn't mentioned at all in the cover letter, and > > > goes well beyond the subject of the series. Is is an intentional change > > > ? > > > > Therefore, I took this as a fix of an issue, instead of adding a feature. > > > > Let me know if you prefer to merge this as a separate series. > > > > > > > > > Looking at the combined diff, it feels to me you could populate > > > > buffer.src for the Direct case in the switch() and keep using an > > > > Sorry, I don't get it. Mapped streams need to be handled the last, > > so that other streams and `requestedDirectBuffers` and > > `internalBuffers_` are setup, IIUC. > > > > No you're right, there's no buffer.srcBuffer to populate for Direct > > Thanks > j > > > > > std::set for requestedDirectBuffers (or requestedStreams). This would > > > > make the diff smaller probably. > > > > > > > > > > > > > buffer.srcBuffer = frameBuffer; > > > > > > > > > > descriptor->request_->addBuffer(sourceStream->stream(), > > > > > frameBuffer, nullptr); > > > > > > > > > > - requestedBuffers[sourceStream] = frameBuffer; > > > > > + /* Track the allocated internal buffer. */ > > > > > + descriptor->internalBuffers_[sourceStream] = frameBuffer; > > > > > } > > > > > > > > > > /* > > > > > @@ -1256,13 +1277,6 @@ void CameraDevice::requestComplete(Request *request) > > > > > if (ret) { > > > > > setBufferStatus(*buffer, StreamBuffer::Status::Error); > > > > > descriptor->pendingStreamsToProcess_.erase(stream); > > > > > - > > > > > - /* > > > > > - * If the framebuffer is internal to CameraStream return > > > > > - * it back now that we're done processing it. > > > > > - */ > > > > > - if (buffer->internalBuffer) > > > > > - stream->putBuffer(buffer->internalBuffer); > > > > > } > > > > > } > > > > > > > > > > @@ -1381,13 +1395,6 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer, > > > > > { > > > > > setBufferStatus(*streamBuffer, status); > > > > > > > > > > - /* > > > > > - * If the framebuffer is internal to CameraStream return it back now > > > > > - * that we're done processing it. > > > > > - */ > > > > > - if (streamBuffer->internalBuffer) > > > > > - streamBuffer->stream->putBuffer(streamBuffer->internalBuffer); > > > > > - > > > > > Camera3RequestDescriptor *request = streamBuffer->request; > > > > > > > > > > { > > > > > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp > > > > > index 52a3ac1f7..a9240a83c 100644 > > > > > --- a/src/android/camera_request.cpp > > > > > +++ b/src/android/camera_request.cpp > > > > > @@ -10,6 +10,7 @@ > > > > > #include <libcamera/base/span.h> > > > > > > > > > > #include "camera_buffer.h" > > > > > +#include "camera_stream.h" > > > > > > > > > > using namespace libcamera; > > > > > > > > > > @@ -138,7 +139,14 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( > > > > > request_ = camera->createRequest(reinterpret_cast<uint64_t>(this)); > > > > > } > > > > > > > > > > -Camera3RequestDescriptor::~Camera3RequestDescriptor() = default; > > > > > +Camera3RequestDescriptor::~Camera3RequestDescriptor() > > > > > +{ > > > > > + /* > > > > > + * Recycle the allocated internal buffer back to its source stream. > > > > > + */ > > > > > > > > Fits in one line most probably > > > > Done > > > > BR, > > Harvey > > > > > > > > > > > + for (auto &[sourceStream, frameBuffer] : internalBuffers_) > > > > > + sourceStream->putBuffer(frameBuffer); > > > > > +} > > > > > > > > > > /** > > > > > * \class StreamBuffer > > > > > @@ -166,9 +174,6 @@ Camera3RequestDescriptor::~Camera3RequestDescriptor() = default; > > > > > * \var StreamBuffer::status > > > > > * \brief Track the status of the buffer > > > > > * > > > > > - * \var StreamBuffer::internalBuffer > > > > > - * \brief Pointer to a buffer internally handled by CameraStream (if any) > > > > > - * > > > > > * \var StreamBuffer::srcBuffer > > > > > * \brief Pointer to the source frame buffer used for post-processing > > > > > * > > > > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h > > > > > index 335f1985d..6b2a00795 100644 > > > > > --- a/src/android/camera_request.h > > > > > +++ b/src/android/camera_request.h > > > > > @@ -49,7 +49,6 @@ public: > > > > > std::unique_ptr<HALFrameBuffer> frameBuffer; > > > > > libcamera::UniqueFD fence; > > > > > Status status = Status::Success; > > > > > - libcamera::FrameBuffer *internalBuffer = nullptr; > > > > > const libcamera::FrameBuffer *srcBuffer = nullptr; > > > > > std::unique_ptr<CameraBuffer> dstBuffer; > > > > > Camera3RequestDescriptor *request; > > > > > @@ -85,6 +84,8 @@ public: > > > > > std::unique_ptr<libcamera::Request> request_; > > > > > std::unique_ptr<CameraMetadata> resultMetadata_; > > > > > > > > > > + std::map<CameraStream *, libcamera::FrameBuffer *> internalBuffers_; > > > > > + > > > > > bool complete_ = false; > > > > > Status status_ = Status::Success; > > > > > > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 11613fac9..62f724041 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -967,9 +967,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * to a libcamera stream. Streams of type Mapped will be handled later. * * Collect the CameraStream associated to each requested capture stream. - * Since requestedStreams is an std:map<>, no duplications can happen. + * Since requestedDirectBuffers is an std:map<>, no duplications can + * happen. */ - std::map<CameraStream *, libcamera::FrameBuffer *> requestedBuffers; + std::map<CameraStream *, libcamera::FrameBuffer *> requestedDirectBuffers; for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { CameraStream *cameraStream = buffer.stream; camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); @@ -1009,7 +1010,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques frameBuffer = buffer.frameBuffer.get(); acquireFence = std::move(buffer.fence); - requestedBuffers[cameraStream] = frameBuffer; + requestedDirectBuffers[cameraStream] = frameBuffer; LOG(HAL, Debug) << ss.str() << " (direct)"; break; @@ -1018,14 +1019,17 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * Get the frame buffer from the CameraStream internal * buffer pool. * - * The buffer has to be returned to the CameraStream - * once it has been processed. + * The buffer will be returned to the CameraStream when + * the request is destructed. */ frameBuffer = cameraStream->getBuffer(); - buffer.internalBuffer = frameBuffer; buffer.srcBuffer = frameBuffer; - requestedBuffers[cameraStream] = frameBuffer; + /* + * Track the allocated internal buffers, which will be + * recycled when the descriptor destroyed. + * */ + descriptor->internalBuffers_[cameraStream] = frameBuffer; LOG(HAL, Debug) << ss.str() << " (internal)"; descriptor->pendingStreamsToProcess_.insert( @@ -1079,24 +1083,41 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques * post-processing. No need to recycle the buffer since it's * owned by Android. */ - auto iterDirectBuffer = requestedBuffers.find(sourceStream); - if (iterDirectBuffer != requestedBuffers.end()) { + auto iterDirectBuffer = requestedDirectBuffers.find(sourceStream); + if (iterDirectBuffer != requestedDirectBuffers.end()) { buffer.srcBuffer = iterDirectBuffer->second; continue; } /* - * If that's not the case, we need to add a buffer to the request - * for this stream. + * If that's not the case, we use an internal buffer allocated + * from the source stream. + * + * If an internal buffer has been requested for the source + * stream before, we should reuse it. + */ + auto iterInternalBuffer = descriptor->internalBuffers_.find(sourceStream); + if (iterInternalBuffer != descriptor->internalBuffers_.end()) { + buffer.srcBuffer = iterInternalBuffer->second; + continue; + } + + /* + * Otherwise, we need to create an internal buffer to the + * request for the source stream. Get the frame buffer from the + * source stream's internal buffer pool. + * + * The buffer will be returned to the CameraStream when the + * request is destructed. */ - FrameBuffer *frameBuffer = cameraStream->getBuffer(); - buffer.internalBuffer = frameBuffer; + FrameBuffer *frameBuffer = sourceStream->getBuffer(); buffer.srcBuffer = frameBuffer; descriptor->request_->addBuffer(sourceStream->stream(), frameBuffer, nullptr); - requestedBuffers[sourceStream] = frameBuffer; + /* Track the allocated internal buffer. */ + descriptor->internalBuffers_[sourceStream] = frameBuffer; } /* @@ -1256,13 +1277,6 @@ void CameraDevice::requestComplete(Request *request) if (ret) { setBufferStatus(*buffer, StreamBuffer::Status::Error); descriptor->pendingStreamsToProcess_.erase(stream); - - /* - * If the framebuffer is internal to CameraStream return - * it back now that we're done processing it. - */ - if (buffer->internalBuffer) - stream->putBuffer(buffer->internalBuffer); } } @@ -1381,13 +1395,6 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer, { setBufferStatus(*streamBuffer, status); - /* - * If the framebuffer is internal to CameraStream return it back now - * that we're done processing it. - */ - if (streamBuffer->internalBuffer) - streamBuffer->stream->putBuffer(streamBuffer->internalBuffer); - Camera3RequestDescriptor *request = streamBuffer->request; { diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp index 52a3ac1f7..a9240a83c 100644 --- a/src/android/camera_request.cpp +++ b/src/android/camera_request.cpp @@ -10,6 +10,7 @@ #include <libcamera/base/span.h> #include "camera_buffer.h" +#include "camera_stream.h" using namespace libcamera; @@ -138,7 +139,14 @@ Camera3RequestDescriptor::Camera3RequestDescriptor( request_ = camera->createRequest(reinterpret_cast<uint64_t>(this)); } -Camera3RequestDescriptor::~Camera3RequestDescriptor() = default; +Camera3RequestDescriptor::~Camera3RequestDescriptor() +{ + /* + * Recycle the allocated internal buffer back to its source stream. + */ + for (auto &[sourceStream, frameBuffer] : internalBuffers_) + sourceStream->putBuffer(frameBuffer); +} /** * \class StreamBuffer @@ -166,9 +174,6 @@ Camera3RequestDescriptor::~Camera3RequestDescriptor() = default; * \var StreamBuffer::status * \brief Track the status of the buffer * - * \var StreamBuffer::internalBuffer - * \brief Pointer to a buffer internally handled by CameraStream (if any) - * * \var StreamBuffer::srcBuffer * \brief Pointer to the source frame buffer used for post-processing * diff --git a/src/android/camera_request.h b/src/android/camera_request.h index 335f1985d..6b2a00795 100644 --- a/src/android/camera_request.h +++ b/src/android/camera_request.h @@ -49,7 +49,6 @@ public: std::unique_ptr<HALFrameBuffer> frameBuffer; libcamera::UniqueFD fence; Status status = Status::Success; - libcamera::FrameBuffer *internalBuffer = nullptr; const libcamera::FrameBuffer *srcBuffer = nullptr; std::unique_ptr<CameraBuffer> dstBuffer; Camera3RequestDescriptor *request; @@ -85,6 +84,8 @@ public: std::unique_ptr<libcamera::Request> request_; std::unique_ptr<CameraMetadata> resultMetadata_; + std::map<CameraStream *, libcamera::FrameBuffer *> internalBuffers_; + bool complete_ = false; Status status_ = Status::Success;