[RFC,v1,2/5] apps: cam: sdl_texture: Add single plane generic texture

23198 diff mbox series

Add the `SDLTexture1Plane` type that can be instantiated with
an arbitrary SDL pixel format and that uses `SDL_UpdateTexture()`
to update the texture using exactly a single plane.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/apps/cam/sdl_texture_1plane.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 src/apps/cam/sdl_texture_1plane.h

Hi Barnabás,

Thank you for the patch.

On Mon, Apr 21, 2025 at 05:51:06PM +0200, Barnabás Pőcze wrote:
> Add the `SDLTexture1Plane` type that can be instantiated with
> an arbitrary SDL pixel format and that uses `SDL_UpdateTexture()`
> to update the texture using exactly a single plane.

You can squash patches 2/5, 3/5 and 4/5 together. The commit message can
be reworded to explain how SDLTextureYUYV is generalized for later use
with other 1-plane formats.

> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/apps/cam/sdl_texture_1plane.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 src/apps/cam/sdl_texture_1plane.h
> 
> diff --git a/src/apps/cam/sdl_texture_1plane.h b/src/apps/cam/sdl_texture_1plane.h
> new file mode 100644
> index 000000000..ded35c589
> --- /dev/null
> +++ b/src/apps/cam/sdl_texture_1plane.h
> @@ -0,0 +1,18 @@
> +#pragma once
> +
> +#include <assert.h>
> +
> +#include "sdl_texture.h"
> +
> +class SDLTexture1Plane final : public SDLTexture
> +{
> +public:
> +	using SDLTexture::SDLTexture;
> +
> +	void update(libcamera::Span<const libcamera::Span<const uint8_t>> data) override
> +	{
> +		assert(data.size() == 1);
> +		assert(data[0].size_bytes() == std::size_t(rect_.h * stride_));
> +		SDL_UpdateTexture(ptr_, nullptr, data[0].data(), stride_);

No need to pass &rect_ as the second argument, like done by
SDLTextureYUYV ?

> +	}
> +};

Hi


2025. 04. 23. 1:40 keltezéssel, Laurent Pinchart írta:
> Hi Barnabás,
> 
> Thank you for the patch.
> 
> On Mon, Apr 21, 2025 at 05:51:06PM +0200, Barnabás Pőcze wrote:
>> Add the `SDLTexture1Plane` type that can be instantiated with
>> an arbitrary SDL pixel format and that uses `SDL_UpdateTexture()`
>> to update the texture using exactly a single plane.
> 
> You can squash patches 2/5, 3/5 and 4/5 together. The commit message can
> be reworded to explain how SDLTextureYUYV is generalized for later use
> with other 1-plane formats.
> 
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/apps/cam/sdl_texture_1plane.h | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>   create mode 100644 src/apps/cam/sdl_texture_1plane.h
>>
>> diff --git a/src/apps/cam/sdl_texture_1plane.h b/src/apps/cam/sdl_texture_1plane.h
>> new file mode 100644
>> index 000000000..ded35c589
>> --- /dev/null
>> +++ b/src/apps/cam/sdl_texture_1plane.h
>> @@ -0,0 +1,18 @@
>> +#pragma once
>> +
>> +#include <assert.h>
>> +
>> +#include "sdl_texture.h"
>> +
>> +class SDLTexture1Plane final : public SDLTexture
>> +{
>> +public:
>> +	using SDLTexture::SDLTexture;
>> +
>> +	void update(libcamera::Span<const libcamera::Span<const uint8_t>> data) override
>> +	{
>> +		assert(data.size() == 1);
>> +		assert(data[0].size_bytes() == std::size_t(rect_.h * stride_));
>> +		SDL_UpdateTexture(ptr_, nullptr, data[0].data(), stride_);
> 
> No need to pass &rect_ as the second argument, like done by
> SDLTextureYUYV ?

As far as I can see it is only required if only a part of the texture is to be updated,
and if it is `nullptr, then SDL updates the full size of the texture, which is what
is wanted here.

   https://github.com/libsdl-org/SDL/blob/3343cb21473ea454261bc4187393aaec9cc3f28f/src/render/SDL_render.c#L2153


Regards,
Barnabás Pőcze

> 
>> +	}
>> +};
>

Hi Barnabás,

On Thu, Apr 24, 2025 at 01:29:54PM +0200, Barnabás Pőcze wrote:
> 2025. 04. 23. 1:40 keltezéssel, Laurent Pinchart írta:
> > On Mon, Apr 21, 2025 at 05:51:06PM +0200, Barnabás Pőcze wrote:
> >> Add the `SDLTexture1Plane` type that can be instantiated with
> >> an arbitrary SDL pixel format and that uses `SDL_UpdateTexture()`
> >> to update the texture using exactly a single plane.
> > 
> > You can squash patches 2/5, 3/5 and 4/5 together. The commit message can
> > be reworded to explain how SDLTextureYUYV is generalized for later use
> > with other 1-plane formats.
> > 
> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >> ---
> >>   src/apps/cam/sdl_texture_1plane.h | 18 ++++++++++++++++++
> >>   1 file changed, 18 insertions(+)
> >>   create mode 100644 src/apps/cam/sdl_texture_1plane.h
> >>
> >> diff --git a/src/apps/cam/sdl_texture_1plane.h b/src/apps/cam/sdl_texture_1plane.h
> >> new file mode 100644
> >> index 000000000..ded35c589
> >> --- /dev/null
> >> +++ b/src/apps/cam/sdl_texture_1plane.h
> >> @@ -0,0 +1,18 @@
> >> +#pragma once
> >> +
> >> +#include <assert.h>
> >> +
> >> +#include "sdl_texture.h"
> >> +
> >> +class SDLTexture1Plane final : public SDLTexture
> >> +{
> >> +public:
> >> +	using SDLTexture::SDLTexture;
> >> +
> >> +	void update(libcamera::Span<const libcamera::Span<const uint8_t>> data) override
> >> +	{
> >> +		assert(data.size() == 1);
> >> +		assert(data[0].size_bytes() == std::size_t(rect_.h * stride_));
> >> +		SDL_UpdateTexture(ptr_, nullptr, data[0].data(), stride_);
> > 
> > No need to pass &rect_ as the second argument, like done by
> > SDLTextureYUYV ?
> 
> As far as I can see it is only required if only a part of the texture is to be updated,
> and if it is `nullptr, then SDL updates the full size of the texture, which is what
> is wanted here.
> 
>    https://github.com/libsdl-org/SDL/blob/3343cb21473ea454261bc4187393aaec9cc3f28f/src/render/SDL_render.c#L2153

Indeed. It seems it was a bug to pass &rect_ to the SDL_UpdateTexture()
call. It causes no issue in practice as rect_ always covers the full
size of the image in the current code.

Could you drop the rect argument in a separate patch ? It should also be
dropped from SDL_UpdateNVTexture(), which this series doesn't touch.

> >> +	}
> >> +};