[libcamera-devel,1/4] cam: sdl_texture_yuyv: Make line stride configurable
diff mbox series

Message ID 20220807021718.9789-2-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • cam: Add support for NV12 in the SDL sink
Related show

Commit Message

Laurent Pinchart Aug. 7, 2022, 2:17 a.m. UTC 
The line stride of the texture is currently hardcoded based on the image
width, which may not match the real stride. Use the stride value from
the stream configuration instead.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/cam/sdl_sink.cpp         | 2 +-
 src/cam/sdl_texture_yuyv.cpp | 4 ++--
 src/cam/sdl_texture_yuyv.h   | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

Comments

Kieran Bingham Aug. 7, 2022, 10:21 p.m. UTC | #1 
Quoting Laurent Pinchart via libcamera-devel (2022-08-07 03:17:15)
> The line stride of the texture is currently hardcoded based on the image
> width, which may not match the real stride. Use the stride value from
> the stream configuration instead.
> 

Looks reasonable

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

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/cam/sdl_sink.cpp         | 2 +-
>  src/cam/sdl_texture_yuyv.cpp | 4 ++--
>  src/cam/sdl_texture_yuyv.h   | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> index 19fdfd6dced5..a59d07519e1f 100644
> --- a/src/cam/sdl_sink.cpp
> +++ b/src/cam/sdl_sink.cpp
> @@ -68,7 +68,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config)
>                 break;
>  #endif
>         case libcamera::formats::YUYV:
> -               texture_ = std::make_unique<SDLTextureYUYV>(rect_);
> +               texture_ = std::make_unique<SDLTextureYUYV>(rect_, cfg.stride);
>                 break;
>         default:
>                 std::cerr << "Unsupported pixel format "
> diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp
> index 637c0900edff..cb51fb0ef478 100644
> --- a/src/cam/sdl_texture_yuyv.cpp
> +++ b/src/cam/sdl_texture_yuyv.cpp
> @@ -9,8 +9,8 @@
>  
>  using namespace libcamera;
>  
> -SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &rect)
> -       : SDLTexture(rect, SDL_PIXELFORMAT_YUY2, 4 * ((rect.w + 1) / 2))
> +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &rect, unsigned int stride)
> +       : SDLTexture(rect, SDL_PIXELFORMAT_YUY2, stride)
>  {
>  }
>  
> diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h
> index 529a72d6a40e..81e51381ec62 100644
> --- a/src/cam/sdl_texture_yuyv.h
> +++ b/src/cam/sdl_texture_yuyv.h
> @@ -12,6 +12,6 @@
>  class SDLTextureYUYV : public SDLTexture
>  {
>  public:
> -       SDLTextureYUYV(const SDL_Rect &rect);
> +       SDLTextureYUYV(const SDL_Rect &rect, unsigned int stride);
>         void update(libcamera::Span<const uint8_t> data) override;
>  };
> -- 
> Regards,
> 
> Laurent Pinchart
>
Jacopo Mondi Aug. 8, 2022, 8:20 a.m. UTC | #2 
Hi Laurent

On Sun, Aug 07, 2022 at 05:17:15AM +0300, Laurent Pinchart via libcamera-devel wrote:
> The line stride of the texture is currently hardcoded based on the image
> width, which may not match the real stride. Use the stride value from
> the stream configuration instead.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/cam/sdl_sink.cpp         | 2 +-
>  src/cam/sdl_texture_yuyv.cpp | 4 ++--
>  src/cam/sdl_texture_yuyv.h   | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> index 19fdfd6dced5..a59d07519e1f 100644
> --- a/src/cam/sdl_sink.cpp
> +++ b/src/cam/sdl_sink.cpp
> @@ -68,7 +68,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config)
>  		break;
>  #endif
>  	case libcamera::formats::YUYV:
> -		texture_ = std::make_unique<SDLTextureYUYV>(rect_);
> +		texture_ = std::make_unique<SDLTextureYUYV>(rect_, cfg.stride);
>  		break;
>  	default:
>  		std::cerr << "Unsupported pixel format "
> diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp
> index 637c0900edff..cb51fb0ef478 100644
> --- a/src/cam/sdl_texture_yuyv.cpp
> +++ b/src/cam/sdl_texture_yuyv.cpp
> @@ -9,8 +9,8 @@
>
>  using namespace libcamera;
>
> -SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &rect)
> -	: SDLTexture(rect, SDL_PIXELFORMAT_YUY2, 4 * ((rect.w + 1) / 2))
> +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &rect, unsigned int stride)

