| Message ID | 20211130112259.18723-2-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting David Plowman (2021-11-30 11:22:58) > The code was reducing the number of raw stream buffers allocated when > the application is providing some of its own. However, it was not > taking account of the fact that the application cannot supply embedded > data buffers, so it must always allocate a reasonable minimum number > of these buffers (possibly more than the number of raw stream buffers) > to prevent frame drops. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 25 ++++++++++++------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index e31fa3b2..045725dd 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1248,18 +1248,25 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > /* Decide how many internal buffers to allocate. */ > for (auto const stream : data->streams_) { > unsigned int numBuffers; > - > - if (stream == &data->unicam_[Unicam::Image] || > - stream == &data->unicam_[Unicam::Embedded]) { > + /* > + * For Unicam, allocate a minimum of 4 buffers as we want > + * to avoid any frame drops. > + */ > + constexpr unsigned int minBuffers = 4; > + if (stream == &data->unicam_[Unicam::Image]) { > /* > - * For Unicam, allocate a minimum of 4 buffers as we want > - * to avoid any frame drops. If an application has configured > - * a RAW stream, allocate additional buffers to make up the > - * minimum, but ensure we have at least 2 sets of internal > - * buffers to use to minimise frame drops. > + * If an application has configured a RAW stream, allocate > + * additional buffers to make up the minimum, but ensure > + * we have at least 2 sets of internal buffers to use to > + * minimise frame drops. > */ > - constexpr unsigned int minBuffers = 4; > numBuffers = std::max<int>(2, minBuffers - numRawBuffers); > + } else if (stream == &data->unicam_[Unicam::Embedded]) { > + /* > + * Embedded data buffers are (currently) for internal use, > + * so allocate the minimum required to avoid frame drops. > + */ > + numBuffers = minBuffers; Ouch, yes I see how this would have bitten. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > } else { > /* > * Since the ISP runs synchronous with the IPA and requests, > -- > 2.30.2 >
Hi David, Thank you for your patch. On Tue, 30 Nov 2021 at 11:23, David Plowman <david.plowman@raspberrypi.com> wrote: > The code was reducing the number of raw stream buffers allocated when > the application is providing some of its own. However, it was not > taking account of the fact that the application cannot supply embedded > data buffers, so it must always allocate a reasonable minimum number > of these buffers (possibly more than the number of raw stream buffers) > to prevent frame drops. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> --- > .../pipeline/raspberrypi/raspberrypi.cpp | 25 ++++++++++++------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index e31fa3b2..045725dd 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1248,18 +1248,25 @@ int PipelineHandlerRPi::prepareBuffers(Camera > *camera) > /* Decide how many internal buffers to allocate. */ > for (auto const stream : data->streams_) { > unsigned int numBuffers; > - > - if (stream == &data->unicam_[Unicam::Image] || > - stream == &data->unicam_[Unicam::Embedded]) { > + /* > + * For Unicam, allocate a minimum of 4 buffers as we want > + * to avoid any frame drops. > + */ > + constexpr unsigned int minBuffers = 4; > + if (stream == &data->unicam_[Unicam::Image]) { > /* > - * For Unicam, allocate a minimum of 4 buffers as > we want > - * to avoid any frame drops. If an application has > configured > - * a RAW stream, allocate additional buffers to > make up the > - * minimum, but ensure we have at least 2 sets of > internal > - * buffers to use to minimise frame drops. > + * If an application has configured a RAW stream, > allocate > + * additional buffers to make up the minimum, but > ensure > + * we have at least 2 sets of internal buffers to > use to > + * minimise frame drops. > */ > - constexpr unsigned int minBuffers = 4; > numBuffers = std::max<int>(2, minBuffers - > numRawBuffers); > + } else if (stream == &data->unicam_[Unicam::Embedded]) { > + /* > + * Embedded data buffers are (currently) for > internal use, > + * so allocate the minimum required to avoid frame > drops. > + */ > + numBuffers = minBuffers; > } else { > /* > * Since the ISP runs synchronous with the IPA and > requests, > -- > 2.30.2 > >
Hi David On Tue, Nov 30, 2021 at 11:22:58AM +0000, David Plowman wrote: > The code was reducing the number of raw stream buffers allocated when > the application is providing some of its own. However, it was not > taking account of the fact that the application cannot supply embedded > data buffers, so it must always allocate a reasonable minimum number > of these buffers (possibly more than the number of raw stream buffers) > to prevent frame drops. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 25 ++++++++++++------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index e31fa3b2..045725dd 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1248,18 +1248,25 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) > /* Decide how many internal buffers to allocate. */ > for (auto const stream : data->streams_) { > unsigned int numBuffers; > - > - if (stream == &data->unicam_[Unicam::Image] || > - stream == &data->unicam_[Unicam::Embedded]) { > + /* > + * For Unicam, allocate a minimum of 4 buffers as we want > + * to avoid any frame drops. > + */ > + constexpr unsigned int minBuffers = 4; > + if (stream == &data->unicam_[Unicam::Image]) { > /* > - * For Unicam, allocate a minimum of 4 buffers as we want > - * to avoid any frame drops. If an application has configured > - * a RAW stream, allocate additional buffers to make up the > - * minimum, but ensure we have at least 2 sets of internal > - * buffers to use to minimise frame drops. > + * If an application has configured a RAW stream, allocate > + * additional buffers to make up the minimum, but ensure > + * we have at least 2 sets of internal buffers to use to > + * minimise frame drops. > */ > - constexpr unsigned int minBuffers = 4; > numBuffers = std::max<int>(2, minBuffers - numRawBuffers); > + } else if (stream == &data->unicam_[Unicam::Embedded]) { > + /* > + * Embedded data buffers are (currently) for internal use, > + * so allocate the minimum required to avoid frame drops. > + */ > + numBuffers = minBuffers; > } else { > /* > * Since the ISP runs synchronous with the IPA and requests, > -- > 2.30.2 >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index e31fa3b2..045725dd 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1248,18 +1248,25 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera) /* Decide how many internal buffers to allocate. */ for (auto const stream : data->streams_) { unsigned int numBuffers; - - if (stream == &data->unicam_[Unicam::Image] || - stream == &data->unicam_[Unicam::Embedded]) { + /* + * For Unicam, allocate a minimum of 4 buffers as we want + * to avoid any frame drops. + */ + constexpr unsigned int minBuffers = 4; + if (stream == &data->unicam_[Unicam::Image]) { /* - * For Unicam, allocate a minimum of 4 buffers as we want - * to avoid any frame drops. If an application has configured - * a RAW stream, allocate additional buffers to make up the - * minimum, but ensure we have at least 2 sets of internal - * buffers to use to minimise frame drops. + * If an application has configured a RAW stream, allocate + * additional buffers to make up the minimum, but ensure + * we have at least 2 sets of internal buffers to use to + * minimise frame drops. */ - constexpr unsigned int minBuffers = 4; numBuffers = std::max<int>(2, minBuffers - numRawBuffers); + } else if (stream == &data->unicam_[Unicam::Embedded]) { + /* + * Embedded data buffers are (currently) for internal use, + * so allocate the minimum required to avoid frame drops. + */ + numBuffers = minBuffers; } else { /* * Since the ISP runs synchronous with the IPA and requests,
The code was reducing the number of raw stream buffers allocated when the application is providing some of its own. However, it was not taking account of the fact that the application cannot supply embedded data buffers, so it must always allocate a reasonable minimum number of these buffers (possibly more than the number of raw stream buffers) to prevent frame drops. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-)