[libcamera-devel] [PATCH 10/14] py: implement PixelFormat class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue May 17 10:58:00 CEST 2022
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 at 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.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list