[v1] libcamera: framebuffer: Fix cookie arg type

26633 diff mbox series

The cookie has type `uint64_t`, but the constructor argument is of type
`unsigned int`. Fix that.

Unfortunately this is a public API and ABI change.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 include/libcamera/framebuffer.h | 2 +-
 src/libcamera/framebuffer.cpp   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

On Tue, May 05, 2026 at 10:33:32AM +0200, Barnabás Pőcze wrote:
> The cookie has type `uint64_t`, but the constructor argument is of type
> `unsigned int`. Fix that.
> 
> Unfortunately this is a public API and ABI change.

We just need to convince Linus to rename v7.2 to v8.0 to stay in sync
:-)

> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/framebuffer.h | 2 +-
>  src/libcamera/framebuffer.cpp   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> index 723525d05..c633d22a6 100644
> --- a/include/libcamera/framebuffer.h
> +++ b/include/libcamera/framebuffer.h
> @@ -58,7 +58,7 @@ public:
>  		unsigned int length;
>  	};
>  
> -	FrameBuffer(Span<const Plane> planes, unsigned int cookie = 0);
> +	FrameBuffer(Span<const Plane> planes, uint64_t cookie = 0);
>  	FrameBuffer(std::unique_ptr<Private> d);
>  	virtual ~FrameBuffer() {}
>  
> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index 765dab95a..d61319030 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -315,7 +315,7 @@ ino_t fileDescriptorInode(const SharedFD &fd)
>   * \param[in] planes The frame memory planes
>   * \param[in] cookie Cookie
>   */
> -FrameBuffer::FrameBuffer(Span<const Plane> planes, unsigned int cookie)
> +FrameBuffer::FrameBuffer(Span<const Plane> planes, uint64_t cookie)
>  	: FrameBuffer(std::make_unique<Private>(planes, cookie))
>  {
>  }

This looks good, but I wonder if it's enough. The cookie is also passed
to IPA modules as a way to identify buffers. It is used by mapBuffers(),
unmapBuffers(), fillParams() and processStats(), and IPA modules
currently use a 32-bit value.

As we only map stats and params buffers in IPA modules, and those are
always created by pipeline handlers, there's a guarantee the cookies
will fit in 32 bits. Still, this may be a time bomb, maybe we should fix
it now instead of waiting for weird bugs ?

There's no point in delaying this patch, so

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

The IPA issue can be fixed on top.

Quoting Laurent Pinchart (2026-05-05 22:31:25)
> On Tue, May 05, 2026 at 10:33:32AM +0200, Barnabás Pőcze wrote:
> > The cookie has type `uint64_t`, but the constructor argument is of type
> > `unsigned int`. Fix that.
> > 
> > Unfortunately this is a public API and ABI change.
> 
> We just need to convince Linus to rename v7.2 to v8.0 to stay in sync
> :-)
> 
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > ---
> >  include/libcamera/framebuffer.h | 2 +-
> >  src/libcamera/framebuffer.cpp   | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> > index 723525d05..c633d22a6 100644
> > --- a/include/libcamera/framebuffer.h
> > +++ b/include/libcamera/framebuffer.h
> > @@ -58,7 +58,7 @@ public:
> >               unsigned int length;
> >       };
> >  
> > -     FrameBuffer(Span<const Plane> planes, unsigned int cookie = 0);
> > +     FrameBuffer(Span<const Plane> planes, uint64_t cookie = 0);

I see the other usages in public API indeed use a uint64_t, so this
definitely makes sense.

In my burden to try to protect the ABI, could we either put this on an
'accepted for next ABI breakage branch' (maybe we'll end up with a
libcamera/next ?)

or we could simply add the new uint64_t constructor now, and keep the
unsigned int cookie constructor as a call to the uint64_t type ? That
would prevent ABI breakage ?

We could then drop the deprecated constructor later.


> >       FrameBuffer(std::unique_ptr<Private> d);
> >       virtual ~FrameBuffer() {}
> >  
> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > index 765dab95a..d61319030 100644
> > --- a/src/libcamera/framebuffer.cpp
> > +++ b/src/libcamera/framebuffer.cpp
> > @@ -315,7 +315,7 @@ ino_t fileDescriptorInode(const SharedFD &fd)
> >   * \param[in] planes The frame memory planes
> >   * \param[in] cookie Cookie
> >   */
> > -FrameBuffer::FrameBuffer(Span<const Plane> planes, unsigned int cookie)
> > +FrameBuffer::FrameBuffer(Span<const Plane> planes, uint64_t cookie)
> >       : FrameBuffer(std::make_unique<Private>(planes, cookie))
> >  {
> >  }
> 
> This looks good, but I wonder if it's enough. The cookie is also passed
> to IPA modules as a way to identify buffers. It is used by mapBuffers(),
> unmapBuffers(), fillParams() and processStats(), and IPA modules
> currently use a 32-bit value.
> 
> As we only map stats and params buffers in IPA modules, and those are
> always created by pipeline handlers, there's a guarantee the cookies
> will fit in 32 bits. Still, this may be a time bomb, maybe we should fix
> it now instead of waiting for weird bugs ?
> 
> There's no point in delaying this patch, so

Except the ABI breakage!



> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> The IPA issue can be fixed on top.
> 
> -- 
> Regards,
> 
> Laurent Pinchart