[libcamera-devel,12/30] libcamera: v4l2_videodevice: Remove assertion involving BufferPool

2360 diff mbox series

The BufferPool class will be removed when the buffer logic is reworked.
During the transition some functions will be shared between the new and
old buffer code and removing this assert allows sharing of the
dequeueBuffer() function.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/v4l2_videodevice.cpp | 1 -
 1 file changed, 1 deletion(-)

Hi Niklas,

On Wed, Nov 27, 2019 at 12:36:02AM +0100, Niklas Söderlund wrote:
> The BufferPool class will be removed when the buffer logic is reworked.
> During the transition some functions will be shared between the new and
> old buffer code and removing this assert allows sharing of the
> dequeueBuffer() function.
>

This single line change alone doesn't make much sense to me.
I would like to see it where it is actually required. Anyway

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/v4l2_videodevice.cpp | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 7b6fa5347ef320f8..644e4545a2f33b2e 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1100,7 +1100,6 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
>  		return nullptr;
>  	}
>
> -	ASSERT(buf.index < bufferPool_->count());
>  	LOG(V4L2, Debug) << "Buffer " << buf.index << " is available";
>
>  	auto it = queuedBuffers_.find(buf.index);
> --
> 2.24.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

Thank you for the patch.

On Wed, Nov 27, 2019 at 03:50:04PM +0100, Jacopo Mondi wrote:
> On Wed, Nov 27, 2019 at 12:36:02AM +0100, Niklas Söderlund wrote:
> > The BufferPool class will be removed when the buffer logic is reworked.
> > During the transition some functions will be shared between the new and
> > old buffer code and removing this assert allows sharing of the
> > dequeueBuffer() function.
> 
> This single line change alone doesn't make much sense to me.
> I would like to see it where it is actually required. Anyway

I agree with Jacopo, please squash this with the patch that requires it.
It's actually counter-productive this split this out from a review point
of view as it forces me to figure out which patch triggered this change.

> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/libcamera/v4l2_videodevice.cpp | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 7b6fa5347ef320f8..644e4545a2f33b2e 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1100,7 +1100,6 @@ Buffer *V4L2VideoDevice::dequeueBuffer()
> >  		return nullptr;
> >  	}
> >
> > -	ASSERT(buf.index < bufferPool_->count());
> >  	LOG(V4L2, Debug) << "Buffer " << buf.index << " is available";
> >
> >  	auto it = queuedBuffers_.find(buf.index);