| Message ID | 20220527093440.953377-4-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul On Fri, May 27, 2022 at 06:34:38PM +0900, Paul Elder wrote: > From: Jacopo Mondi <jacopo@jmondi.org> > > Mapped streams are generated by post-processing and always require a > source buffer from where to process image data from. > > In case a Mapped stream is requested but its source stream is not, it > is required to allocate a buffer on the fly and add it to the > libcamera::Request. > > Make sure a source stream is available for all mapped streams, and if > that's not the case, add a dedicated buffer to the request for that > purpose. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > Changes in v2: > - cosmetic changes in code > - fix typo in the commit message > --- > src/android/camera_device.cpp | 80 +++++++++++++++++++++++++++++++---- > 1 file changed, 72 insertions(+), 8 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 20599665..95c14f60 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -9,6 +9,7 @@ > > #include <algorithm> > #include <fstream> > +#include <set> > #include <sys/mman.h> > #include <unistd.h> > #include <vector> > @@ -923,6 +924,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() > << " with " << descriptor->buffers_.size() << " streams"; > > + /* > + * Collect the CameraStream associated to each requested capture stream. > + * Since requestedStreams is an std:set<>, no duplications can happen. std::set<> > + */ > + std::set<CameraStream *> requestedStreams; > + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > + CameraStream *cameraStream = buffer.stream; > + > + switch (cameraStream->type()) { > + case CameraStream::Type::Mapped: > + requestedStreams.insert(cameraStream->sourceStream()); > + break; > + > + case CameraStream::Type::Direct: > + case CameraStream::Type::Internal: > + requestedStreams.insert(cameraStream); > + break; > + } > + } > + > + /* > + * Process all the Direct and Internal streams, for which the CameraStream > + * they refer to is the one that points to the right libcamera::Stream. > + * > + * Streams of type Mapped will be handled later. > + */ > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > CameraStream *cameraStream = buffer.stream; > camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); > @@ -945,14 +972,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > switch (cameraStream->type()) { > case CameraStream::Type::Mapped: > - /* > - * Mapped streams don't need buffers added to the > - * Request. > - */ > - LOG(HAL, Debug) << ss.str() << " (mapped)"; > - > - descriptor->pendingStreamsToProcess_.insert( > - { cameraStream, &buffer }); > continue; > > case CameraStream::Type::Direct: > @@ -996,6 +1015,51 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > auto fence = std::make_unique<Fence>(std::move(acquireFence)); > descriptor->request_->addBuffer(cameraStream->stream(), > frameBuffer, std::move(fence)); > + > + requestedStreams.erase(cameraStream); > + } > + > + /* > + * Now handle the Mapped streams. If no buffer has been added for them > + * as their corresponding direct source stream has not been requested, > + * add it here. > + */ > + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > + CameraStream *cameraStream = buffer.stream; > + camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); > + > + if (cameraStream->type() != CameraStream::Type::Mapped) > + continue; > + > + LOG(HAL, Debug) << i << " - (" << camera3Stream->width << "x" > + << camera3Stream->height << ")" > + << "[" << utils::hex(camera3Stream->format) << "] -> " > + << "(" << cameraStream->configuration().size.toString() << ")[" > + << cameraStream->configuration().pixelFormat.toString() << "]" > + << " (mapped)"; > + > + MutexLocker lock(descriptor->streamsProcessMutex_); > + descriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer }); > + > + /* > + * Make sure the CameraStream this stream is mapped on has been > + * added to the request. > + */ > + CameraStream *sourceStream = cameraStream->sourceStream(); > + if (requestedStreams.find(sourceStream) == requestedStreams.end()) > + continue; > + > + /* > + * If that's not the case, we need to add a buffer to the request > + * for this stream. > + */ > + FrameBuffer *frameBuffer = cameraStream->getBuffer(); > + buffer.internalBuffer = frameBuffer; > + > + descriptor->request_->addBuffer(sourceStream->stream(), > + frameBuffer, nullptr); > + > + requestedStreams.erase(sourceStream); > } We could possibily make sure here that now requestedStreams is empty, but that's just an additional safety check. The patch still looks ok to me. > > /* > -- > 2.30.2 >
Hi Jacopo, On 5/30/22 11:56, Jacopo Mondi via libcamera-devel wrote: > Hi Paul > > On Fri, May 27, 2022 at 06:34:38PM +0900, Paul Elder wrote: >> From: Jacopo Mondi <jacopo@jmondi.org> >> >> Mapped streams are generated by post-processing and always require a >> source buffer from where to process image data from. >> >> In case a Mapped stream is requested but its source stream is not, it >> is required to allocate a buffer on the fly and add it to the >> libcamera::Request. >> >> Make sure a source stream is available for all mapped streams, and if >> that's not the case, add a dedicated buffer to the request for that >> purpose. >> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> >> >> --- >> Changes in v2: >> - cosmetic changes in code >> - fix typo in the commit message >> --- >> src/android/camera_device.cpp | 80 +++++++++++++++++++++++++++++++---- >> 1 file changed, 72 insertions(+), 8 deletions(-) >> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >> index 20599665..95c14f60 100644 >> --- a/src/android/camera_device.cpp >> +++ b/src/android/camera_device.cpp >> @@ -9,6 +9,7 @@ >> >> #include <algorithm> >> #include <fstream> >> +#include <set> >> #include <sys/mman.h> >> #include <unistd.h> >> #include <vector> >> @@ -923,6 +924,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >> LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() >> << " with " << descriptor->buffers_.size() << " streams"; >> >> + /* >> + * Collect the CameraStream associated to each requested capture stream. >> + * Since requestedStreams is an std:set<>, no duplications can happen. > std::set<> > >> + */ >> + std::set<CameraStream *> requestedStreams; >> + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { >> + CameraStream *cameraStream = buffer.stream; >> + >> + switch (cameraStream->type()) { >> + case CameraStream::Type::Mapped: >> + requestedStreams.insert(cameraStream->sourceStream()); Can sourceStream field for Mapped streams be nullptr here? Should we ensure it via an ASSERT? >> + break; >> + >> + case CameraStream::Type::Direct: >> + case CameraStream::Type::Internal: >> + requestedStreams.insert(cameraStream); >> + break; >> + } >> + } >> + >> + /* >> + * Process all the Direct and Internal streams, for which the CameraStream >> + * they refer to is the one that points to the right libcamera::Stream. >> + * >> + * Streams of type Mapped will be handled later. >> + */ >> for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { >> CameraStream *cameraStream = buffer.stream; >> camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); >> @@ -945,14 +972,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >> >> switch (cameraStream->type()) { >> case CameraStream::Type::Mapped: >> - /* >> - * Mapped streams don't need buffers added to the >> - * Request. >> - */ >> - LOG(HAL, Debug) << ss.str() << " (mapped)"; >> - >> - descriptor->pendingStreamsToProcess_.insert( >> - { cameraStream, &buffer }); >> continue; >> >> case CameraStream::Type::Direct: >> @@ -996,6 +1015,51 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >> auto fence = std::make_unique<Fence>(std::move(acquireFence)); >> descriptor->request_->addBuffer(cameraStream->stream(), >> frameBuffer, std::move(fence)); >> + >> + requestedStreams.erase(cameraStream); >> + } >> + >> + /* >> + * Now handle the Mapped streams. If no buffer has been added for them >> + * as their corresponding direct source stream has not been requested, >> + * add it here. I am wondering a situation where a direct stream D, and a mapped stream M, is requested and M is mapped onto D so, M->sourceStream = D; Provided the requestedStreams is a std::set<> where no duplications can happen, and given the above scenario: I see the requestedStreams will consist of { D } while populating the set in the switch-case above, which then gets erased from the requestedStreams (above the comment block) so now, the requestedStreams become an empty set { } here . . . >> + */ >> + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { >> + CameraStream *cameraStream = buffer.stream; >> + camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); >> + >> + if (cameraStream->type() != CameraStream::Type::Mapped) >> + continue; >> + >> + LOG(HAL, Debug) << i << " - (" << camera3Stream->width << "x" >> + << camera3Stream->height << ")" >> + << "[" << utils::hex(camera3Stream->format) << "] -> " >> + << "(" << cameraStream->configuration().size.toString() << ")[" >> + << cameraStream->configuration().pixelFormat.toString() << "]" >> + << " (mapped)"; >> + >> + MutexLocker lock(descriptor->streamsProcessMutex_); >> + descriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer }); >> + >> + /* >> + * Make sure the CameraStream this stream is mapped on has been >> + * added to the request. >> + */ >> + CameraStream *sourceStream = cameraStream->sourceStream(); >> + if (requestedStreams.find(sourceStream) == requestedStreams.end()) and then while handling mapped streams, it will try to find { D } in requestedStreams (which is now empty) >> + continue; >> + >> + /* >> + * If that's not the case, we need to add a buffer to the request >> + * for this stream. >> + */ >> + FrameBuffer *frameBuffer = cameraStream->getBuffer(); >> + buffer.internalBuffer = frameBuffer; >> + >> + descriptor->request_->addBuffer(sourceStream->stream(), >> + frameBuffer, nullptr); ... and add a internal buffer for D so we have 2 buffers for D in the end ? >> + >> + requestedStreams.erase(sourceStream); >> } > We could possibily make sure here that now requestedStreams is empty, > but that's just an additional safety check. > > The patch still looks ok to me. > >> /* >> -- >> 2.30.2 >>
Hi Umang, On Mon, May 30, 2022 at 03:41:20PM +0200, Umang Jain wrote: > Hi Jacopo, > > On 5/30/22 11:56, Jacopo Mondi via libcamera-devel wrote: > > Hi Paul > > > > On Fri, May 27, 2022 at 06:34:38PM +0900, Paul Elder wrote: > > > From: Jacopo Mondi <jacopo@jmondi.org> > > > > > > Mapped streams are generated by post-processing and always require a > > > source buffer from where to process image data from. > > > > > > In case a Mapped stream is requested but its source stream is not, it > > > is required to allocate a buffer on the fly and add it to the > > > libcamera::Request. > > > > > > Make sure a source stream is available for all mapped streams, and if > > > that's not the case, add a dedicated buffer to the request for that > > > purpose. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > > --- > > > Changes in v2: > > > - cosmetic changes in code > > > - fix typo in the commit message > > > --- > > > src/android/camera_device.cpp | 80 +++++++++++++++++++++++++++++++---- > > > 1 file changed, 72 insertions(+), 8 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 20599665..95c14f60 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -9,6 +9,7 @@ > > > > > > #include <algorithm> > > > #include <fstream> > > > +#include <set> > > > #include <sys/mman.h> > > > #include <unistd.h> > > > #include <vector> > > > @@ -923,6 +924,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() > > > << " with " << descriptor->buffers_.size() << " streams"; > > > > > > + /* > > > + * Collect the CameraStream associated to each requested capture stream. > > > + * Since requestedStreams is an std:set<>, no duplications can happen. > > std::set<> > > > > > + */ > > > + std::set<CameraStream *> requestedStreams; > > > + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > > > + CameraStream *cameraStream = buffer.stream; > > > + > > > + switch (cameraStream->type()) { > > > + case CameraStream::Type::Mapped: > > > + requestedStreams.insert(cameraStream->sourceStream()); > > > Can sourceStream field for Mapped streams be nullptr here? Should we ensure > it via an ASSERT? > > > > + break; > > > + > > > + case CameraStream::Type::Direct: > > > + case CameraStream::Type::Internal: > > > + requestedStreams.insert(cameraStream); > > > + break; > > > + } > > > + } > > > + > > > + /* > > > + * Process all the Direct and Internal streams, for which the CameraStream > > > + * they refer to is the one that points to the right libcamera::Stream. > > > + * > > > + * Streams of type Mapped will be handled later. > > > + */ > > > for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > > > CameraStream *cameraStream = buffer.stream; > > > camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); > > > @@ -945,14 +972,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > > > > switch (cameraStream->type()) { > > > case CameraStream::Type::Mapped: > > > - /* > > > - * Mapped streams don't need buffers added to the > > > - * Request. > > > - */ > > > - LOG(HAL, Debug) << ss.str() << " (mapped)"; > > > - > > > - descriptor->pendingStreamsToProcess_.insert( > > > - { cameraStream, &buffer }); > > > continue; > > > > > > case CameraStream::Type::Direct: > > > @@ -996,6 +1015,51 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > > > auto fence = std::make_unique<Fence>(std::move(acquireFence)); > > > descriptor->request_->addBuffer(cameraStream->stream(), > > > frameBuffer, std::move(fence)); > > > + > > > + requestedStreams.erase(cameraStream); > > > + } > > > + > > > + /* > > > + * Now handle the Mapped streams. If no buffer has been added for them > > > + * as their corresponding direct source stream has not been requested, > > > + * add it here. > > > I am wondering a situation where a direct stream D, and a mapped stream M, > is requested and M is mapped onto D so, > > M->sourceStream = D; > > Provided the requestedStreams is a std::set<> where no duplications can > happen, and given the above scenario: > I see the requestedStreams will consist of { D } while populating the set > in the switch-case above, which then gets > erased from the requestedStreams (above the comment block) so now, the > requestedStreams become an empty set { } here . . . > > > > + */ > > > + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > > > + CameraStream *cameraStream = buffer.stream; > > > + camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); > > > + > > > + if (cameraStream->type() != CameraStream::Type::Mapped) > > > + continue; > > > + > > > + LOG(HAL, Debug) << i << " - (" << camera3Stream->width << "x" > > > + << camera3Stream->height << ")" > > > + << "[" << utils::hex(camera3Stream->format) << "] -> " > > > + << "(" << cameraStream->configuration().size.toString() << ")[" > > > + << cameraStream->configuration().pixelFormat.toString() << "]" > > > + << " (mapped)"; > > > + > > > + MutexLocker lock(descriptor->streamsProcessMutex_); > > > + descriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer }); > > > + > > > + /* > > > + * Make sure the CameraStream this stream is mapped on has been > > > + * added to the request. > > > + */ > > > + CameraStream *sourceStream = cameraStream->sourceStream(); > > > + if (requestedStreams.find(sourceStream) == requestedStreams.end()) > > > and then while handling mapped streams, it will try to find { D } in > requestedStreams (which is now empty) > > > > + continue; So that the find() operation returns == requestedStreams.end() and we continue here > > > + > > > + /* > > > + * If that's not the case, we need to add a buffer to the request > > > + * for this stream. > > > + */ > > > + FrameBuffer *frameBuffer = cameraStream->getBuffer(); > > > + buffer.internalBuffer = frameBuffer; > > > + > > > + descriptor->request_->addBuffer(sourceStream->stream(), > > > + frameBuffer, nullptr); > > > ... and add a internal buffer for D > > so we have 2 buffers for D in the end ? So we don't get here and won't add the buffer twice. Have I missed something on you comment ? > > > > + > > > + requestedStreams.erase(sourceStream); > > > } > > We could possibily make sure here that now requestedStreams is empty, > > but that's just an additional safety check. > > > > The patch still looks ok to me. > > > > > /* > > > -- > > > 2.30.2 > > >
Hi Jacopo, On 5/30/22 16:27, Jacopo Mondi wrote: > Hi Umang, > > On Mon, May 30, 2022 at 03:41:20PM +0200, Umang Jain wrote: >> Hi Jacopo, >> >> On 5/30/22 11:56, Jacopo Mondi via libcamera-devel wrote: >>> Hi Paul >>> >>> On Fri, May 27, 2022 at 06:34:38PM +0900, Paul Elder wrote: >>>> From: Jacopo Mondi <jacopo@jmondi.org> >>>> >>>> Mapped streams are generated by post-processing and always require a >>>> source buffer from where to process image data from. >>>> >>>> In case a Mapped stream is requested but its source stream is not, it >>>> is required to allocate a buffer on the fly and add it to the >>>> libcamera::Request. >>>> >>>> Make sure a source stream is available for all mapped streams, and if >>>> that's not the case, add a dedicated buffer to the request for that >>>> purpose. >>>> >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> >>>> >>>> --- >>>> Changes in v2: >>>> - cosmetic changes in code >>>> - fix typo in the commit message >>>> --- >>>> src/android/camera_device.cpp | 80 +++++++++++++++++++++++++++++++---- >>>> 1 file changed, 72 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp >>>> index 20599665..95c14f60 100644 >>>> --- a/src/android/camera_device.cpp >>>> +++ b/src/android/camera_device.cpp >>>> @@ -9,6 +9,7 @@ >>>> >>>> #include <algorithm> >>>> #include <fstream> >>>> +#include <set> >>>> #include <sys/mman.h> >>>> #include <unistd.h> >>>> #include <vector> >>>> @@ -923,6 +924,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >>>> LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() >>>> << " with " << descriptor->buffers_.size() << " streams"; >>>> >>>> + /* >>>> + * Collect the CameraStream associated to each requested capture stream. >>>> + * Since requestedStreams is an std:set<>, no duplications can happen. >>> std::set<> >>> >>>> + */ >>>> + std::set<CameraStream *> requestedStreams; >>>> + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { >>>> + CameraStream *cameraStream = buffer.stream; >>>> + >>>> + switch (cameraStream->type()) { >>>> + case CameraStream::Type::Mapped: >>>> + requestedStreams.insert(cameraStream->sourceStream()); >> >> Can sourceStream field for Mapped streams be nullptr here? Should we ensure >> it via an ASSERT? Would like your thoughts on this as well.. >> >>>> + break; >>>> + >>>> + case CameraStream::Type::Direct: >>>> + case CameraStream::Type::Internal: >>>> + requestedStreams.insert(cameraStream); >>>> + break; >>>> + } >>>> + } >>>> + >>>> + /* >>>> + * Process all the Direct and Internal streams, for which the CameraStream >>>> + * they refer to is the one that points to the right libcamera::Stream. >>>> + * >>>> + * Streams of type Mapped will be handled later. >>>> + */ >>>> for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { >>>> CameraStream *cameraStream = buffer.stream; >>>> camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); >>>> @@ -945,14 +972,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >>>> >>>> switch (cameraStream->type()) { >>>> case CameraStream::Type::Mapped: >>>> - /* >>>> - * Mapped streams don't need buffers added to the >>>> - * Request. >>>> - */ >>>> - LOG(HAL, Debug) << ss.str() << " (mapped)"; >>>> - >>>> - descriptor->pendingStreamsToProcess_.insert( >>>> - { cameraStream, &buffer }); >>>> continue; >>>> >>>> case CameraStream::Type::Direct: >>>> @@ -996,6 +1015,51 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques >>>> auto fence = std::make_unique<Fence>(std::move(acquireFence)); >>>> descriptor->request_->addBuffer(cameraStream->stream(), >>>> frameBuffer, std::move(fence)); >>>> + >>>> + requestedStreams.erase(cameraStream); >>>> + } >>>> + >>>> + /* >>>> + * Now handle the Mapped streams. If no buffer has been added for them >>>> + * as their corresponding direct source stream has not been requested, >>>> + * add it here. >> >> I am wondering a situation where a direct stream D, and a mapped stream M, >> is requested and M is mapped onto D so, >> >> M->sourceStream = D; >> >> Provided the requestedStreams is a std::set<> where no duplications can >> happen, and given the above scenario: >> I see the requestedStreams will consist of { D } while populating the set >> in the switch-case above, which then gets >> erased from the requestedStreams (above the comment block) so now, the >> requestedStreams become an empty set { } here . . . >> >>>> + */ >>>> + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { >>>> + CameraStream *cameraStream = buffer.stream; >>>> + camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); >>>> + >>>> + if (cameraStream->type() != CameraStream::Type::Mapped) >>>> + continue; >>>> + >>>> + LOG(HAL, Debug) << i << " - (" << camera3Stream->width << "x" >>>> + << camera3Stream->height << ")" >>>> + << "[" << utils::hex(camera3Stream->format) << "] -> " >>>> + << "(" << cameraStream->configuration().size.toString() << ")[" >>>> + << cameraStream->configuration().pixelFormat.toString() << "]" >>>> + << " (mapped)"; >>>> + >>>> + MutexLocker lock(descriptor->streamsProcessMutex_); >>>> + descriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer }); >>>> + >>>> + /* >>>> + * Make sure the CameraStream this stream is mapped on has been >>>> + * added to the request. >>>> + */ >>>> + CameraStream *sourceStream = cameraStream->sourceStream(); >>>> + if (requestedStreams.find(sourceStream) == requestedStreams.end()) >> >> and then while handling mapped streams, it will try to find { D } in >> requestedStreams (which is now empty) >> >>>> + continue; > So that the find() operation returns == requestedStreams.end() and we Ah .. so silly of me thinking the comparator as != requestedStreams.end() instead of == requestedStreams.end(). So code patterns do stick in the brain :D > continue here > >>>> + >>>> + /* >>>> + * If that's not the case, we need to add a buffer to the request >>>> + * for this stream. >>>> + */ >>>> + FrameBuffer *frameBuffer = cameraStream->getBuffer(); >>>> + buffer.internalBuffer = frameBuffer; >>>> + >>>> + descriptor->request_->addBuffer(sourceStream->stream(), >>>> + frameBuffer, nullptr); >> >> ... and add a internal buffer for D >> >> so we have 2 buffers for D in the end ? > So we don't get here and won't add the buffer twice. Makes sense now > > Have I missed something on you comment ? No. It looks good to me now! > >>>> + >>>> + requestedStreams.erase(sourceStream); >>>> } >>> We could possibily make sure here that now requestedStreams is empty, >>> but that's just an additional safety check. >>> >>> The patch still looks ok to me. >>> >>>> /* >>>> -- >>>> 2.30.2 >>>>
Hi Jacopo, Just to re-iterate on point, On 5/30/22 21:58, Umang Jain via libcamera-devel wrote: > Hi Jacopo, > > On 5/30/22 16:27, Jacopo Mondi wrote: >> Hi Umang, >> >> On Mon, May 30, 2022 at 03:41:20PM +0200, Umang Jain wrote: >>> Hi Jacopo, >>> >>> On 5/30/22 11:56, Jacopo Mondi via libcamera-devel wrote: >>>> Hi Paul >>>> >>>> On Fri, May 27, 2022 at 06:34:38PM +0900, Paul Elder wrote: >>>>> From: Jacopo Mondi <jacopo@jmondi.org> >>>>> >>>>> Mapped streams are generated by post-processing and always require a >>>>> source buffer from where to process image data from. >>>>> >>>>> In case a Mapped stream is requested but its source stream is not, it >>>>> is required to allocate a buffer on the fly and add it to the >>>>> libcamera::Request. >>>>> >>>>> Make sure a source stream is available for all mapped streams, and if >>>>> that's not the case, add a dedicated buffer to the request for that >>>>> purpose. >>>>> >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>>>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> >>>>> >>>>> --- >>>>> Changes in v2: >>>>> - cosmetic changes in code >>>>> - fix typo in the commit message >>>>> --- >>>>> src/android/camera_device.cpp | 80 >>>>> +++++++++++++++++++++++++++++++---- >>>>> 1 file changed, 72 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/src/android/camera_device.cpp >>>>> b/src/android/camera_device.cpp >>>>> index 20599665..95c14f60 100644 >>>>> --- a/src/android/camera_device.cpp >>>>> +++ b/src/android/camera_device.cpp >>>>> @@ -9,6 +9,7 @@ >>>>> >>>>> #include <algorithm> >>>>> #include <fstream> >>>>> +#include <set> >>>>> #include <sys/mman.h> >>>>> #include <unistd.h> >>>>> #include <vector> >>>>> @@ -923,6 +924,32 @@ int >>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t >>>>> *camera3Reques >>>>> LOG(HAL, Debug) << "Queueing request " << >>>>> descriptor->request_->cookie() >>>>> << " with " << descriptor->buffers_.size() << " >>>>> streams"; >>>>> >>>>> + /* >>>>> + * Collect the CameraStream associated to each requested >>>>> capture stream. >>>>> + * Since requestedStreams is an std:set<>, no duplications >>>>> can happen. >>>> std::set<> >>>> >>>>> + */ >>>>> + std::set<CameraStream *> requestedStreams; >>>>> + for (const auto &[i, buffer] : >>>>> utils::enumerate(descriptor->buffers_)) { >>>>> + CameraStream *cameraStream = buffer.stream; >>>>> + >>>>> + switch (cameraStream->type()) { >>>>> + case CameraStream::Type::Mapped: >>>>> + requestedStreams.insert(cameraStream->sourceStream()); >>> >>> Can sourceStream field for Mapped streams be nullptr here? Should we >>> ensure >>> it via an ASSERT? > > > Would like your thoughts on this as well.. Probably we should merge in with the ASSERT(cameraStream->sourceStream() != nullptr) as a safety check. That gives me a bit of mental peace: With that, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > >>> >>>>> + break; >>>>> + >>>>> + case CameraStream::Type::Direct: >>>>> + case CameraStream::Type::Internal: >>>>> + requestedStreams.insert(cameraStream); >>>>> + break; >>>>> + } >>>>> + } >>>>> + >>>>> + /* >>>>> + * Process all the Direct and Internal streams, for which the >>>>> CameraStream >>>>> + * they refer to is the one that points to the right >>>>> libcamera::Stream. >>>>> + * >>>>> + * Streams of type Mapped will be handled later. >>>>> + */ >>>>> for (const auto &[i, buffer] : >>>>> utils::enumerate(descriptor->buffers_)) { >>>>> CameraStream *cameraStream = buffer.stream; >>>>> camera3_stream_t *camera3Stream = >>>>> cameraStream->camera3Stream(); >>>>> @@ -945,14 +972,6 @@ int >>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t >>>>> *camera3Reques >>>>> >>>>> switch (cameraStream->type()) { >>>>> case CameraStream::Type::Mapped: >>>>> - /* >>>>> - * Mapped streams don't need buffers added to the >>>>> - * Request. >>>>> - */ >>>>> - LOG(HAL, Debug) << ss.str() << " (mapped)"; >>>>> - >>>>> - descriptor->pendingStreamsToProcess_.insert( >>>>> - { cameraStream, &buffer }); >>>>> continue; >>>>> >>>>> case CameraStream::Type::Direct: >>>>> @@ -996,6 +1015,51 @@ int >>>>> CameraDevice::processCaptureRequest(camera3_capture_request_t >>>>> *camera3Reques >>>>> auto fence = >>>>> std::make_unique<Fence>(std::move(acquireFence)); >>>>> descriptor->request_->addBuffer(cameraStream->stream(), >>>>> frameBuffer, std::move(fence)); >>>>> + >>>>> + requestedStreams.erase(cameraStream); >>>>> + } >>>>> + >>>>> + /* >>>>> + * Now handle the Mapped streams. If no buffer has been added >>>>> for them >>>>> + * as their corresponding direct source stream has not been >>>>> requested, >>>>> + * add it here. >>> >>> I am wondering a situation where a direct stream D, and a mapped >>> stream M, >>> is requested and M is mapped onto D so, >>> >>> M->sourceStream = D; >>> >>> Provided the requestedStreams is a std::set<> where no duplications can >>> happen, and given the above scenario: >>> I see the requestedStreams will consist of { D } while populating >>> the set >>> in the switch-case above, which then gets >>> erased from the requestedStreams (above the comment block) so now, the >>> requestedStreams become an empty set { } here . . . >>> >>>>> + */ >>>>> + for (const auto &[i, buffer] : >>>>> utils::enumerate(descriptor->buffers_)) { >>>>> + CameraStream *cameraStream = buffer.stream; >>>>> + camera3_stream_t *camera3Stream = >>>>> cameraStream->camera3Stream(); >>>>> + >>>>> + if (cameraStream->type() != CameraStream::Type::Mapped) >>>>> + continue; >>>>> + >>>>> + LOG(HAL, Debug) << i << " - (" << camera3Stream->width << >>>>> "x" >>>>> + << camera3Stream->height << ")" >>>>> + << "[" << utils::hex(camera3Stream->format) << "] >>>>> -> " >>>>> + << "(" << >>>>> cameraStream->configuration().size.toString() << ")[" >>>>> + << >>>>> cameraStream->configuration().pixelFormat.toString() << "]" >>>>> + << " (mapped)"; >>>>> + >>>>> + MutexLocker lock(descriptor->streamsProcessMutex_); >>>>> + descriptor->pendingStreamsToProcess_.insert({ >>>>> cameraStream, &buffer }); >>>>> + >>>>> + /* >>>>> + * Make sure the CameraStream this stream is mapped on >>>>> has been >>>>> + * added to the request. >>>>> + */ >>>>> + CameraStream *sourceStream = cameraStream->sourceStream(); >>>>> + if (requestedStreams.find(sourceStream) == >>>>> requestedStreams.end()) >>> >>> and then while handling mapped streams, it will try to find { D } in >>> requestedStreams (which is now empty) >>> >>>>> + continue; >> So that the find() operation returns == requestedStreams.end() and we > > > Ah .. so silly of me thinking the comparator as != requestedStreams.end() > instead of == requestedStreams.end(). So code patterns do stick in the > brain :D > >> continue here >> >>>>> + >>>>> + /* >>>>> + * If that's not the case, we need to add a buffer to the >>>>> request >>>>> + * for this stream. >>>>> + */ >>>>> + FrameBuffer *frameBuffer = cameraStream->getBuffer(); >>>>> + buffer.internalBuffer = frameBuffer; >>>>> + >>>>> + descriptor->request_->addBuffer(sourceStream->stream(), >>>>> + frameBuffer, nullptr); >>> >>> ... and add a internal buffer for D >>> >>> so we have 2 buffers for D in the end ? >> So we don't get here and won't add the buffer twice. > > > Makes sense now > >> >> Have I missed something on you comment ? > > > No. It looks good to me now! > >> >>>>> + >>>>> + requestedStreams.erase(sourceStream); >>>>> } >>>> We could possibily make sure here that now requestedStreams is empty, >>>> but that's just an additional safety check. >>>> >>>> The patch still looks ok to me. >>>> >>>>> /* >>>>> -- >>>>> 2.30.2 >>>>>
On Thu, Jun 02, 2022 at 09:32:13AM +0200, Umang Jain via libcamera-devel wrote: > On 5/30/22 21:58, Umang Jain via libcamera-devel wrote: > > On 5/30/22 16:27, Jacopo Mondi wrote: > >> On Mon, May 30, 2022 at 03:41:20PM +0200, Umang Jain wrote: > >>> On 5/30/22 11:56, Jacopo Mondi via libcamera-devel wrote: > >>>> On Fri, May 27, 2022 at 06:34:38PM +0900, Paul Elder wrote: > >>>>> From: Jacopo Mondi <jacopo@jmondi.org> > >>>>> > >>>>> Mapped streams are generated by post-processing and always require a > >>>>> source buffer from where to process image data from. s/from where // > >>>>> > >>>>> In case a Mapped stream is requested but its source stream is not, it > >>>>> is required to allocate a buffer on the fly and add it to the > >>>>> libcamera::Request. > >>>>> > >>>>> Make sure a source stream is available for all mapped streams, and if > >>>>> that's not the case, add a dedicated buffer to the request for that > >>>>> purpose. > >>>>> > >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > >>>>> > >>>>> --- > >>>>> Changes in v2: > >>>>> - cosmetic changes in code > >>>>> - fix typo in the commit message > >>>>> --- > >>>>> src/android/camera_device.cpp | 80 +++++++++++++++++++++++++++++++---- > >>>>> 1 file changed, 72 insertions(+), 8 deletions(-) > >>>>> > >>>>> diff --git a/src/android/camera_device.cpp > >>>>> b/src/android/camera_device.cpp > >>>>> index 20599665..95c14f60 100644 > >>>>> --- a/src/android/camera_device.cpp > >>>>> +++ b/src/android/camera_device.cpp > >>>>> @@ -9,6 +9,7 @@ > >>>>> > >>>>> #include <algorithm> > >>>>> #include <fstream> > >>>>> +#include <set> > >>>>> #include <sys/mman.h> > >>>>> #include <unistd.h> > >>>>> #include <vector> > >>>>> @@ -923,6 +924,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > >>>>> LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() > >>>>> << " with " << descriptor->buffers_.size() << " streams"; > >>>>> > >>>>> + /* > >>>>> + * Collect the CameraStream associated to each requested capture stream. > >>>>> + * Since requestedStreams is an std:set<>, no duplications can happen. > >>>> std::set<> > >>>> > >>>>> + */ > >>>>> + std::set<CameraStream *> requestedStreams; > >>>>> + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > >>>>> + CameraStream *cameraStream = buffer.stream; > >>>>> + > >>>>> + switch (cameraStream->type()) { > >>>>> + case CameraStream::Type::Mapped: > >>>>> + requestedStreams.insert(cameraStream->sourceStream()); > >>> > >>> Can sourceStream field for Mapped streams be nullptr here? Should we ensure > >>> it via an ASSERT? > > > > Would like your thoughts on this as well.. > > Probably we should merge in with the ASSERT(cameraStream->sourceStream() > != nullptr) as a safety check. > That gives me a bit of mental peace: It shouldn't be null, so an assert is enough if it helps you sleeping :-) > With that, > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > >>>>> + break; > >>>>> + > >>>>> + case CameraStream::Type::Direct: > >>>>> + case CameraStream::Type::Internal: > >>>>> + requestedStreams.insert(cameraStream); > >>>>> + break; > >>>>> + } > >>>>> + } > >>>>> + > >>>>> + /* > >>>>> + * Process all the Direct and Internal streams, for which the CameraStream > >>>>> + * they refer to is the one that points to the right libcamera::Stream. I have trouble parsing this, what is "the right libcamera::Stream" ? Do you mean something along the lines of /* * Process all the Direct and Internal streams first, they map directly * to a libcamera stream. */ > >>>>> + * > >>>>> + * Streams of type Mapped will be handled later. > >>>>> + */ > >>>>> for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > >>>>> CameraStream *cameraStream = buffer.stream; > >>>>> camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); > >>>>> @@ -945,14 +972,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > >>>>> > >>>>> switch (cameraStream->type()) { > >>>>> case CameraStream::Type::Mapped: > >>>>> - /* > >>>>> - * Mapped streams don't need buffers added to the > >>>>> - * Request. > >>>>> - */ > >>>>> - LOG(HAL, Debug) << ss.str() << " (mapped)"; > >>>>> - > >>>>> - descriptor->pendingStreamsToProcess_.insert( > >>>>> - { cameraStream, &buffer }); Let's keep a comment: /* Mapped streams will be handled in the next loop. */ > >>>>> continue; > >>>>> > >>>>> case CameraStream::Type::Direct: > >>>>> @@ -996,6 +1015,51 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques > >>>>> auto fence = std::make_unique<Fence>(std::move(acquireFence)); > >>>>> descriptor->request_->addBuffer(cameraStream->stream(), > >>>>> frameBuffer, std::move(fence)); > >>>>> + > >>>>> + requestedStreams.erase(cameraStream); > >>>>> + } > >>>>> + > >>>>> + /* > >>>>> + * Now handle the Mapped streams. If no buffer has been added for them > >>>>> + * as their corresponding direct source stream has not been requested, > >>>>> + * add it here. * Now handle the Mapped streams. If no buffer has been added for them, * because their corresponding direct source stream is not part of this * particular request, add one here. > >>> > >>> I am wondering a situation where a direct stream D, and a mapped stream M, > >>> is requested and M is mapped onto D so, > >>> > >>> M->sourceStream = D; > >>> > >>> Provided the requestedStreams is a std::set<> where no duplications can > >>> happen, and given the above scenario: > >>> I see the requestedStreams will consist of { D } while populating the set > >>> in the switch-case above, which then gets > >>> erased from the requestedStreams (above the comment block) so now, the > >>> requestedStreams become an empty set { } here . . . > >>> > >>>>> + */ > >>>>> + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { > >>>>> + CameraStream *cameraStream = buffer.stream; > >>>>> + camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); > >>>>> + > >>>>> + if (cameraStream->type() != CameraStream::Type::Mapped) > >>>>> + continue; > >>>>> + > >>>>> + LOG(HAL, Debug) << i << " - (" << camera3Stream->width << "x" > >>>>> + << camera3Stream->height << ")" > >>>>> + << "[" << utils::hex(camera3Stream->format) << "] -> " > >>>>> + << "(" << cameraStream->configuration().size.toString() << ")[" > >>>>> + << cameraStream->configuration().pixelFormat.toString() << "]" We can now drop the .toString() :-) > >>>>> + << " (mapped)"; > >>>>> + > >>>>> + MutexLocker lock(descriptor->streamsProcessMutex_); > >>>>> + descriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer }); > >>>>> + > >>>>> + /* > >>>>> + * Make sure the CameraStream this stream is mapped on has been s/mapped on/mapped to/ > >>>>> + * added to the request. > >>>>> + */ > >>>>> + CameraStream *sourceStream = cameraStream->sourceStream(); > >>>>> + if (requestedStreams.find(sourceStream) == requestedStreams.end()) > >>> > >>> and then while handling mapped streams, it will try to find { D } in > >>> requestedStreams (which is now empty) > >>> > >>>>> + continue; > >> > >> So that the find() operation returns == requestedStreams.end() and we > > > > Ah .. so silly of me thinking the comparator as != requestedStreams.end() > > instead of == requestedStreams.end(). So code patterns do stick in the > > brain :D Part of the confusion may come from the variable name. It contains the streams that haven't been added to the request yet. How about renaming it to unhandledStreams ? Thinking more about it, couldn't we simplify the code by inverting the logic ? It seems we could drop the first loop that populates requestedStreams, and instead add streams to requestedStreams in the loop that handles the Direct and Internal streams. The check here would then become != requestedStreams.end(), and below the erase() would be turned into an insert(). Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> continue here > >> > >>>>> + > >>>>> + /* > >>>>> + * If that's not the case, we need to add a buffer to the request > >>>>> + * for this stream. > >>>>> + */ > >>>>> + FrameBuffer *frameBuffer = cameraStream->getBuffer(); > >>>>> + buffer.internalBuffer = frameBuffer; > >>>>> + > >>>>> + descriptor->request_->addBuffer(sourceStream->stream(), > >>>>> + frameBuffer, nullptr); > >>> > >>> ... and add a internal buffer for D > >>> > >>> so we have 2 buffers for D in the end ? > >> > >> So we don't get here and won't add the buffer twice. > > > > Makes sense now > > > >> Have I missed something on you comment ? > > > > No. It looks good to me now! > > > >>>>> + > >>>>> + requestedStreams.erase(sourceStream); > >>>>> } > >>>> > >>>> We could possibily make sure here that now requestedStreams is empty, > >>>> but that's just an additional safety check. > >>>> > >>>> The patch still looks ok to me. > >>>> > >>>>> /*
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 20599665..95c14f60 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -9,6 +9,7 @@ #include <algorithm> #include <fstream> +#include <set> #include <sys/mman.h> #include <unistd.h> #include <vector> @@ -923,6 +924,32 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques LOG(HAL, Debug) << "Queueing request " << descriptor->request_->cookie() << " with " << descriptor->buffers_.size() << " streams"; + /* + * Collect the CameraStream associated to each requested capture stream. + * Since requestedStreams is an std:set<>, no duplications can happen. + */ + std::set<CameraStream *> requestedStreams; + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { + CameraStream *cameraStream = buffer.stream; + + switch (cameraStream->type()) { + case CameraStream::Type::Mapped: + requestedStreams.insert(cameraStream->sourceStream()); + break; + + case CameraStream::Type::Direct: + case CameraStream::Type::Internal: + requestedStreams.insert(cameraStream); + break; + } + } + + /* + * Process all the Direct and Internal streams, for which the CameraStream + * they refer to is the one that points to the right libcamera::Stream. + * + * Streams of type Mapped will be handled later. + */ for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { CameraStream *cameraStream = buffer.stream; camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); @@ -945,14 +972,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques switch (cameraStream->type()) { case CameraStream::Type::Mapped: - /* - * Mapped streams don't need buffers added to the - * Request. - */ - LOG(HAL, Debug) << ss.str() << " (mapped)"; - - descriptor->pendingStreamsToProcess_.insert( - { cameraStream, &buffer }); continue; case CameraStream::Type::Direct: @@ -996,6 +1015,51 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques auto fence = std::make_unique<Fence>(std::move(acquireFence)); descriptor->request_->addBuffer(cameraStream->stream(), frameBuffer, std::move(fence)); + + requestedStreams.erase(cameraStream); + } + + /* + * Now handle the Mapped streams. If no buffer has been added for them + * as their corresponding direct source stream has not been requested, + * add it here. + */ + for (const auto &[i, buffer] : utils::enumerate(descriptor->buffers_)) { + CameraStream *cameraStream = buffer.stream; + camera3_stream_t *camera3Stream = cameraStream->camera3Stream(); + + if (cameraStream->type() != CameraStream::Type::Mapped) + continue; + + LOG(HAL, Debug) << i << " - (" << camera3Stream->width << "x" + << camera3Stream->height << ")" + << "[" << utils::hex(camera3Stream->format) << "] -> " + << "(" << cameraStream->configuration().size.toString() << ")[" + << cameraStream->configuration().pixelFormat.toString() << "]" + << " (mapped)"; + + MutexLocker lock(descriptor->streamsProcessMutex_); + descriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer }); + + /* + * Make sure the CameraStream this stream is mapped on has been + * added to the request. + */ + CameraStream *sourceStream = cameraStream->sourceStream(); + if (requestedStreams.find(sourceStream) == requestedStreams.end()) + continue; + + /* + * If that's not the case, we need to add a buffer to the request + * for this stream. + */ + FrameBuffer *frameBuffer = cameraStream->getBuffer(); + buffer.internalBuffer = frameBuffer; + + descriptor->request_->addBuffer(sourceStream->stream(), + frameBuffer, nullptr); + + requestedStreams.erase(sourceStream); } /*