| Message ID | 20210824142450.3157833-4-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, On Tue, Aug 24, 2021 at 03:24:49PM +0100, Kieran Bingham wrote: > The Buffer Allocation notes contains a TODO. Improve the notes regarding > the FrameBufferAllocation and remove the todo. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > simple-cam.cpp | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/simple-cam.cpp b/simple-cam.cpp > index 2e646a5485c9..47c5dc9987bc 100644 > --- a/simple-cam.cpp > +++ b/simple-cam.cpp > @@ -274,10 +274,16 @@ int main() > * Buffer Allocation > * > * Now that a camera has been configured, it knows all about its > - * Streams sizes and formats, so we now have to ask it to reserve > - * memory for all of them. > + * Streams sizes and formats. We need to provide buffers to store > + * captured images in. An application might choose to provide buffers > + * externally, for instance from a display driver which will render the > + * captured images, however libcamera can also provide buffers from a > + * configured camera. What about one paragraph for each of the two use cases ? * Streams sizes and formats. The captured images need to be stored in * memory buffers which can be either provided by the application to the * library, or allocated in the Camera memory and exposed to * the application by libcamera. * * An application can decide to allocate memory buffers from * elsewhere, in example in the memory of the display driver that * will render the captured frames, and provide them to * libcamera to capture images there. * * Alternatively libcamera can help the application by providing * buffers allocated in the Camera memory by the usage of a * FrameBufferAllocator instance, which references the * (configured) Camera for which the application wishes to * allocated buffers for. */ feel free to take whatever or ignore. Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + * > + * To request buffers from libcamera, we use the FrameBufferAllocator, > + * and reference the (configured) camera for which we wish to allocate > + * buffers for. > */ > - /* TODO: Update the comment here too */ > FrameBufferAllocator *allocator = new FrameBufferAllocator(camera); > > for (StreamConfiguration &cfg : *config) { > -- > 2.30.2 >
On 25/08/2021 10:21, Jacopo Mondi wrote: > Hi Kieran, > > On Tue, Aug 24, 2021 at 03:24:49PM +0100, Kieran Bingham wrote: >> The Buffer Allocation notes contains a TODO. Improve the notes regarding >> the FrameBufferAllocation and remove the todo. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> simple-cam.cpp | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/simple-cam.cpp b/simple-cam.cpp >> index 2e646a5485c9..47c5dc9987bc 100644 >> --- a/simple-cam.cpp >> +++ b/simple-cam.cpp >> @@ -274,10 +274,16 @@ int main() >> * Buffer Allocation >> * >> * Now that a camera has been configured, it knows all about its >> - * Streams sizes and formats, so we now have to ask it to reserve >> - * memory for all of them. >> + * Streams sizes and formats. We need to provide buffers to store >> + * captured images in. An application might choose to provide buffers >> + * externally, for instance from a display driver which will render the >> + * captured images, however libcamera can also provide buffers from a >> + * configured camera. > > What about one paragraph for each of the two use cases ? > > > * Streams sizes and formats. The captured images need to be stored in > * memory buffers which can be either provided by the application to the > * library, or allocated in the Camera memory and exposed to > * the application by libcamera. > * > * An application can decide to allocate memory buffers from > * elsewhere, in example in the memory of the display driver that > * will render the captured frames, and provide them to > * libcamera to capture images there. > * > * Alternatively libcamera can help the application by providing > * buffers allocated in the Camera memory by the usage of a > * FrameBufferAllocator instance, which references the > * (configured) Camera for which the application wishes to > * allocated buffers for. > */ > > feel free to take whatever or ignore. Trying to expand with your text, and fixing up a few parts I get this: > /* > * Now that a camera has been configured, it knows all about its > * Streams sizes and formats. The captured images need to be stored in > * memory buffers which can either be provided by the application to the > * library, or allocated in the Camera and exposed to the application by > * libcamera. > * > * An application may decide to allocate memory buffers from elsewhere, > * for example in memory allocated by the display driver that will > * render the captured frames. The application will provide them to > * libcamera by constructing FrameBuffer instances to capture images > * directly into. > * > * Alternatively libcamera can help the application by providing buffers > * allocated by the Camera using a FrameBufferAllocator instance and > * referencing a configured Camera to determine the appropriate buffer > * size and types to create. > */ Hows that? It feels a little bit repetitive, but I don't mind that if it expands more. > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > > >> + * >> + * To request buffers from libcamera, we use the FrameBufferAllocator, >> + * and reference the (configured) camera for which we wish to allocate >> + * buffers for. >> */ >> - /* TODO: Update the comment here too */ >> FrameBufferAllocator *allocator = new FrameBufferAllocator(camera); >> >> for (StreamConfiguration &cfg : *config) { >> -- >> 2.30.2 >>
Hi Kieran, On Wed, Aug 25, 2021 at 12:48:40PM +0100, Kieran Bingham wrote: > > > On 25/08/2021 10:21, Jacopo Mondi wrote: > > Hi Kieran, > > > > On Tue, Aug 24, 2021 at 03:24:49PM +0100, Kieran Bingham wrote: > >> The Buffer Allocation notes contains a TODO. Improve the notes regarding > >> the FrameBufferAllocation and remove the todo. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> simple-cam.cpp | 12 +++++++++--- > >> 1 file changed, 9 insertions(+), 3 deletions(-) > >> > >> diff --git a/simple-cam.cpp b/simple-cam.cpp > >> index 2e646a5485c9..47c5dc9987bc 100644 > >> --- a/simple-cam.cpp > >> +++ b/simple-cam.cpp > >> @@ -274,10 +274,16 @@ int main() > >> * Buffer Allocation > >> * > >> * Now that a camera has been configured, it knows all about its > >> - * Streams sizes and formats, so we now have to ask it to reserve > >> - * memory for all of them. > >> + * Streams sizes and formats. We need to provide buffers to store > >> + * captured images in. An application might choose to provide buffers > >> + * externally, for instance from a display driver which will render the > >> + * captured images, however libcamera can also provide buffers from a > >> + * configured camera. > > > > What about one paragraph for each of the two use cases ? > > > > > > * Streams sizes and formats. The captured images need to be stored in > > * memory buffers which can be either provided by the application to the > > * library, or allocated in the Camera memory and exposed to > > * the application by libcamera. > > * > > * An application can decide to allocate memory buffers from > > * elsewhere, in example in the memory of the display driver that > > * will render the captured frames, and provide them to > > * libcamera to capture images there. > > * > > * Alternatively libcamera can help the application by providing > > * buffers allocated in the Camera memory by the usage of a > > * FrameBufferAllocator instance, which references the > > * (configured) Camera for which the application wishes to > > * allocated buffers for. > > */ > > > > feel free to take whatever or ignore. > > > > Trying to expand with your text, and fixing up a few parts I get this: > Thanks for considering the suggestion. A few more comments > > /* > > * Now that a camera has been configured, it knows all about its > > * Streams sizes and formats. The captured images need to be stored in > > * memory buffers which can either be provided by the application to the > > * library, or allocated in the Camera and exposed to the application by > > * libcamera. > > * > > * An application may decide to allocate memory buffers from elsewhere, s/memory buffers/buffers to avoid the 'memory' repetition with the below 'memory allocated' > > * for example in memory allocated by the display driver that will > > * render the captured frames. The application will provide them to > > * libcamera by constructing FrameBuffer instances to capture images > > * directly into. > > * > > * Alternatively libcamera can help the application by providing buffers s/providing/exporting ? > > * allocated by the Camera using a FrameBufferAllocator instance and s/by/in/ ? > > * referencing a configured Camera to determine the appropriate buffer > > * size and types to create. > > */ > > Hows that? It feels a little bit repetitive, but I don't mind that if it > expands more. That's good! I think even if a bit pedantic, it's good to give more details. > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Feel free to push your current version or adjust it! Thanks j > > > > Thanks > > j > > > > > >> + * > >> + * To request buffers from libcamera, we use the FrameBufferAllocator, > >> + * and reference the (configured) camera for which we wish to allocate > >> + * buffers for. > >> */ > >> - /* TODO: Update the comment here too */ > >> FrameBufferAllocator *allocator = new FrameBufferAllocator(camera); > >> > >> for (StreamConfiguration &cfg : *config) { > >> -- > >> 2.30.2 > >>
Hello, On Wed, Aug 25, 2021 at 02:44:16PM +0200, Jacopo Mondi wrote: > On Wed, Aug 25, 2021 at 12:48:40PM +0100, Kieran Bingham wrote: > > On 25/08/2021 10:21, Jacopo Mondi wrote: > > > On Tue, Aug 24, 2021 at 03:24:49PM +0100, Kieran Bingham wrote: > > >> The Buffer Allocation notes contains a TODO. Improve the notes regarding > > >> the FrameBufferAllocation and remove the todo. > > >> > > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> --- > > >> simple-cam.cpp | 12 +++++++++--- > > >> 1 file changed, 9 insertions(+), 3 deletions(-) > > >> > > >> diff --git a/simple-cam.cpp b/simple-cam.cpp > > >> index 2e646a5485c9..47c5dc9987bc 100644 > > >> --- a/simple-cam.cpp > > >> +++ b/simple-cam.cpp > > >> @@ -274,10 +274,16 @@ int main() > > >> * Buffer Allocation > > >> * > > >> * Now that a camera has been configured, it knows all about its > > >> - * Streams sizes and formats, so we now have to ask it to reserve > > >> - * memory for all of them. > > >> + * Streams sizes and formats. We need to provide buffers to store > > >> + * captured images in. An application might choose to provide buffers > > >> + * externally, for instance from a display driver which will render the > > >> + * captured images, however libcamera can also provide buffers from a > > >> + * configured camera. > > > > > > What about one paragraph for each of the two use cases ? > > > > > > > > > * Streams sizes and formats. The captured images need to be stored in > > > * memory buffers which can be either provided by the application to the > > > * library, or allocated in the Camera memory and exposed to > > > * the application by libcamera. > > > * > > > * An application can decide to allocate memory buffers from > > > * elsewhere, in example in the memory of the display driver that > > > * will render the captured frames, and provide them to > > > * libcamera to capture images there. > > > * > > > * Alternatively libcamera can help the application by providing > > > * buffers allocated in the Camera memory by the usage of a > > > * FrameBufferAllocator instance, which references the > > > * (configured) Camera for which the application wishes to > > > * allocated buffers for. > > > */ > > > > > > feel free to take whatever or ignore. > > > > Trying to expand with your text, and fixing up a few parts I get this: > > Thanks for considering the suggestion. > A few more comments > > > > /* > > > * Now that a camera has been configured, it knows all about its > > > * Streams sizes and formats. The captured images need to be stored in > > > * memory buffers which can either be provided by the application to the > > > * library, or allocated in the Camera and exposed to the application by > > > * libcamera. > > > * > > > * An application may decide to allocate memory buffers from elsewhere, > > s/memory buffers/buffers > to avoid the 'memory' repetition with the below 'memory allocated' I'd say framebuffers, as that's the term we use in libcamera. Same above and below. > > > * for example in memory allocated by the display driver that will > > > * render the captured frames. The application will provide them to > > > * libcamera by constructing FrameBuffer instances to capture images > > > * directly into. > > > * > > > * Alternatively libcamera can help the application by providing buffers > > s/providing/exporting ? > > > > * allocated by the Camera using a FrameBufferAllocator instance and > > s/by/in/ ? > > > > * referencing a configured Camera to determine the appropriate buffer > > > * size and types to create. > > > */ > > > > Hows that? It feels a little bit repetitive, but I don't mind that if it > > expands more. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > That's good! I think even if a bit pedantic, it's good to give more > details. > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Feel free to push your current version or adjust it! > > > >> + * > > >> + * To request buffers from libcamera, we use the FrameBufferAllocator, > > >> + * and reference the (configured) camera for which we wish to allocate > > >> + * buffers for. > > >> */ > > >> - /* TODO: Update the comment here too */ > > >> FrameBufferAllocator *allocator = new FrameBufferAllocator(camera); > > >> > > >> for (StreamConfiguration &cfg : *config) {
diff --git a/simple-cam.cpp b/simple-cam.cpp index 2e646a5485c9..47c5dc9987bc 100644 --- a/simple-cam.cpp +++ b/simple-cam.cpp @@ -274,10 +274,16 @@ int main() * Buffer Allocation * * Now that a camera has been configured, it knows all about its - * Streams sizes and formats, so we now have to ask it to reserve - * memory for all of them. + * Streams sizes and formats. We need to provide buffers to store + * captured images in. An application might choose to provide buffers + * externally, for instance from a display driver which will render the + * captured images, however libcamera can also provide buffers from a + * configured camera. + * + * To request buffers from libcamera, we use the FrameBufferAllocator, + * and reference the (configured) camera for which we wish to allocate + * buffers for. */ - /* TODO: Update the comment here too */ FrameBufferAllocator *allocator = new FrameBufferAllocator(camera); for (StreamConfiguration &cfg : *config) {
The Buffer Allocation notes contains a TODO. Improve the notes regarding the FrameBufferAllocation and remove the todo. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- simple-cam.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)