[libcamera-devel] [PATCH 10/14] py: implement PixelFormat class
Tomi Valkeinen
tomi.valkeinen at ideasonboard.com
Tue May 17 10:53:39 CEST 2022
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 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.
>> + 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
More information about the libcamera-devel
mailing list