nit: the base class has the same parameter but calls it "pitch".

The rest looks good
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>


> +	: SDLTexture(rect, SDL_PIXELFORMAT_YUY2, stride)
>  {
>  }
>
> diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h
> index 529a72d6a40e..81e51381ec62 100644
> --- a/src/cam/sdl_texture_yuyv.h
> +++ b/src/cam/sdl_texture_yuyv.h
> @@ -12,6 +12,6 @@
>  class SDLTextureYUYV : public SDLTexture
>  {
>  public:
> -	SDLTextureYUYV(const SDL_Rect &rect);
> +	SDLTextureYUYV(const SDL_Rect &rect, unsigned int stride);
>  	void update(libcamera::Span<const uint8_t> data) override;
>  };
> --
> Regards,
>
> Laurent Pinchart
>
Eric Curtin Aug. 8, 2022, 9:19 a.m. UTC | #3 
Hi Laurent,

Good morning.

On Sun, 7 Aug 2022 at 03:17, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> The line stride of the texture is currently hardcoded based on the image
> width, which may not match the real stride. Use the stride value from
> the stream configuration instead.
>

Optionally if you'd like to fix the pitch/stride inconsistency in the
code, pre-existing this change of course.

Reviewed-by: Eric Curtin <ecurtin@redhat.com>

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/cam/sdl_sink.cpp         | 2 +-
>  src/cam/sdl_texture_yuyv.cpp | 4 ++--
>  src/cam/sdl_texture_yuyv.h   | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> index 19fdfd6dced5..a59d07519e1f 100644
> --- a/src/cam/sdl_sink.cpp
> +++ b/src/cam/sdl_sink.cpp
> @@ -68,7 +68,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config)
>                 break;
>  #endif
>         case libcamera::formats::YUYV:
> -               texture_ = std::make_unique<SDLTextureYUYV>(rect_);
> +               texture_ = std::make_unique<SDLTextureYUYV>(rect_, cfg.stride);
>                 break;
>         default:
>                 std::cerr << "Unsupported pixel format "
> diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp
> index 637c0900edff..cb51fb0ef478 100644
> --- a/src/cam/sdl_texture_yuyv.cpp
> +++ b/src/cam/sdl_texture_yuyv.cpp
> @@ -9,8 +9,8 @@
>
>  using namespace libcamera;
>
> -SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &rect)
> -       : SDLTexture(rect, SDL_PIXELFORMAT_YUY2, 4 * ((rect.w + 1) / 2))
> +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &rect, unsigned int stride)
> +       : SDLTexture(rect, SDL_PIXELFORMAT_YUY2, stride)
>  {
>  }
>
> diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h
> index 529a72d6a40e..81e51381ec62 100644
> --- a/src/cam/sdl_texture_yuyv.h
> +++ b/src/cam/sdl_texture_yuyv.h
> @@ -12,6 +12,6 @@
>  class SDLTextureYUYV : public SDLTexture
>  {
>  public:
> -       SDLTextureYUYV(const SDL_Rect &rect);
> +       SDLTextureYUYV(const SDL_Rect &rect, unsigned int stride);
>         void update(libcamera::Span<const uint8_t> data) override;
>  };
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart Aug. 8, 2022, 2:12 p.m. UTC | #4 
Hi Jacopo,

