[libcamera-devel,v3,4/8] android: camera_device: Clear allocator at configureStream
diff mbox series

Message ID 20200922094738.5327-5-jacopo@jmondi.org
State Superseded
Headers show
Series
  • android: camera_device: Add support for internal buffers
Related show

Commit Message

Jacopo Mondi Sept. 22, 2020, 9:47 a.m. UTC 
The configureStream operation might be called by the Android framework
in two successive capture session without going through a close().

Clear all the allocated buffers before configuring the camera streams.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Kieran Bingham Sept. 23, 2020, 12:09 p.m. UTC | #1 
Hi Jacopo,

On 22/09/2020 10:47, Jacopo Mondi wrote:
> The configureStream operation might be called by the Android framework

s/might/could/
Not sure why, it just sounds better in my internal grammar parser.

> in two successive capture session without going through a close().

s/session/sessions/

> 
> Clear all the allocated buffers before configuring the camera streams.
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 98b8159ebd8c..42fb9ea4e113 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1182,12 +1182,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	}
>  
>  	/*
> -	 * Clear and remove any existing configuration from previous calls, and
> -	 * ensure the required entries are available without further
> -	 * reallocation.
> +	 * Clear and remove any existing configuration and memory allocated from
> +	 * previous calls, and ensure the required entries are available without
> +	 * further reallocation.
>  	 */
>  	streams_.clear();
>  	streams_.reserve(stream_list->num_streams);
> +	allocator_.clear();

>  
>  	/* First handle all non-MJPEG streams. */
>  	camera3_stream_t *jpegStream = nullptr;
>
Laurent Pinchart Sept. 29, 2020, 1:51 a.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.

On Tue, Sep 22, 2020 at 11:47:34AM +0200, Jacopo Mondi wrote:
> The configureStream operation might be called by the Android framework
> in two successive capture session without going through a close().

With s/session/sessions/,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Clear all the allocated buffers before configuring the camera streams.
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 98b8159ebd8c..42fb9ea4e113 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1182,12 +1182,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	}
>  
>  	/*
> -	 * Clear and remove any existing configuration from previous calls, and
> -	 * ensure the required entries are available without further
> -	 * reallocation.
> +	 * Clear and remove any existing configuration and memory allocated from
> +	 * previous calls, and ensure the required entries are available without
> +	 * further reallocation.
>  	 */
>  	streams_.clear();
>  	streams_.reserve(stream_list->num_streams);
> +	allocator_.clear();
>  
>  	/* First handle all non-MJPEG streams. */
>  	camera3_stream_t *jpegStream = nullptr;
Laurent Pinchart Sept. 29, 2020, 1:53 a.m. UTC | #3 
On Tue, Sep 29, 2020 at 04:51:32AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Tue, Sep 22, 2020 at 11:47:34AM +0200, Jacopo Mondi wrote:
> > The configureStream operation might be called by the Android framework
> > in two successive capture session without going through a close().
> 
> With s/session/sessions/,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Not necessarily for this patch, but shouldn't we also free buffers (and
probably clear streams_ too) in CameraDevice::close() ?

> > Clear all the allocated buffers before configuring the camera streams.
> > 
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 98b8159ebd8c..42fb9ea4e113 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1182,12 +1182,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  	}
> >  
> >  	/*
> > -	 * Clear and remove any existing configuration from previous calls, and
> > -	 * ensure the required entries are available without further
> > -	 * reallocation.
> > +	 * Clear and remove any existing configuration and memory allocated from
> > +	 * previous calls, and ensure the required entries are available without
> > +	 * further reallocation.
> >  	 */
> >  	streams_.clear();
> >  	streams_.reserve(stream_list->num_streams);
> > +	allocator_.clear();
> >  
> >  	/* First handle all non-MJPEG streams. */
> >  	camera3_stream_t *jpegStream = nullptr;
Jacopo Mondi Sept. 30, 2020, 11:04 a.m. UTC | #4 
Hi Laurent,

On Tue, Sep 29, 2020 at 04:53:31AM +0300, Laurent Pinchart wrote:
> On Tue, Sep 29, 2020 at 04:51:32AM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Tue, Sep 22, 2020 at 11:47:34AM +0200, Jacopo Mondi wrote:
> > > The configureStream operation might be called by the Android framework
> > > in two successive capture session without going through a close().
> >
> > With s/session/sessions/,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Not necessarily for this patch, but shouldn't we also free buffers (and
> probably clear streams_ too) in CameraDevice::close() ?

