| Message ID | 20251023145139.1085153-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Thu, Oct 23, 2025 at 04:51:39PM +0200, Barnabás Pőcze wrote: > SDL also supports NV21 with the same `SDL_UpdateNVTexture()`, > so add support for it. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/apps/cam/sdl_sink.cpp | 4 +++- > src/apps/cam/sdl_texture_yuv.cpp | 13 +++++++++---- > src/apps/cam/sdl_texture_yuv.h | 4 ++-- > 3 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/src/apps/cam/sdl_sink.cpp b/src/apps/cam/sdl_sink.cpp > index 30cfdf0af..17a9e04b5 100644 > --- a/src/apps/cam/sdl_sink.cpp > +++ b/src/apps/cam/sdl_sink.cpp > @@ -112,7 +112,9 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) > #endif > #if SDL_VERSION_ATLEAST(2, 0, 16) > else if (cfg.pixelFormat == libcamera::formats::NV12) > - texture_ = std::make_unique<SDLTextureNV12>(rect_, cfg.stride); > + texture_ = std::make_unique<SDLTextureNV>(rect_, SDL_PIXELFORMAT_NV12, cfg.stride); > + else if (cfg.pixelFormat == libcamera::formats::NV21) > + texture_ = std::make_unique<SDLTextureNV>(rect_, SDL_PIXELFORMAT_NV21, cfg.stride); > #endif > else { > std::cerr << "Unsupported pixel format " << cfg.pixelFormat << std::endl; > diff --git a/src/apps/cam/sdl_texture_yuv.cpp b/src/apps/cam/sdl_texture_yuv.cpp > index bed297d28..40d1a3ae9 100644 > --- a/src/apps/cam/sdl_texture_yuv.cpp > +++ b/src/apps/cam/sdl_texture_yuv.cpp > @@ -7,16 +7,21 @@ > > #include "sdl_texture_yuv.h" > > -using namespace libcamera; > +#include <assert.h> > > #if SDL_VERSION_ATLEAST(2, 0, 16) > -SDLTextureNV12::SDLTextureNV12(const SDL_Rect &rect, unsigned int stride) > - : SDLTexture(rect, SDL_PIXELFORMAT_NV12, stride) > +SDLTextureNV::SDLTextureNV(const SDL_Rect &rect, uint32_t pixelFormat, unsigned int stride) > + : SDLTexture(rect, pixelFormat, stride) > { > + assert(pixelFormat == SDL_PIXELFORMAT_NV12 || pixelFormat == SDL_PIXELFORMAT_NV21); > } > > -void SDLTextureNV12::update(libcamera::Span<const libcamera::Span<const uint8_t>> data) > +void SDLTextureNV::update(libcamera::Span<const libcamera::Span<const uint8_t>> data) > { > + assert(data.size() == 2); > + assert(data[0].size_bytes() == std::size_t(rect_.h) * std::size_t(stride_)); > + assert(data[1].size_bytes() == std::size_t(rect_.h) * std::size_t(stride_) / 2); That seems quite strict, >= could be better. You should also mention the reason for this change in the commit message. > + > SDL_UpdateNVTexture(ptr_, nullptr, data[0].data(), stride_, > data[1].data(), stride_); > } > diff --git a/src/apps/cam/sdl_texture_yuv.h b/src/apps/cam/sdl_texture_yuv.h > index c271f901b..8e73506d1 100644 > --- a/src/apps/cam/sdl_texture_yuv.h > +++ b/src/apps/cam/sdl_texture_yuv.h > @@ -10,10 +10,10 @@ > #include "sdl_texture.h" > > #if SDL_VERSION_ATLEAST(2, 0, 16) > -class SDLTextureNV12 : public SDLTexture > +class SDLTextureNV final : public SDLTexture Same here, explain in the commit message why you add final. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > { > public: > - SDLTextureNV12(const SDL_Rect &rect, unsigned int stride); > + SDLTextureNV(const SDL_Rect &rect, uint32_t pixelFormat, unsigned int stride); > void update(libcamera::Span<const libcamera::Span<const uint8_t>> data) override; > }; > #endif
2025. 10. 23. 18:32 keltezéssel, Laurent Pinchart írta: > On Thu, Oct 23, 2025 at 04:51:39PM +0200, Barnabás Pőcze wrote: >> SDL also supports NV21 with the same `SDL_UpdateNVTexture()`, >> so add support for it. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/apps/cam/sdl_sink.cpp | 4 +++- >> src/apps/cam/sdl_texture_yuv.cpp | 13 +++++++++---- >> src/apps/cam/sdl_texture_yuv.h | 4 ++-- >> 3 files changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/src/apps/cam/sdl_sink.cpp b/src/apps/cam/sdl_sink.cpp >> index 30cfdf0af..17a9e04b5 100644 >> --- a/src/apps/cam/sdl_sink.cpp >> +++ b/src/apps/cam/sdl_sink.cpp >> @@ -112,7 +112,9 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) >> #endif >> #if SDL_VERSION_ATLEAST(2, 0, 16) >> else if (cfg.pixelFormat == libcamera::formats::NV12) >> - texture_ = std::make_unique<SDLTextureNV12>(rect_, cfg.stride); >> + texture_ = std::make_unique<SDLTextureNV>(rect_, SDL_PIXELFORMAT_NV12, cfg.stride); >> + else if (cfg.pixelFormat == libcamera::formats::NV21) >> + texture_ = std::make_unique<SDLTextureNV>(rect_, SDL_PIXELFORMAT_NV21, cfg.stride); >> #endif >> else { >> std::cerr << "Unsupported pixel format " << cfg.pixelFormat << std::endl; >> diff --git a/src/apps/cam/sdl_texture_yuv.cpp b/src/apps/cam/sdl_texture_yuv.cpp >> index bed297d28..40d1a3ae9 100644 >> --- a/src/apps/cam/sdl_texture_yuv.cpp >> +++ b/src/apps/cam/sdl_texture_yuv.cpp >> @@ -7,16 +7,21 @@ >> >> #include "sdl_texture_yuv.h" >> >> -using namespace libcamera; >> +#include <assert.h> >> >> #if SDL_VERSION_ATLEAST(2, 0, 16) >> -SDLTextureNV12::SDLTextureNV12(const SDL_Rect &rect, unsigned int stride) >> - : SDLTexture(rect, SDL_PIXELFORMAT_NV12, stride) >> +SDLTextureNV::SDLTextureNV(const SDL_Rect &rect, uint32_t pixelFormat, unsigned int stride) >> + : SDLTexture(rect, pixelFormat, stride) >> { >> + assert(pixelFormat == SDL_PIXELFORMAT_NV12 || pixelFormat == SDL_PIXELFORMAT_NV21); >> } >> >> -void SDLTextureNV12::update(libcamera::Span<const libcamera::Span<const uint8_t>> data) >> +void SDLTextureNV::update(libcamera::Span<const libcamera::Span<const uint8_t>> data) >> { >> + assert(data.size() == 2); >> + assert(data[0].size_bytes() == std::size_t(rect_.h) * std::size_t(stride_)); >> + assert(data[1].size_bytes() == std::size_t(rect_.h) * std::size_t(stride_) / 2); > > That seems quite strict, >= could be better. You should also mention the > reason for this change in the commit message. I assume the `data.size()` check is fine with `==`. For the other two, do you have anything specific in mind? Is it reasonable to expect tail padding in the buffer? Regards, Barnabás Pőcze > >> + >> SDL_UpdateNVTexture(ptr_, nullptr, data[0].data(), stride_, >> data[1].data(), stride_); >> } >> diff --git a/src/apps/cam/sdl_texture_yuv.h b/src/apps/cam/sdl_texture_yuv.h >> index c271f901b..8e73506d1 100644 >> --- a/src/apps/cam/sdl_texture_yuv.h >> +++ b/src/apps/cam/sdl_texture_yuv.h >> @@ -10,10 +10,10 @@ >> #include "sdl_texture.h" >> >> #if SDL_VERSION_ATLEAST(2, 0, 16) >> -class SDLTextureNV12 : public SDLTexture >> +class SDLTextureNV final : public SDLTexture > > Same here, explain in the commit message why you add final. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> { >> public: >> - SDLTextureNV12(const SDL_Rect &rect, unsigned int stride); >> + SDLTextureNV(const SDL_Rect &rect, uint32_t pixelFormat, unsigned int stride); >> void update(libcamera::Span<const libcamera::Span<const uint8_t>> data) override; >> }; >> #endif >
On Thu, Oct 23, 2025 at 08:25:06PM +0200, Barnabás Pőcze wrote: > 2025. 10. 23. 18:32 keltezéssel, Laurent Pinchart írta: > > On Thu, Oct 23, 2025 at 04:51:39PM +0200, Barnabás Pőcze wrote: > >> SDL also supports NV21 with the same `SDL_UpdateNVTexture()`, > >> so add support for it. > >> > >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >> --- > >> src/apps/cam/sdl_sink.cpp | 4 +++- > >> src/apps/cam/sdl_texture_yuv.cpp | 13 +++++++++---- > >> src/apps/cam/sdl_texture_yuv.h | 4 ++-- > >> 3 files changed, 14 insertions(+), 7 deletions(-) > >> > >> diff --git a/src/apps/cam/sdl_sink.cpp b/src/apps/cam/sdl_sink.cpp > >> index 30cfdf0af..17a9e04b5 100644 > >> --- a/src/apps/cam/sdl_sink.cpp > >> +++ b/src/apps/cam/sdl_sink.cpp > >> @@ -112,7 +112,9 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) > >> #endif > >> #if SDL_VERSION_ATLEAST(2, 0, 16) > >> else if (cfg.pixelFormat == libcamera::formats::NV12) > >> - texture_ = std::make_unique<SDLTextureNV12>(rect_, cfg.stride); > >> + texture_ = std::make_unique<SDLTextureNV>(rect_, SDL_PIXELFORMAT_NV12, cfg.stride); > >> + else if (cfg.pixelFormat == libcamera::formats::NV21) > >> + texture_ = std::make_unique<SDLTextureNV>(rect_, SDL_PIXELFORMAT_NV21, cfg.stride); > >> #endif > >> else { > >> std::cerr << "Unsupported pixel format " << cfg.pixelFormat << std::endl; > >> diff --git a/src/apps/cam/sdl_texture_yuv.cpp b/src/apps/cam/sdl_texture_yuv.cpp > >> index bed297d28..40d1a3ae9 100644 > >> --- a/src/apps/cam/sdl_texture_yuv.cpp > >> +++ b/src/apps/cam/sdl_texture_yuv.cpp > >> @@ -7,16 +7,21 @@ > >> > >> #include "sdl_texture_yuv.h" > >> > >> -using namespace libcamera; > >> +#include <assert.h> > >> > >> #if SDL_VERSION_ATLEAST(2, 0, 16) > >> -SDLTextureNV12::SDLTextureNV12(const SDL_Rect &rect, unsigned int stride) > >> - : SDLTexture(rect, SDL_PIXELFORMAT_NV12, stride) > >> +SDLTextureNV::SDLTextureNV(const SDL_Rect &rect, uint32_t pixelFormat, unsigned int stride) > >> + : SDLTexture(rect, pixelFormat, stride) > >> { > >> + assert(pixelFormat == SDL_PIXELFORMAT_NV12 || pixelFormat == SDL_PIXELFORMAT_NV21); > >> } > >> > >> -void SDLTextureNV12::update(libcamera::Span<const libcamera::Span<const uint8_t>> data) > >> +void SDLTextureNV::update(libcamera::Span<const libcamera::Span<const uint8_t>> data) > >> { > >> + assert(data.size() == 2); > >> + assert(data[0].size_bytes() == std::size_t(rect_.h) * std::size_t(stride_)); > >> + assert(data[1].size_bytes() == std::size_t(rect_.h) * std::size_t(stride_) / 2); > > > > That seems quite strict, >= could be better. You should also mention the > > reason for this change in the commit message. > > I assume the `data.size()` check is fine with `==`. Yes sorry. > For the other two, > do you have anything specific in mind? Is it reasonable to expect tail > padding in the buffer? Padding is what I had in mind, yes. We may not have a use case for that in the cam application though. The SDL classes are not generic. Have you seen those assertions fail, can it happen with the current code base ? > >> + > >> SDL_UpdateNVTexture(ptr_, nullptr, data[0].data(), stride_, > >> data[1].data(), stride_); > >> } > >> diff --git a/src/apps/cam/sdl_texture_yuv.h b/src/apps/cam/sdl_texture_yuv.h > >> index c271f901b..8e73506d1 100644 > >> --- a/src/apps/cam/sdl_texture_yuv.h > >> +++ b/src/apps/cam/sdl_texture_yuv.h > >> @@ -10,10 +10,10 @@ > >> #include "sdl_texture.h" > >> > >> #if SDL_VERSION_ATLEAST(2, 0, 16) > >> -class SDLTextureNV12 : public SDLTexture > >> +class SDLTextureNV final : public SDLTexture > > > > Same here, explain in the commit message why you add final. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > >> { > >> public: > >> - SDLTextureNV12(const SDL_Rect &rect, unsigned int stride); > >> + SDLTextureNV(const SDL_Rect &rect, uint32_t pixelFormat, unsigned int stride); > >> void update(libcamera::Span<const libcamera::Span<const uint8_t>> data) override; > >> }; > >> #endif
2025. 10. 23. 21:18 keltezéssel, Laurent Pinchart írta: > On Thu, Oct 23, 2025 at 08:25:06PM +0200, Barnabás Pőcze wrote: >> 2025. 10. 23. 18:32 keltezéssel, Laurent Pinchart írta: >>> On Thu, Oct 23, 2025 at 04:51:39PM +0200, Barnabás Pőcze wrote: >>>> SDL also supports NV21 with the same `SDL_UpdateNVTexture()`, >>>> so add support for it. >>>> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> --- >>>> src/apps/cam/sdl_sink.cpp | 4 +++- >>>> src/apps/cam/sdl_texture_yuv.cpp | 13 +++++++++---- >>>> src/apps/cam/sdl_texture_yuv.h | 4 ++-- >>>> 3 files changed, 14 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/src/apps/cam/sdl_sink.cpp b/src/apps/cam/sdl_sink.cpp >>>> index 30cfdf0af..17a9e04b5 100644 >>>> --- a/src/apps/cam/sdl_sink.cpp >>>> +++ b/src/apps/cam/sdl_sink.cpp >>>> @@ -112,7 +112,9 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) >>>> #endif >>>> #if SDL_VERSION_ATLEAST(2, 0, 16) >>>> else if (cfg.pixelFormat == libcamera::formats::NV12) >>>> - texture_ = std::make_unique<SDLTextureNV12>(rect_, cfg.stride); >>>> + texture_ = std::make_unique<SDLTextureNV>(rect_, SDL_PIXELFORMAT_NV12, cfg.stride); >>>> + else if (cfg.pixelFormat == libcamera::formats::NV21) >>>> + texture_ = std::make_unique<SDLTextureNV>(rect_, SDL_PIXELFORMAT_NV21, cfg.stride); >>>> #endif >>>> else { >>>> std::cerr << "Unsupported pixel format " << cfg.pixelFormat << std::endl; >>>> diff --git a/src/apps/cam/sdl_texture_yuv.cpp b/src/apps/cam/sdl_texture_yuv.cpp >>>> index bed297d28..40d1a3ae9 100644 >>>> --- a/src/apps/cam/sdl_texture_yuv.cpp >>>> +++ b/src/apps/cam/sdl_texture_yuv.cpp >>>> @@ -7,16 +7,21 @@ >>>> >>>> #include "sdl_texture_yuv.h" >>>> >>>> -using namespace libcamera; >>>> +#include <assert.h> >>>> >>>> #if SDL_VERSION_ATLEAST(2, 0, 16) >>>> -SDLTextureNV12::SDLTextureNV12(const SDL_Rect &rect, unsigned int stride) >>>> - : SDLTexture(rect, SDL_PIXELFORMAT_NV12, stride) >>>> +SDLTextureNV::SDLTextureNV(const SDL_Rect &rect, uint32_t pixelFormat, unsigned int stride) >>>> + : SDLTexture(rect, pixelFormat, stride) >>>> { >>>> + assert(pixelFormat == SDL_PIXELFORMAT_NV12 || pixelFormat == SDL_PIXELFORMAT_NV21); >>>> } >>>> >>>> -void SDLTextureNV12::update(libcamera::Span<const libcamera::Span<const uint8_t>> data) >>>> +void SDLTextureNV::update(libcamera::Span<const libcamera::Span<const uint8_t>> data) >>>> { >>>> + assert(data.size() == 2); >>>> + assert(data[0].size_bytes() == std::size_t(rect_.h) * std::size_t(stride_)); >>>> + assert(data[1].size_bytes() == std::size_t(rect_.h) * std::size_t(stride_) / 2); >>> >>> That seems quite strict, >= could be better. You should also mention the >>> reason for this change in the commit message. >> >> I assume the `data.size()` check is fine with `==`. > > Yes sorry. > >> For the other two, >> do you have anything specific in mind? Is it reasonable to expect tail >> padding in the buffer? > > Padding is what I had in mind, yes. We may not have a use case for that > in the cam application though. The SDL classes are not generic. > > Have you seen those assertions fail, can it happen with the current code > base ? I haven't seen them fail. For different pixel formats, but there is a similar assertion in `SDLTexture1Plane::update()`, which was added before 0.5.1, and no complaints so far. > >>>> + >>>> SDL_UpdateNVTexture(ptr_, nullptr, data[0].data(), stride_, >>>> data[1].data(), stride_); >>>> } >>>> diff --git a/src/apps/cam/sdl_texture_yuv.h b/src/apps/cam/sdl_texture_yuv.h >>>> index c271f901b..8e73506d1 100644 >>>> --- a/src/apps/cam/sdl_texture_yuv.h >>>> +++ b/src/apps/cam/sdl_texture_yuv.h >>>> @@ -10,10 +10,10 @@ >>>> #include "sdl_texture.h" >>>> >>>> #if SDL_VERSION_ATLEAST(2, 0, 16) >>>> -class SDLTextureNV12 : public SDLTexture >>>> +class SDLTextureNV final : public SDLTexture >>> >>> Same here, explain in the commit message why you add final. >>> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> >>>> { >>>> public: >>>> - SDLTextureNV12(const SDL_Rect &rect, unsigned int stride); >>>> + SDLTextureNV(const SDL_Rect &rect, uint32_t pixelFormat, unsigned int stride); >>>> void update(libcamera::Span<const libcamera::Span<const uint8_t>> data) override; >>>> }; >>>> #endif >
diff --git a/src/apps/cam/sdl_sink.cpp b/src/apps/cam/sdl_sink.cpp index 30cfdf0af..17a9e04b5 100644 --- a/src/apps/cam/sdl_sink.cpp +++ b/src/apps/cam/sdl_sink.cpp @@ -112,7 +112,9 @@ int SDLSink::configure(const libcamera::CameraConfiguration &config) #endif #if SDL_VERSION_ATLEAST(2, 0, 16) else if (cfg.pixelFormat == libcamera::formats::NV12) - texture_ = std::make_unique<SDLTextureNV12>(rect_, cfg.stride); + texture_ = std::make_unique<SDLTextureNV>(rect_, SDL_PIXELFORMAT_NV12, cfg.stride); + else if (cfg.pixelFormat == libcamera::formats::NV21) + texture_ = std::make_unique<SDLTextureNV>(rect_, SDL_PIXELFORMAT_NV21, cfg.stride); #endif else { std::cerr << "Unsupported pixel format " << cfg.pixelFormat << std::endl; diff --git a/src/apps/cam/sdl_texture_yuv.cpp b/src/apps/cam/sdl_texture_yuv.cpp index bed297d28..40d1a3ae9 100644 --- a/src/apps/cam/sdl_texture_yuv.cpp +++ b/src/apps/cam/sdl_texture_yuv.cpp @@ -7,16 +7,21 @@ #include "sdl_texture_yuv.h" -using namespace libcamera; +#include <assert.h> #if SDL_VERSION_ATLEAST(2, 0, 16) -SDLTextureNV12::SDLTextureNV12(const SDL_Rect &rect, unsigned int stride) - : SDLTexture(rect, SDL_PIXELFORMAT_NV12, stride) +SDLTextureNV::SDLTextureNV(const SDL_Rect &rect, uint32_t pixelFormat, unsigned int stride) + : SDLTexture(rect, pixelFormat, stride) { + assert(pixelFormat == SDL_PIXELFORMAT_NV12 || pixelFormat == SDL_PIXELFORMAT_NV21); } -void SDLTextureNV12::update(libcamera::Span<const libcamera::Span<const uint8_t>> data) +void SDLTextureNV::update(libcamera::Span<const libcamera::Span<const uint8_t>> data) { + assert(data.size() == 2); + assert(data[0].size_bytes() == std::size_t(rect_.h) * std::size_t(stride_)); + assert(data[1].size_bytes() == std::size_t(rect_.h) * std::size_t(stride_) / 2); + SDL_UpdateNVTexture(ptr_, nullptr, data[0].data(), stride_, data[1].data(), stride_); } diff --git a/src/apps/cam/sdl_texture_yuv.h b/src/apps/cam/sdl_texture_yuv.h index c271f901b..8e73506d1 100644 --- a/src/apps/cam/sdl_texture_yuv.h +++ b/src/apps/cam/sdl_texture_yuv.h @@ -10,10 +10,10 @@ #include "sdl_texture.h" #if SDL_VERSION_ATLEAST(2, 0, 16) -class SDLTextureNV12 : public SDLTexture +class SDLTextureNV final : public SDLTexture { public: - SDLTextureNV12(const SDL_Rect &rect, unsigned int stride); + SDLTextureNV(const SDL_Rect &rect, uint32_t pixelFormat, unsigned int stride); void update(libcamera::Span<const libcamera::Span<const uint8_t>> data) override; }; #endif
SDL also supports NV21 with the same `SDL_UpdateNVTexture()`, so add support for it. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/apps/cam/sdl_sink.cpp | 4 +++- src/apps/cam/sdl_texture_yuv.cpp | 13 +++++++++---- src/apps/cam/sdl_texture_yuv.h | 4 ++-- 3 files changed, 14 insertions(+), 7 deletions(-)