| Message ID | 20220516141022.96327-11-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi, Thank you for the patch. On Mon, May 16, 2022 at 05:10:18PM +0300, Tomi Valkeinen wrote: > Implement PixelFormat bindings properly with a PixelFormat class. Change > the bindings to use the new class instead of a string. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > src/py/cam/cam.py | 4 +-- > src/py/cam/cam_kms.py | 9 +----- > src/py/cam/cam_qt.py | 2 +- > src/py/cam/cam_qtgl.py | 17 +----------- > src/py/cam/gl_helpers.py | 8 ------ > src/py/libcamera/pymain.cpp | 51 ++++++++++++++++++++-------------- > src/py/libcamera/utils/conv.py | 2 ++ > 7 files changed, 37 insertions(+), 56 deletions(-) > > diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py > index c7da97d7..63c67126 100755 > --- a/src/py/cam/cam.py > +++ b/src/py/cam/cam.py > @@ -80,7 +80,7 @@ def do_cmd_info(ctx): > > formats = stream_config.formats > for fmt in formats.pixel_formats: > - print('\t * Pixelformat:', fmt, formats.range(fmt)) > + print('\t * Pixelformat:', fmt.name, formats.range(fmt)) > > for size in formats.sizes(fmt): > print('\t -', size) > @@ -164,7 +164,7 @@ def configure(ctx): > stream_config.size = (stream_opts['width'], stream_opts['height']) > > if 'pixelformat' in stream_opts: > - stream_config.pixel_format = stream_opts['pixelformat'] > + stream_config.pixel_format = libcam.PixelFormat.from_name(stream_opts['pixelformat']) > > stat = camconfig.validate() > > diff --git a/src/py/cam/cam_kms.py b/src/py/cam/cam_kms.py > index ae6be277..7d11564b 100644 > --- a/src/py/cam/cam_kms.py > +++ b/src/py/cam/cam_kms.py > @@ -5,13 +5,6 @@ import pykms > import selectors > import sys > > -FMT_MAP = { > - 'RGB888': pykms.PixelFormat.RGB888, > - 'YUYV': pykms.PixelFormat.YUYV, > - 'ARGB8888': pykms.PixelFormat.ARGB8888, > - 'XRGB8888': pykms.PixelFormat.XRGB8888, > -} > - > > class KMSRenderer: > def __init__(self, state): > @@ -118,7 +111,7 @@ class KMSRenderer: > > cfg = stream.configuration > fmt = cfg.pixel_format > - fmt = FMT_MAP[fmt] > + fmt = pykms.PixelFormat(fmt.fourcc) > > plane = self.resman.reserve_generic_plane(self.crtc, fmt) > assert(plane is not None) > diff --git a/src/py/cam/cam_qt.py b/src/py/cam/cam_qt.py > index dc5219eb..64f49f33 100644 > --- a/src/py/cam/cam_qt.py > +++ b/src/py/cam/cam_qt.py > @@ -139,7 +139,7 @@ class MainWindow(QtWidgets.QWidget): > w, h = cfg.size > pitch = cfg.stride > > - if cfg.pixel_format == 'MJPEG': > + if cfg.pixel_format.name == 'MJPEG': > img = Image.open(BytesIO(mfb.planes[0])) > qim = ImageQt(img).copy() > pix = QtGui.QPixmap.fromImage(qim) > diff --git a/src/py/cam/cam_qtgl.py b/src/py/cam/cam_qtgl.py > index 8a95994e..261accb8 100644 > --- a/src/py/cam/cam_qtgl.py > +++ b/src/py/cam/cam_qtgl.py > @@ -30,14 +30,6 @@ from OpenGL.GL import shaders > > from gl_helpers import * > > -# libcamera format string -> DRM fourcc > -FMT_MAP = { > - 'RGB888': 'RG24', > - 'XRGB8888': 'XR24', > - 'ARGB8888': 'AR24', > - 'YUYV': 'YUYV', > -} > - > > class EglState: > def __init__(self): > @@ -204,12 +196,6 @@ class MainWindow(QtWidgets.QWidget): > self.current[ctx['idx']] = [] > > for stream in ctx['streams']: > - fmt = stream.configuration.pixel_format > - size = stream.configuration.size > - > - if fmt not in FMT_MAP: > - raise Exception('Unsupported pixel format: ' + str(fmt)) > - > self.textures[stream] = None > > num_tiles = len(self.textures) > @@ -281,8 +267,7 @@ class MainWindow(QtWidgets.QWidget): > > def create_texture(self, stream, fb): > cfg = stream.configuration > - fmt = cfg.pixel_format > - fmt = str_to_fourcc(FMT_MAP[fmt]) > + fmt = cfg.pixel_format.fourcc > w, h = cfg.size > > attribs = [ > diff --git a/src/py/cam/gl_helpers.py b/src/py/cam/gl_helpers.py > index ac5e6889..53b3e9df 100644 > --- a/src/py/cam/gl_helpers.py > +++ b/src/py/cam/gl_helpers.py > @@ -30,14 +30,6 @@ def getglEGLImageTargetTexture2DOES(): > > glEGLImageTargetTexture2DOES = getglEGLImageTargetTexture2DOES() > > -# \todo This can be dropped when we have proper PixelFormat bindings > -def str_to_fourcc(str): > - assert(len(str) == 4) > - fourcc = 0 > - for i, v in enumerate([ord(c) for c in str]): > - fourcc |= v << (i * 8) > - return fourcc > - > > def get_gl_extensions(): > n = GLint() > diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp > index af22205e..cc2ddee5 100644 > --- a/src/py/libcamera/pymain.cpp > +++ b/src/py/libcamera/pymain.cpp > @@ -8,7 +8,6 @@ > /* > * \todo Add geometry classes (Point, Rectangle...) > * \todo Add bindings for the ControlInfo class > - * \todo Add bindings for the PixelFormat class > */ > > #include <mutex> > @@ -173,6 +172,7 @@ PYBIND11_MODULE(_libcamera, m) > auto pyColorSpaceTransferFunction = py::enum_<ColorSpace::TransferFunction>(pyColorSpace, "TransferFunction"); > auto pyColorSpaceYcbcrEncoding = py::enum_<ColorSpace::YcbcrEncoding>(pyColorSpace, "YcbcrEncoding"); > auto pyColorSpaceRange = py::enum_<ColorSpace::Range>(pyColorSpace, "Range"); > + auto pyPixelFormat = py::class_<PixelFormat>(m, "PixelFormat"); > > /* Global functions */ > m.def("log_set_level", &logSetLevel); > @@ -404,14 +404,7 @@ PYBIND11_MODULE(_libcamera, m) > self.size.width = std::get<0>(size); > self.size.height = std::get<1>(size); > }) > - .def_property( > - "pixel_format", > - [](StreamConfiguration &self) { > - return self.pixelFormat.toString(); > - }, > - [](StreamConfiguration &self, std::string fmt) { > - self.pixelFormat = PixelFormat::fromString(fmt); > - }) > + .def_readwrite("pixel_format", &StreamConfiguration::pixelFormat) > .def_readwrite("stride", &StreamConfiguration::stride) > .def_readwrite("frame_size", &StreamConfiguration::frameSize) > .def_readwrite("buffer_count", &StreamConfiguration::bufferCount) > @@ -420,22 +413,15 @@ PYBIND11_MODULE(_libcamera, m) > .def_readwrite("color_space", &StreamConfiguration::colorSpace); > > pyStreamFormats > - .def_property_readonly("pixel_formats", [](StreamFormats &self) { > - std::vector<std::string> fmts; > - for (auto &fmt : self.pixelformats()) > - fmts.push_back(fmt.toString()); > - return fmts; > - }) > - .def("sizes", [](StreamFormats &self, const std::string &pixelFormat) { > - auto fmt = PixelFormat::fromString(pixelFormat); > + .def_property_readonly("pixel_formats", &StreamFormats::pixelformats) > + .def("sizes", [](StreamFormats &self, const PixelFormat &pixelFormat) { > std::vector<std::tuple<uint32_t, uint32_t>> fmts; > - for (const auto &s : self.sizes(fmt)) > + for (const auto &s : self.sizes(pixelFormat)) > fmts.push_back(std::make_tuple(s.width, s.height)); > return fmts; > }) > - .def("range", [](StreamFormats &self, const std::string &pixelFormat) { > - auto fmt = PixelFormat::fromString(pixelFormat); > - const auto &range = self.range(fmt); > + .def("range", [](StreamFormats &self, const PixelFormat &pixelFormat) { > + const auto &range = self.range(pixelFormat); > return make_tuple(std::make_tuple(range.hStep, range.vStep), > std::make_tuple(range.min.width, range.min.height), > std::make_tuple(range.max.width, range.max.height)); > @@ -648,4 +634,27 @@ PYBIND11_MODULE(_libcamera, m) > pyColorSpaceRange > .value("Full", ColorSpace::Range::Full) > .value("Limited", ColorSpace::Range::Limited); > + > + pyPixelFormat > + .def(py::init<>()) > + .def(py::init<uint32_t, uint64_t>()) > + .def_property_readonly("fourcc", &PixelFormat::fourcc) > + .def_property_readonly("modifier", &PixelFormat::modifier) > + .def_property_readonly("name", &PixelFormat::toString) > + .def(py::self == py::self) > + .def("__str__", [](const PixelFormat &self) { > + return "<libcamera.PixelFormat '" + self.toString() + "'>"; > + }) I'd just return self.toString() here. We could also implement a __repr__ that prints "libcamera.PixelFormat('" + self.toString() + "')" if we add a constructor that takes a string as __repr__ is supposed to return a canonical string representation that can be run to recreate the object. > + .def_static("from_name", [](const std::string &name) { Could we name this "from_string" to avoid diverging from the C++ API ? > + return PixelFormat::fromString(name); > + }) > + .def_static("from_fourcc", [](const std::string &fourcc) { > + if (fourcc.size() != 4) > + throw std::invalid_argument("Invalid fourcc length"); > + > + uint32_t v = fourcc[0] | (fourcc[1] << 8) | > + (fourcc[2] << 16) | (fourcc[3] << 24); > + > + return PixelFormat(v); > + }); This isn't used in your series, so I'd drop it. > } > diff --git a/src/py/libcamera/utils/conv.py b/src/py/libcamera/utils/conv.py > index 2e483003..30f6733e 100644 > --- a/src/py/libcamera/utils/conv.py > +++ b/src/py/libcamera/utils/conv.py > @@ -76,6 +76,8 @@ def to_rgb(fmt, size, data): > w = size[0] > h = size[1] > > + fmt = fmt.name > + > if fmt == 'YUYV': > # YUV422 > yuyv = data.reshape((h, w // 2 * 4))
On 17/05/2022 11:32, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, May 16, 2022 at 05:10:18PM +0300, Tomi Valkeinen wrote: >> Implement PixelFormat bindings properly with a PixelFormat class. Change >> the bindings to use the new class instead of a string. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> src/py/cam/cam.py | 4 +-- >> src/py/cam/cam_kms.py | 9 +----- >> src/py/cam/cam_qt.py | 2 +- >> src/py/cam/cam_qtgl.py | 17 +----------- >> src/py/cam/gl_helpers.py | 8 ------ >> src/py/libcamera/pymain.cpp | 51 ++++++++++++++++++++-------------- >> src/py/libcamera/utils/conv.py | 2 ++ >> 7 files changed, 37 insertions(+), 56 deletions(-) >> >> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py >> index c7da97d7..63c67126 100755 >> --- a/src/py/cam/cam.py >> +++ b/src/py/cam/cam.py >> @@ -80,7 +80,7 @@ def do_cmd_info(ctx): >> >> formats = stream_config.formats >> for fmt in formats.pixel_formats: >> - print('\t * Pixelformat:', fmt, formats.range(fmt)) >> + print('\t * Pixelformat:', fmt.name, formats.range(fmt)) >> >> for size in formats.sizes(fmt): >> print('\t -', size) >> @@ -164,7 +164,7 @@ def configure(ctx): >> stream_config.size = (stream_opts['width'], stream_opts['height']) >> >> if 'pixelformat' in stream_opts: >> - stream_config.pixel_format = stream_opts['pixelformat'] >> + stream_config.pixel_format = libcam.PixelFormat.from_name(stream_opts['pixelformat']) >> >> stat = camconfig.validate() >> >> diff --git a/src/py/cam/cam_kms.py b/src/py/cam/cam_kms.py >> index ae6be277..7d11564b 100644 >> --- a/src/py/cam/cam_kms.py >> +++ b/src/py/cam/cam_kms.py >> @@ -5,13 +5,6 @@ import pykms >> import selectors >> import sys >> >> -FMT_MAP = { >> - 'RGB888': pykms.PixelFormat.RGB888, >> - 'YUYV': pykms.PixelFormat.YUYV, >> - 'ARGB8888': pykms.PixelFormat.ARGB8888, >> - 'XRGB8888': pykms.PixelFormat.XRGB8888, >> -} >> - >> >> class KMSRenderer: >> def __init__(self, state): >> @@ -118,7 +111,7 @@ class KMSRenderer: >> >> cfg = stream.configuration >> fmt = cfg.pixel_format >> - fmt = FMT_MAP[fmt] >> + fmt = pykms.PixelFormat(fmt.fourcc) >> >> plane = self.resman.reserve_generic_plane(self.crtc, fmt) >> assert(plane is not None) >> diff --git a/src/py/cam/cam_qt.py b/src/py/cam/cam_qt.py >> index dc5219eb..64f49f33 100644 >> --- a/src/py/cam/cam_qt.py >> +++ b/src/py/cam/cam_qt.py >> @@ -139,7 +139,7 @@ class MainWindow(QtWidgets.QWidget): >> w, h = cfg.size >> pitch = cfg.stride >> >> - if cfg.pixel_format == 'MJPEG': >> + if cfg.pixel_format.name == 'MJPEG': >> img = Image.open(BytesIO(mfb.planes[0])) >> qim = ImageQt(img).copy() >> pix = QtGui.QPixmap.fromImage(qim) >> diff --git a/src/py/cam/cam_qtgl.py b/src/py/cam/cam_qtgl.py >> index 8a95994e..261accb8 100644 >> --- a/src/py/cam/cam_qtgl.py >> +++ b/src/py/cam/cam_qtgl.py >> @@ -30,14 +30,6 @@ from OpenGL.GL import shaders >> >> from gl_helpers import * >> >> -# libcamera format string -> DRM fourcc >> -FMT_MAP = { >> - 'RGB888': 'RG24', >> - 'XRGB8888': 'XR24', >> - 'ARGB8888': 'AR24', >> - 'YUYV': 'YUYV', >> -} >> - >> >> class EglState: >> def __init__(self): >> @@ -204,12 +196,6 @@ class MainWindow(QtWidgets.QWidget): >> self.current[ctx['idx']] = [] >> >> for stream in ctx['streams']: >> - fmt = stream.configuration.pixel_format >> - size = stream.configuration.size >> - >> - if fmt not in FMT_MAP: >> - raise Exception('Unsupported pixel format: ' + str(fmt)) >> - >> self.textures[stream] = None >> >> num_tiles = len(self.textures) >> @@ -281,8 +267,7 @@ class MainWindow(QtWidgets.QWidget): >> >> def create_texture(self, stream, fb): >> cfg = stream.configuration >> - fmt = cfg.pixel_format >> - fmt = str_to_fourcc(FMT_MAP[fmt]) >> + fmt = cfg.pixel_format.fourcc >> w, h = cfg.size >> >> attribs = [ >> diff --git a/src/py/cam/gl_helpers.py b/src/py/cam/gl_helpers.py >> index ac5e6889..53b3e9df 100644 >> --- a/src/py/cam/gl_helpers.py >> +++ b/src/py/cam/gl_helpers.py >> @@ -30,14 +30,6 @@ def getglEGLImageTargetTexture2DOES(): >> >> glEGLImageTargetTexture2DOES = getglEGLImageTargetTexture2DOES() >> >> -# \todo This can be dropped when we have proper PixelFormat bindings >> -def str_to_fourcc(str): >> - assert(len(str) == 4) >> - fourcc = 0 >> - for i, v in enumerate([ord(c) for c in str]): >> - fourcc |= v << (i * 8) >> - return fourcc >> - >> >> def get_gl_extensions(): >> n = GLint() >> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp >> index af22205e..cc2ddee5 100644 >> --- a/src/py/libcamera/pymain.cpp >> +++ b/src/py/libcamera/pymain.cpp >> @@ -8,7 +8,6 @@ >> /* >> * \todo Add geometry classes (Point, Rectangle...) >> * \todo Add bindings for the ControlInfo class >> - * \todo Add bindings for the PixelFormat class >> */ >> >> #include <mutex> >> @@ -173,6 +172,7 @@ PYBIND11_MODULE(_libcamera, m) >> auto pyColorSpaceTransferFunction = py::enum_<ColorSpace::TransferFunction>(pyColorSpace, "TransferFunction"); >> auto pyColorSpaceYcbcrEncoding = py::enum_<ColorSpace::YcbcrEncoding>(pyColorSpace, "YcbcrEncoding"); >> auto pyColorSpaceRange = py::enum_<ColorSpace::Range>(pyColorSpace, "Range"); >> + auto pyPixelFormat = py::class_<PixelFormat>(m, "PixelFormat"); >> >> /* Global functions */ >> m.def("log_set_level", &logSetLevel); >> @@ -404,14 +404,7 @@ PYBIND11_MODULE(_libcamera, m) >> self.size.width = std::get<0>(size); >> self.size.height = std::get<1>(size); >> }) >> - .def_property( >> - "pixel_format", >> - [](StreamConfiguration &self) { >> - return self.pixelFormat.toString(); >> - }, >> - [](StreamConfiguration &self, std::string fmt) { >> - self.pixelFormat = PixelFormat::fromString(fmt); >> - }) >> + .def_readwrite("pixel_format", &StreamConfiguration::pixelFormat) >> .def_readwrite("stride", &StreamConfiguration::stride) >> .def_readwrite("frame_size", &StreamConfiguration::frameSize) >> .def_readwrite("buffer_count", &StreamConfiguration::bufferCount) >> @@ -420,22 +413,15 @@ PYBIND11_MODULE(_libcamera, m) >> .def_readwrite("color_space", &StreamConfiguration::colorSpace); >> >> pyStreamFormats >> - .def_property_readonly("pixel_formats", [](StreamFormats &self) { >> - std::vector<std::string> fmts; >> - for (auto &fmt : self.pixelformats()) >> - fmts.push_back(fmt.toString()); >> - return fmts; >> - }) >> - .def("sizes", [](StreamFormats &self, const std::string &pixelFormat) { >> - auto fmt = PixelFormat::fromString(pixelFormat); >> + .def_property_readonly("pixel_formats", &StreamFormats::pixelformats) >> + .def("sizes", [](StreamFormats &self, const PixelFormat &pixelFormat) { >> std::vector<std::tuple<uint32_t, uint32_t>> fmts; >> - for (const auto &s : self.sizes(fmt)) >> + for (const auto &s : self.sizes(pixelFormat)) >> fmts.push_back(std::make_tuple(s.width, s.height)); >> return fmts; >> }) >> - .def("range", [](StreamFormats &self, const std::string &pixelFormat) { >> - auto fmt = PixelFormat::fromString(pixelFormat); >> - const auto &range = self.range(fmt); >> + .def("range", [](StreamFormats &self, const PixelFormat &pixelFormat) { >> + const auto &range = self.range(pixelFormat); >> return make_tuple(std::make_tuple(range.hStep, range.vStep), >> std::make_tuple(range.min.width, range.min.height), >> std::make_tuple(range.max.width, range.max.height)); >> @@ -648,4 +634,27 @@ PYBIND11_MODULE(_libcamera, m) >> pyColorSpaceRange >> .value("Full", ColorSpace::Range::Full) >> .value("Limited", ColorSpace::Range::Limited); >> + >> + pyPixelFormat >> + .def(py::init<>()) >> + .def(py::init<uint32_t, uint64_t>()) >> + .def_property_readonly("fourcc", &PixelFormat::fourcc) >> + .def_property_readonly("modifier", &PixelFormat::modifier) >> + .def_property_readonly("name", &PixelFormat::toString) >> + .def(py::self == py::self) >> + .def("__str__", [](const PixelFormat &self) { >> + return "<libcamera.PixelFormat '" + self.toString() + "'>"; >> + }) > > I'd just return self.toString() here. We could also implement a __repr__ > that prints "libcamera.PixelFormat('" + self.toString() + "')" if we add > a constructor that takes a string as __repr__ is supposed to return a > canonical string representation that can be run to recreate the object. Yes. I missed __repr__ here. I think I implemented __str__ and __repr__ better with the geometry classes. I'll improve this. >> + .def_static("from_name", [](const std::string &name) { > > Could we name this "from_string" to avoid diverging from the C++ API ? We could, but... The reason I didn't use "string" was that fourcc is a string too. I used "name" in the property above too. But I guess the libcamera strategy is to never expose fourcc as a string? It's always uint32_t if it's fourcc, or string if it's the "long name" of the format? I've always used fourcc as a string (e.g. in kms++) as it's easy, but perhaps the above is a better approach. >> + return PixelFormat::fromString(name); >> + }) >> + .def_static("from_fourcc", [](const std::string &fourcc) { >> + if (fourcc.size() != 4) >> + throw std::invalid_argument("Invalid fourcc length"); >> + >> + uint32_t v = fourcc[0] | (fourcc[1] << 8) | >> + (fourcc[2] << 16) | (fourcc[3] << 24); >> + >> + return PixelFormat(v); >> + }); > > This isn't used in your series, so I'd drop it. True. Tomi
Hi Tomi, On Tue, May 17, 2022 at 11:53:39AM +0300, Tomi Valkeinen wrote: > On 17/05/2022 11:32, Laurent Pinchart wrote: > > On Mon, May 16, 2022 at 05:10:18PM +0300, Tomi Valkeinen wrote: > >> Implement PixelFormat bindings properly with a PixelFormat class. Change > >> the bindings to use the new class instead of a string. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >> --- > >> src/py/cam/cam.py | 4 +-- > >> src/py/cam/cam_kms.py | 9 +----- > >> src/py/cam/cam_qt.py | 2 +- > >> src/py/cam/cam_qtgl.py | 17 +----------- > >> src/py/cam/gl_helpers.py | 8 ------ > >> src/py/libcamera/pymain.cpp | 51 ++++++++++++++++++++-------------- > >> src/py/libcamera/utils/conv.py | 2 ++ > >> 7 files changed, 37 insertions(+), 56 deletions(-) > >> > >> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py > >> index c7da97d7..63c67126 100755 > >> --- a/src/py/cam/cam.py > >> +++ b/src/py/cam/cam.py > >> @@ -80,7 +80,7 @@ def do_cmd_info(ctx): > >> > >> formats = stream_config.formats > >> for fmt in formats.pixel_formats: > >> - print('\t * Pixelformat:', fmt, formats.range(fmt)) > >> + print('\t * Pixelformat:', fmt.name, formats.range(fmt)) > >> > >> for size in formats.sizes(fmt): > >> print('\t -', size) > >> @@ -164,7 +164,7 @@ def configure(ctx): > >> stream_config.size = (stream_opts['width'], stream_opts['height']) > >> > >> if 'pixelformat' in stream_opts: > >> - stream_config.pixel_format = stream_opts['pixelformat'] > >> + stream_config.pixel_format = libcam.PixelFormat.from_name(stream_opts['pixelformat']) > >> > >> stat = camconfig.validate() > >> > >> diff --git a/src/py/cam/cam_kms.py b/src/py/cam/cam_kms.py > >> index ae6be277..7d11564b 100644 > >> --- a/src/py/cam/cam_kms.py > >> +++ b/src/py/cam/cam_kms.py > >> @@ -5,13 +5,6 @@ import pykms > >> import selectors > >> import sys > >> > >> -FMT_MAP = { > >> - 'RGB888': pykms.PixelFormat.RGB888, > >> - 'YUYV': pykms.PixelFormat.YUYV, > >> - 'ARGB8888': pykms.PixelFormat.ARGB8888, > >> - 'XRGB8888': pykms.PixelFormat.XRGB8888, > >> -} > >> - > >> > >> class KMSRenderer: > >> def __init__(self, state): > >> @@ -118,7 +111,7 @@ class KMSRenderer: > >> > >> cfg = stream.configuration > >> fmt = cfg.pixel_format > >> - fmt = FMT_MAP[fmt] > >> + fmt = pykms.PixelFormat(fmt.fourcc) > >> > >> plane = self.resman.reserve_generic_plane(self.crtc, fmt) > >> assert(plane is not None) > >> diff --git a/src/py/cam/cam_qt.py b/src/py/cam/cam_qt.py > >> index dc5219eb..64f49f33 100644 > >> --- a/src/py/cam/cam_qt.py > >> +++ b/src/py/cam/cam_qt.py > >> @@ -139,7 +139,7 @@ class MainWindow(QtWidgets.QWidget): > >> w, h = cfg.size > >> pitch = cfg.stride > >> > >> - if cfg.pixel_format == 'MJPEG': > >> + if cfg.pixel_format.name == 'MJPEG': > >> img = Image.open(BytesIO(mfb.planes[0])) > >> qim = ImageQt(img).copy() > >> pix = QtGui.QPixmap.fromImage(qim) > >> diff --git a/src/py/cam/cam_qtgl.py b/src/py/cam/cam_qtgl.py > >> index 8a95994e..261accb8 100644 > >> --- a/src/py/cam/cam_qtgl.py > >> +++ b/src/py/cam/cam_qtgl.py > >> @@ -30,14 +30,6 @@ from OpenGL.GL import shaders > >> > >> from gl_helpers import * > >> > >> -# libcamera format string -> DRM fourcc > >> -FMT_MAP = { > >> - 'RGB888': 'RG24', > >> - 'XRGB8888': 'XR24', > >> - 'ARGB8888': 'AR24', > >> - 'YUYV': 'YUYV', > >> -} > >> - > >> > >> class EglState: > >> def __init__(self): > >> @@ -204,12 +196,6 @@ class MainWindow(QtWidgets.QWidget): > >> self.current[ctx['idx']] = [] > >> > >> for stream in ctx['streams']: > >> - fmt = stream.configuration.pixel_format > >> - size = stream.configuration.size > >> - > >> - if fmt not in FMT_MAP: > >> - raise Exception('Unsupported pixel format: ' + str(fmt)) > >> - > >> self.textures[stream] = None > >> > >> num_tiles = len(self.textures) > >> @@ -281,8 +267,7 @@ class MainWindow(QtWidgets.QWidget): > >> > >> def create_texture(self, stream, fb): > >> cfg = stream.configuration > >> - fmt = cfg.pixel_format > >> - fmt = str_to_fourcc(FMT_MAP[fmt]) > >> + fmt = cfg.pixel_format.fourcc > >> w, h = cfg.size > >> > >> attribs = [ > >> diff --git a/src/py/cam/gl_helpers.py b/src/py/cam/gl_helpers.py > >> index ac5e6889..53b3e9df 100644 > >> --- a/src/py/cam/gl_helpers.py > >> +++ b/src/py/cam/gl_helpers.py > >> @@ -30,14 +30,6 @@ def getglEGLImageTargetTexture2DOES(): > >> > >> glEGLImageTargetTexture2DOES = getglEGLImageTargetTexture2DOES() > >> > >> -# \todo This can be dropped when we have proper PixelFormat bindings > >> -def str_to_fourcc(str): > >> - assert(len(str) == 4) > >> - fourcc = 0 > >> - for i, v in enumerate([ord(c) for c in str]): > >> - fourcc |= v << (i * 8) > >> - return fourcc > >> - > >> > >> def get_gl_extensions(): > >> n = GLint() > >> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp > >> index af22205e..cc2ddee5 100644 > >> --- a/src/py/libcamera/pymain.cpp > >> +++ b/src/py/libcamera/pymain.cpp > >> @@ -8,7 +8,6 @@ > >> /* > >> * \todo Add geometry classes (Point, Rectangle...) > >> * \todo Add bindings for the ControlInfo class > >> - * \todo Add bindings for the PixelFormat class > >> */ > >> > >> #include <mutex> > >> @@ -173,6 +172,7 @@ PYBIND11_MODULE(_libcamera, m) > >> auto pyColorSpaceTransferFunction = py::enum_<ColorSpace::TransferFunction>(pyColorSpace, "TransferFunction"); > >> auto pyColorSpaceYcbcrEncoding = py::enum_<ColorSpace::YcbcrEncoding>(pyColorSpace, "YcbcrEncoding"); > >> auto pyColorSpaceRange = py::enum_<ColorSpace::Range>(pyColorSpace, "Range"); > >> + auto pyPixelFormat = py::class_<PixelFormat>(m, "PixelFormat"); > >> > >> /* Global functions */ > >> m.def("log_set_level", &logSetLevel); > >> @@ -404,14 +404,7 @@ PYBIND11_MODULE(_libcamera, m) > >> self.size.width = std::get<0>(size); > >> self.size.height = std::get<1>(size); > >> }) > >> - .def_property( > >> - "pixel_format", > >> - [](StreamConfiguration &self) { > >> - return self.pixelFormat.toString(); > >> - }, > >> - [](StreamConfiguration &self, std::string fmt) { > >> - self.pixelFormat = PixelFormat::fromString(fmt); > >> - }) > >> + .def_readwrite("pixel_format", &StreamConfiguration::pixelFormat) > >> .def_readwrite("stride", &StreamConfiguration::stride) > >> .def_readwrite("frame_size", &StreamConfiguration::frameSize) > >> .def_readwrite("buffer_count", &StreamConfiguration::bufferCount) > >> @@ -420,22 +413,15 @@ PYBIND11_MODULE(_libcamera, m) > >> .def_readwrite("color_space", &StreamConfiguration::colorSpace); > >> > >> pyStreamFormats > >> - .def_property_readonly("pixel_formats", [](StreamFormats &self) { > >> - std::vector<std::string> fmts; > >> - for (auto &fmt : self.pixelformats()) > >> - fmts.push_back(fmt.toString()); > >> - return fmts; > >> - }) > >> - .def("sizes", [](StreamFormats &self, const std::string &pixelFormat) { > >> - auto fmt = PixelFormat::fromString(pixelFormat); > >> + .def_property_readonly("pixel_formats", &StreamFormats::pixelformats) > >> + .def("sizes", [](StreamFormats &self, const PixelFormat &pixelFormat) { > >> std::vector<std::tuple<uint32_t, uint32_t>> fmts; > >> - for (const auto &s : self.sizes(fmt)) > >> + for (const auto &s : self.sizes(pixelFormat)) > >> fmts.push_back(std::make_tuple(s.width, s.height)); > >> return fmts; > >> }) > >> - .def("range", [](StreamFormats &self, const std::string &pixelFormat) { > >> - auto fmt = PixelFormat::fromString(pixelFormat); > >> - const auto &range = self.range(fmt); > >> + .def("range", [](StreamFormats &self, const PixelFormat &pixelFormat) { > >> + const auto &range = self.range(pixelFormat); > >> return make_tuple(std::make_tuple(range.hStep, range.vStep), > >> std::make_tuple(range.min.width, range.min.height), > >> std::make_tuple(range.max.width, range.max.height)); > >> @@ -648,4 +634,27 @@ PYBIND11_MODULE(_libcamera, m) > >> pyColorSpaceRange > >> .value("Full", ColorSpace::Range::Full) > >> .value("Limited", ColorSpace::Range::Limited); > >> + > >> + pyPixelFormat > >> + .def(py::init<>()) > >> + .def(py::init<uint32_t, uint64_t>()) > >> + .def_property_readonly("fourcc", &PixelFormat::fourcc) > >> + .def_property_readonly("modifier", &PixelFormat::modifier) > >> + .def_property_readonly("name", &PixelFormat::toString) > >> + .def(py::self == py::self) > >> + .def("__str__", [](const PixelFormat &self) { > >> + return "<libcamera.PixelFormat '" + self.toString() + "'>"; > >> + }) > > > > I'd just return self.toString() here. We could also implement a __repr__ > > that prints "libcamera.PixelFormat('" + self.toString() + "')" if we add > > a constructor that takes a string as __repr__ is supposed to return a > > canonical string representation that can be run to recreate the object. > > Yes. I missed __repr__ here. I think I implemented __str__ and __repr__ > better with the geometry classes. I'll improve this. > > >> + .def_static("from_name", [](const std::string &name) { > > > > Could we name this "from_string" to avoid diverging from the C++ API ? > > We could, but... The reason I didn't use "string" was that fourcc is a > string too. I used "name" in the property above too. > > But I guess the libcamera strategy is to never expose fourcc as a > string? It's always uint32_t if it's fourcc, or string if it's the "long > name" of the format? > > I've always used fourcc as a string (e.g. in kms++) as it's easy, but > perhaps the above is a better approach. Correct, in libcamera we don't expose the fourcc as a string. A pixel format is defined as a combination of a fourcc and modifiers, so I think handling fourccs as string could easily get misleading. The only string for pixel formats is their canonical name. > >> + return PixelFormat::fromString(name); > >> + }) > >> + .def_static("from_fourcc", [](const std::string &fourcc) { > >> + if (fourcc.size() != 4) > >> + throw std::invalid_argument("Invalid fourcc length"); > >> + > >> + uint32_t v = fourcc[0] | (fourcc[1] << 8) | > >> + (fourcc[2] << 16) | (fourcc[3] << 24); > >> + > >> + return PixelFormat(v); > >> + }); > > > > This isn't used in your series, so I'd drop it. > > True.
diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py index c7da97d7..63c67126 100755 --- a/src/py/cam/cam.py +++ b/src/py/cam/cam.py @@ -80,7 +80,7 @@ def do_cmd_info(ctx): formats = stream_config.formats for fmt in formats.pixel_formats: - print('\t * Pixelformat:', fmt, formats.range(fmt)) + print('\t * Pixelformat:', fmt.name, formats.range(fmt)) for size in formats.sizes(fmt): print('\t -', size) @@ -164,7 +164,7 @@ def configure(ctx): stream_config.size = (stream_opts['width'], stream_opts['height']) if 'pixelformat' in stream_opts: - stream_config.pixel_format = stream_opts['pixelformat'] + stream_config.pixel_format = libcam.PixelFormat.from_name(stream_opts['pixelformat']) stat = camconfig.validate() diff --git a/src/py/cam/cam_kms.py b/src/py/cam/cam_kms.py index ae6be277..7d11564b 100644 --- a/src/py/cam/cam_kms.py +++ b/src/py/cam/cam_kms.py @@ -5,13 +5,6 @@ import pykms import selectors import sys -FMT_MAP = { - 'RGB888': pykms.PixelFormat.RGB888, - 'YUYV': pykms.PixelFormat.YUYV, - 'ARGB8888': pykms.PixelFormat.ARGB8888, - 'XRGB8888': pykms.PixelFormat.XRGB8888, -} - class KMSRenderer: def __init__(self, state): @@ -118,7 +111,7 @@ class KMSRenderer: cfg = stream.configuration fmt = cfg.pixel_format - fmt = FMT_MAP[fmt] + fmt = pykms.PixelFormat(fmt.fourcc) plane = self.resman.reserve_generic_plane(self.crtc, fmt) assert(plane is not None) diff --git a/src/py/cam/cam_qt.py b/src/py/cam/cam_qt.py index dc5219eb..64f49f33 100644 --- a/src/py/cam/cam_qt.py +++ b/src/py/cam/cam_qt.py @@ -139,7 +139,7 @@ class MainWindow(QtWidgets.QWidget): w, h = cfg.size pitch = cfg.stride - if cfg.pixel_format == 'MJPEG': + if cfg.pixel_format.name == 'MJPEG': img = Image.open(BytesIO(mfb.planes[0])) qim = ImageQt(img).copy() pix = QtGui.QPixmap.fromImage(qim) diff --git a/src/py/cam/cam_qtgl.py b/src/py/cam/cam_qtgl.py index 8a95994e..261accb8 100644 --- a/src/py/cam/cam_qtgl.py +++ b/src/py/cam/cam_qtgl.py @@ -30,14 +30,6 @@ from OpenGL.GL import shaders from gl_helpers import * -# libcamera format string -> DRM fourcc -FMT_MAP = { - 'RGB888': 'RG24', - 'XRGB8888': 'XR24', - 'ARGB8888': 'AR24', - 'YUYV': 'YUYV', -} - class EglState: def __init__(self): @@ -204,12 +196,6 @@ class MainWindow(QtWidgets.QWidget): self.current[ctx['idx']] = [] for stream in ctx['streams']: - fmt = stream.configuration.pixel_format - size = stream.configuration.size - - if fmt not in FMT_MAP: - raise Exception('Unsupported pixel format: ' + str(fmt)) - self.textures[stream] = None num_tiles = len(self.textures) @@ -281,8 +267,7 @@ class MainWindow(QtWidgets.QWidget): def create_texture(self, stream, fb): cfg = stream.configuration - fmt = cfg.pixel_format - fmt = str_to_fourcc(FMT_MAP[fmt]) + fmt = cfg.pixel_format.fourcc w, h = cfg.size attribs = [ diff --git a/src/py/cam/gl_helpers.py b/src/py/cam/gl_helpers.py index ac5e6889..53b3e9df 100644 --- a/src/py/cam/gl_helpers.py +++ b/src/py/cam/gl_helpers.py @@ -30,14 +30,6 @@ def getglEGLImageTargetTexture2DOES(): glEGLImageTargetTexture2DOES = getglEGLImageTargetTexture2DOES() -# \todo This can be dropped when we have proper PixelFormat bindings -def str_to_fourcc(str): - assert(len(str) == 4) - fourcc = 0 - for i, v in enumerate([ord(c) for c in str]): - fourcc |= v << (i * 8) - return fourcc - def get_gl_extensions(): n = GLint() diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp index af22205e..cc2ddee5 100644 --- a/src/py/libcamera/pymain.cpp +++ b/src/py/libcamera/pymain.cpp @@ -8,7 +8,6 @@ /* * \todo Add geometry classes (Point, Rectangle...) * \todo Add bindings for the ControlInfo class - * \todo Add bindings for the PixelFormat class */ #include <mutex> @@ -173,6 +172,7 @@ PYBIND11_MODULE(_libcamera, m) auto pyColorSpaceTransferFunction = py::enum_<ColorSpace::TransferFunction>(pyColorSpace, "TransferFunction"); auto pyColorSpaceYcbcrEncoding = py::enum_<ColorSpace::YcbcrEncoding>(pyColorSpace, "YcbcrEncoding"); auto pyColorSpaceRange = py::enum_<ColorSpace::Range>(pyColorSpace, "Range"); + auto pyPixelFormat = py::class_<PixelFormat>(m, "PixelFormat"); /* Global functions */ m.def("log_set_level", &logSetLevel); @@ -404,14 +404,7 @@ PYBIND11_MODULE(_libcamera, m) self.size.width = std::get<0>(size); self.size.height = std::get<1>(size); }) - .def_property( - "pixel_format", - [](StreamConfiguration &self) { - return self.pixelFormat.toString(); - }, - [](StreamConfiguration &self, std::string fmt) { - self.pixelFormat = PixelFormat::fromString(fmt); - }) + .def_readwrite("pixel_format", &StreamConfiguration::pixelFormat) .def_readwrite("stride", &StreamConfiguration::stride) .def_readwrite("frame_size", &StreamConfiguration::frameSize) .def_readwrite("buffer_count", &StreamConfiguration::bufferCount) @@ -420,22 +413,15 @@ PYBIND11_MODULE(_libcamera, m) .def_readwrite("color_space", &StreamConfiguration::colorSpace); pyStreamFormats - .def_property_readonly("pixel_formats", [](StreamFormats &self) { - std::vector<std::string> fmts; - for (auto &fmt : self.pixelformats()) - fmts.push_back(fmt.toString()); - return fmts; - }) - .def("sizes", [](StreamFormats &self, const std::string &pixelFormat) { - auto fmt = PixelFormat::fromString(pixelFormat); + .def_property_readonly("pixel_formats", &StreamFormats::pixelformats) + .def("sizes", [](StreamFormats &self, const PixelFormat &pixelFormat) { std::vector<std::tuple<uint32_t, uint32_t>> fmts; - for (const auto &s : self.sizes(fmt)) + for (const auto &s : self.sizes(pixelFormat)) fmts.push_back(std::make_tuple(s.width, s.height)); return fmts; }) - .def("range", [](StreamFormats &self, const std::string &pixelFormat) { - auto fmt = PixelFormat::fromString(pixelFormat); - const auto &range = self.range(fmt); + .def("range", [](StreamFormats &self, const PixelFormat &pixelFormat) { + const auto &range = self.range(pixelFormat); return make_tuple(std::make_tuple(range.hStep, range.vStep), std::make_tuple(range.min.width, range.min.height), std::make_tuple(range.max.width, range.max.height)); @@ -648,4 +634,27 @@ PYBIND11_MODULE(_libcamera, m) pyColorSpaceRange .value("Full", ColorSpace::Range::Full) .value("Limited", ColorSpace::Range::Limited); + + pyPixelFormat + .def(py::init<>()) + .def(py::init<uint32_t, uint64_t>()) + .def_property_readonly("fourcc", &PixelFormat::fourcc) + .def_property_readonly("modifier", &PixelFormat::modifier) + .def_property_readonly("name", &PixelFormat::toString) + .def(py::self == py::self) + .def("__str__", [](const PixelFormat &self) { + return "<libcamera.PixelFormat '" + self.toString() + "'>"; + }) + .def_static("from_name", [](const std::string &name) { + return PixelFormat::fromString(name); + }) + .def_static("from_fourcc", [](const std::string &fourcc) { + if (fourcc.size() != 4) + throw std::invalid_argument("Invalid fourcc length"); + + uint32_t v = fourcc[0] | (fourcc[1] << 8) | + (fourcc[2] << 16) | (fourcc[3] << 24); + + return PixelFormat(v); + }); } diff --git a/src/py/libcamera/utils/conv.py b/src/py/libcamera/utils/conv.py index 2e483003..30f6733e 100644 --- a/src/py/libcamera/utils/conv.py +++ b/src/py/libcamera/utils/conv.py @@ -76,6 +76,8 @@ def to_rgb(fmt, size, data): w = size[0] h = size[1] + fmt = fmt.name + if fmt == 'YUYV': # YUV422 yuyv = data.reshape((h, w // 2 * 4))
Implement PixelFormat bindings properly with a PixelFormat class. Change the bindings to use the new class instead of a string. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- src/py/cam/cam.py | 4 +-- src/py/cam/cam_kms.py | 9 +----- src/py/cam/cam_qt.py | 2 +- src/py/cam/cam_qtgl.py | 17 +----------- src/py/cam/gl_helpers.py | 8 ------ src/py/libcamera/pymain.cpp | 51 ++++++++++++++++++++-------------- src/py/libcamera/utils/conv.py | 2 ++ 7 files changed, 37 insertions(+), 56 deletions(-)