On Mon, Aug 08, 2022 at 10:20:34AM +0200, Jacopo Mondi wrote:
> On Sun, Aug 07, 2022 at 05:17:15AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > The line stride of the texture is currently hardcoded based on the image
> > width, which may not match the real stride. Use the stride value from
> > the stream configuration instead.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/cam/sdl_sink.cpp         | 2 +-
> >  src/cam/sdl_texture_yuyv.cpp | 4 ++--
> >  src/cam/sdl_texture_yuyv.h   | 2 +-
> >  3 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
> > index 19fdfd6dced5..a59d07519e1f 100644
> > --- a/src/cam/sdl_sink.cpp
> > +++ b/src/cam/sdl_sink.cpp
> > @@ -68,7 +68,7 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config)
> >  		break;
> >  #endif
> >  	case libcamera::formats::YUYV:
> > -		texture_ = std::make_unique<SDLTextureYUYV>(rect_);
> > +		texture_ = std::make_unique<SDLTextureYUYV>(rect_, cfg.stride);
> >  		break;
> >  	default:
> >  		std::cerr << "Unsupported pixel format "
> > diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp
> > index 637c0900edff..cb51fb0ef478 100644
> > --- a/src/cam/sdl_texture_yuyv.cpp
> > +++ b/src/cam/sdl_texture_yuyv.cpp
> > @@ -9,8 +9,8 @@
> >
> >  using namespace libcamera;
> >
> > -SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &rect)
> > -	: SDLTexture(rect, SDL_PIXELFORMAT_YUY2, 4 * ((rect.w + 1) / 2))
> > +SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &rect, unsigned int stride)
> 
> nit: the base class has the same parameter but calls it "pitch".

I know, we have stride coming from libcamera, and pitch in the base
class, so it's bound to be inconsistent somewhere. Should I submit
another patch to rename pitch to stride in the base class too ?

> The rest looks good
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> > +	: SDLTexture(rect, SDL_PIXELFORMAT_YUY2, stride)
> >  {
> >  }
> >
> > diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h
> > index 529a72d6a40e..81e51381ec62 100644
> > --- a/src/cam/sdl_texture_yuyv.h
> > +++ b/src/cam/sdl_texture_yuyv.h
> > @@ -12,6 +12,6 @@
> >  class SDLTextureYUYV : public SDLTexture
> >  {
> >  public:
> > -	SDLTextureYUYV(const SDL_Rect &rect);
> > +	SDLTextureYUYV(const SDL_Rect &rect, unsigned int stride);
> >  	void update(libcamera::Span<const uint8_t> data) override;
> >  };

Patch
diff mbox series 

diff --git a/src/cam/sdl_sink.cpp b/src/cam/sdl_sink.cpp
index 19fdfd6dced5..a59d07519e1f 100644
--- a/src/cam/sdl_sink.cpp
+++ b/src/cam/sdl_sink.cpp
@@ -68,7 +68,7 @@  int SDLSink::configure(const libcamera::CameraConfiguration &config)
 		break;
 #endif
 	case libcamera::formats::YUYV:
-		texture_ = std::make_unique<SDLTextureYUYV>(rect_);
+		texture_ = std::make_unique<SDLTextureYUYV>(rect_, cfg.stride);
 		break;
 	default:
 		std::cerr << "Unsupported pixel format "
diff --git a/src/cam/sdl_texture_yuyv.cpp b/src/cam/sdl_texture_yuyv.cpp
index 637c0900edff..cb51fb0ef478 100644
--- a/src/cam/sdl_texture_yuyv.cpp
+++ b/src/cam/sdl_texture_yuyv.cpp
@@ -9,8 +9,8 @@ 
 
 using namespace libcamera;
 
-SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &rect)
-	: SDLTexture(rect, SDL_PIXELFORMAT_YUY2, 4 * ((rect.w + 1) / 2))
+SDLTextureYUYV::SDLTextureYUYV(const SDL_Rect &rect, unsigned int stride)
+	: SDLTexture(rect, SDL_PIXELFORMAT_YUY2, stride)
 {
 }
 
diff --git a/src/cam/sdl_texture_yuyv.h b/src/cam/sdl_texture_yuyv.h
index 529a72d6a40e..81e51381ec62 100644
--- a/src/cam/sdl_texture_yuyv.h
+++ b/src/cam/sdl_texture_yuyv.h
@@ -12,6 +12,6 @@ 
 class SDLTextureYUYV : public SDLTexture
 {
 public:
-	SDLTextureYUYV(const SDL_Rect &rect);
+	SDLTextureYUYV(const SDL_Rect &rect, unsigned int stride);
 	void update(libcamera::Span<const uint8_t> data) override;
 };