I guess it doesn't hurt, indeed. When the camera is closed it's
indeed relevant to release all the allocated buffers, even if any new
allocation happens at configureStream() time, where we already call
freeAll().

Regarding streams_, being that internal memory released at
configureStream() time before any other CameraStream is created, I
think it's less relevant... I can make a separate patch to do so on
top of this one.


>
> > > Clear all the allocated buffers before configuring the camera streams.
> > >
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  src/android/camera_device.cpp | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 98b8159ebd8c..42fb9ea4e113 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -1182,12 +1182,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > >  	}
> > >
> > >  	/*
> > > -	 * Clear and remove any existing configuration from previous calls, and
> > > -	 * ensure the required entries are available without further
> > > -	 * reallocation.
> > > +	 * Clear and remove any existing configuration and memory allocated from
> > > +	 * previous calls, and ensure the required entries are available without
> > > +	 * further reallocation.
> > >  	 */
> > >  	streams_.clear();
> > >  	streams_.reserve(stream_list->num_streams);
> > > +	allocator_.clear();
> > >
> > >  	/* First handle all non-MJPEG streams. */
> > >  	camera3_stream_t *jpegStream = nullptr;
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Oct. 2, 2020, 2 a.m. UTC | #5 
Hi Jacopo,

On Wed, Sep 30, 2020 at 01:04:53PM +0200, Jacopo Mondi wrote:
> On Tue, Sep 29, 2020 at 04:53:31AM +0300, Laurent Pinchart wrote:
> > On Tue, Sep 29, 2020 at 04:51:32AM +0300, Laurent Pinchart wrote:
> > > On Tue, Sep 22, 2020 at 11:47:34AM +0200, Jacopo Mondi wrote:
> > > > The configureStream operation might be called by the Android framework
> > > > in two successive capture session without going through a close().
> > >
> > > With s/session/sessions/,
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > Not necessarily for this patch, but shouldn't we also free buffers (and
> > probably clear streams_ too) in CameraDevice::close() ?
> 
> I guess it doesn't hurt, indeed. When the camera is closed it's
> indeed relevant to release all the allocated buffers, even if any new
> allocation happens at configureStream() time, where we already call
> freeAll().

If the camera is closed the buffers will remain there until the camera
is used again. It's not a memory leak as such as they will eventually be
freed, but we will hold on large pieces of memory that are completely
unused. Not very nice :-)

> Regarding streams_, being that internal memory released at
> configureStream() time before any other CameraStream is created, I
> think it's less relevant... I can make a separate patch to do so on
> top of this one.

streams_ is less of an issue as it's a fairly small amount of memory,
but I think cleaning up properly at close() time would be a good idea.

A separate patch is totally fine.

> > > > Clear all the allocated buffers before configuring the camera streams.
> > > >
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  src/android/camera_device.cpp | 7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > index 98b8159ebd8c..42fb9ea4e113 100644
> > > > --- a/src/android/camera_device.cpp
> > > > +++ b/src/android/camera_device.cpp
> > > > @@ -1182,12 +1182,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > > >  	}
> > > >
> > > >  	/*
> > > > -	 * Clear and remove any existing configuration from previous calls, and
> > > > -	 * ensure the required entries are available without further
> > > > -	 * reallocation.
> > > > +	 * Clear and remove any existing configuration and memory allocated from
> > > > +	 * previous calls, and ensure the required entries are available without
> > > > +	 * further reallocation.
> > > >  	 */
> > > >  	streams_.clear();
> > > >  	streams_.reserve(stream_list->num_streams);
> > > > +	allocator_.clear();
> > > >
> > > >  	/* First handle all non-MJPEG streams. */
> > > >  	camera3_stream_t *jpegStream = nullptr;

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 98b8159ebd8c..42fb9ea4e113 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1182,12 +1182,13 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 	}
 
 	/*
-	 * Clear and remove any existing configuration from previous calls, and
-	 * ensure the required entries are available without further
-	 * reallocation.
+	 * Clear and remove any existing configuration and memory allocated from
+	 * previous calls, and ensure the required entries are available without
+	 * further reallocation.
 	 */
 	streams_.clear();
 	streams_.reserve(stream_list->num_streams);
+	allocator_.clear();
 
 	/* First handle all non-MJPEG streams. */
 	camera3_stream_t *jpegStream = nullptr;