| Message ID | 20220429212348.18063-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Sat, Apr 30, 2022 at 12:23:43AM +0300, Laurent Pinchart via libcamera-devel wrote: > Hello, > > This patch series extends Han-Lin's operator<<() implementation for > geometry classes to format classes. There isn't much else to mention in > the cover letter, please see individual patches for details. > We now have both .toString() and operator<< for printing. Is this intentional ? Shouldn't we stabilize on only one of the two ? > Laurent Pinchart (5): > libcamera: geometry: Add missing ostream header in geometry.h > libcamera: Add operator<<() for pixel format classes > libcamera: bayer_format: Add operator<<() > libcamera: Add operator<<() for V4L2 format classes > libcamera: Replace toString with operator<<() for format classes > > include/libcamera/geometry.h | 1 + > include/libcamera/internal/bayer_format.h | 3 + > include/libcamera/internal/v4l2_pixelformat.h | 3 + > include/libcamera/internal/v4l2_subdevice.h | 3 + > include/libcamera/internal/v4l2_videodevice.h | 3 + > include/libcamera/pixel_format.h | 3 + > src/android/camera_capabilities.cpp | 6 +- > src/android/camera_device.cpp | 4 +- > src/android/jpeg/encoder_libjpeg.cpp | 2 +- > src/android/jpeg/thumbnailer.cpp | 2 +- > src/android/mm/generic_camera_buffer.cpp | 3 +- > src/android/yuv/post_processor_yuv.cpp | 4 +- > src/cam/camera_session.cpp | 2 +- > src/cam/kms_sink.cpp | 2 +- > src/libcamera/bayer_format.cpp | 57 ++++++++++++------- > src/libcamera/pipeline/ipu3/cio2.cpp | 2 +- > src/libcamera/pipeline/ipu3/imgu.cpp | 6 +- > .../pipeline/raspberrypi/raspberrypi.cpp | 20 +++---- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++-- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 +- > src/libcamera/pipeline/simple/converter.cpp | 4 +- > src/libcamera/pipeline/simple/simple.cpp | 9 ++- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +- > src/libcamera/pipeline/vimc/vimc.cpp | 2 +- > src/libcamera/pixel_format.cpp | 12 ++++ > src/libcamera/v4l2_pixelformat.cpp | 13 +++++ > src/libcamera/v4l2_subdevice.cpp | 36 ++++++++---- > src/libcamera/v4l2_videodevice.cpp | 16 +++++- > src/qcam/viewfinder_qt.cpp | 3 +- > src/v4l2/v4l2_camera_proxy.cpp | 2 +- > test/bayer-format.cpp | 24 ++++---- > test/camera-sensor.cpp | 2 +- > 32 files changed, 175 insertions(+), 95 deletions(-) > > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Tue, May 03, 2022 at 09:04:14AM +0200, Jacopo Mondi wrote: > Hi Laurent, > > On Sat, Apr 30, 2022 at 12:23:43AM +0300, Laurent Pinchart via libcamera-devel wrote: > > Hello, > > > > This patch series extends Han-Lin's operator<<() implementation for > > geometry classes to format classes. There isn't much else to mention in > > the cover letter, please see individual patches for details. > > We now have both .toString() and operator<< for printing. Is this > intentional ? Shouldn't we stabilize on only one of the two ? I think both make sense (as long as one of the two is implemented based on the other internally), as they serve different purposes. operator<<() certainly simplifies logging, but not all string representations are used in logs. toString() has few users so far, mostly in tests, for instance in if (PixelFormat().toString() != "<INVALID>") { cerr << "Failed to convert default PixelFormat to string" << endl; return TestFail; } It would need to be written as std::stringstream ss; ss << PixelFormat(); if (ss.str() != "<INVALID>") { ... } which isn't very convenient. I can imagine more use cases in applications, for instance in qcam where available resolutions could be displayed in the UI. > > Laurent Pinchart (5): > > libcamera: geometry: Add missing ostream header in geometry.h > > libcamera: Add operator<<() for pixel format classes > > libcamera: bayer_format: Add operator<<() > > libcamera: Add operator<<() for V4L2 format classes > > libcamera: Replace toString with operator<<() for format classes > > > > include/libcamera/geometry.h | 1 + > > include/libcamera/internal/bayer_format.h | 3 + > > include/libcamera/internal/v4l2_pixelformat.h | 3 + > > include/libcamera/internal/v4l2_subdevice.h | 3 + > > include/libcamera/internal/v4l2_videodevice.h | 3 + > > include/libcamera/pixel_format.h | 3 + > > src/android/camera_capabilities.cpp | 6 +- > > src/android/camera_device.cpp | 4 +- > > src/android/jpeg/encoder_libjpeg.cpp | 2 +- > > src/android/jpeg/thumbnailer.cpp | 2 +- > > src/android/mm/generic_camera_buffer.cpp | 3 +- > > src/android/yuv/post_processor_yuv.cpp | 4 +- > > src/cam/camera_session.cpp | 2 +- > > src/cam/kms_sink.cpp | 2 +- > > src/libcamera/bayer_format.cpp | 57 ++++++++++++------- > > src/libcamera/pipeline/ipu3/cio2.cpp | 2 +- > > src/libcamera/pipeline/ipu3/imgu.cpp | 6 +- > > .../pipeline/raspberrypi/raspberrypi.cpp | 20 +++---- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++-- > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 +- > > src/libcamera/pipeline/simple/converter.cpp | 4 +- > > src/libcamera/pipeline/simple/simple.cpp | 9 ++- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +- > > src/libcamera/pipeline/vimc/vimc.cpp | 2 +- > > src/libcamera/pixel_format.cpp | 12 ++++ > > src/libcamera/v4l2_pixelformat.cpp | 13 +++++ > > src/libcamera/v4l2_subdevice.cpp | 36 ++++++++---- > > src/libcamera/v4l2_videodevice.cpp | 16 +++++- > > src/qcam/viewfinder_qt.cpp | 3 +- > > src/v4l2/v4l2_camera_proxy.cpp | 2 +- > > test/bayer-format.cpp | 24 ++++---- > > test/camera-sensor.cpp | 2 +- > > 32 files changed, 175 insertions(+), 95 deletions(-)
Quoting Laurent Pinchart via libcamera-devel (2022-05-03 15:39:44) > Hi Jacopo, > > On Tue, May 03, 2022 at 09:04:14AM +0200, Jacopo Mondi wrote: > > Hi Laurent, > > > > On Sat, Apr 30, 2022 at 12:23:43AM +0300, Laurent Pinchart via libcamera-devel wrote: > > > Hello, > > > > > > This patch series extends Han-Lin's operator<<() implementation for > > > geometry classes to format classes. There isn't much else to mention in > > > the cover letter, please see individual patches for details. > > > > We now have both .toString() and operator<< for printing. Is this > > intentional ? Shouldn't we stabilize on only one of the two ? > > I think both make sense (as long as one of the two is implemented based > on the other internally), as they serve different purposes. operator<<() > certainly simplifies logging, but not all string representations are > used in logs. toString() has few users so far, mostly in tests, for > instance in > > if (PixelFormat().toString() != "<INVALID>") { > cerr << "Failed to convert default PixelFormat to string" > << endl; > return TestFail; > } > > It would need to be written as > > std::stringstream ss; > ss << PixelFormat(); > if (ss.str() != "<INVALID>") { > ... > } > > which isn't very convenient. I can imagine more use cases in > applications, for instance in qcam where available resolutions could be > displayed in the UI. Yes, I agree/think both are useful in different situations - So I'm glad we're updating to have both. -- Kieran > > > > Laurent Pinchart (5): > > > libcamera: geometry: Add missing ostream header in geometry.h > > > libcamera: Add operator<<() for pixel format classes > > > libcamera: bayer_format: Add operator<<() > > > libcamera: Add operator<<() for V4L2 format classes > > > libcamera: Replace toString with operator<<() for format classes > > > > > > include/libcamera/geometry.h | 1 + > > > include/libcamera/internal/bayer_format.h | 3 + > > > include/libcamera/internal/v4l2_pixelformat.h | 3 + > > > include/libcamera/internal/v4l2_subdevice.h | 3 + > > > include/libcamera/internal/v4l2_videodevice.h | 3 + > > > include/libcamera/pixel_format.h | 3 + > > > src/android/camera_capabilities.cpp | 6 +- > > > src/android/camera_device.cpp | 4 +- > > > src/android/jpeg/encoder_libjpeg.cpp | 2 +- > > > src/android/jpeg/thumbnailer.cpp | 2 +- > > > src/android/mm/generic_camera_buffer.cpp | 3 +- > > > src/android/yuv/post_processor_yuv.cpp | 4 +- > > > src/cam/camera_session.cpp | 2 +- > > > src/cam/kms_sink.cpp | 2 +- > > > src/libcamera/bayer_format.cpp | 57 ++++++++++++------- > > > src/libcamera/pipeline/ipu3/cio2.cpp | 2 +- > > > src/libcamera/pipeline/ipu3/imgu.cpp | 6 +- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 20 +++---- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++-- > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 +- > > > src/libcamera/pipeline/simple/converter.cpp | 4 +- > > > src/libcamera/pipeline/simple/simple.cpp | 9 ++- > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +- > > > src/libcamera/pipeline/vimc/vimc.cpp | 2 +- > > > src/libcamera/pixel_format.cpp | 12 ++++ > > > src/libcamera/v4l2_pixelformat.cpp | 13 +++++ > > > src/libcamera/v4l2_subdevice.cpp | 36 ++++++++---- > > > src/libcamera/v4l2_videodevice.cpp | 16 +++++- > > > src/qcam/viewfinder_qt.cpp | 3 +- > > > src/v4l2/v4l2_camera_proxy.cpp | 2 +- > > > test/bayer-format.cpp | 24 ++++---- > > > test/camera-sensor.cpp | 2 +- > > > 32 files changed, 175 insertions(+), 95 deletions(-) > > -- > Regards, > > Laurent Pinchart
Ah, right, different use cases, so having both is fine For the series: add Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j On Tue, May 03, 2022 at 05:39:44PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Tue, May 03, 2022 at 09:04:14AM +0200, Jacopo Mondi wrote: > > Hi Laurent, > > > > On Sat, Apr 30, 2022 at 12:23:43AM +0300, Laurent Pinchart via libcamera-devel wrote: > > > Hello, > > > > > > This patch series extends Han-Lin's operator<<() implementation for > > > geometry classes to format classes. There isn't much else to mention in > > > the cover letter, please see individual patches for details. > > > > We now have both .toString() and operator<< for printing. Is this > > intentional ? Shouldn't we stabilize on only one of the two ? > > I think both make sense (as long as one of the two is implemented based > on the other internally), as they serve different purposes. operator<<() > certainly simplifies logging, but not all string representations are > used in logs. toString() has few users so far, mostly in tests, for > instance in > > if (PixelFormat().toString() != "<INVALID>") { > cerr << "Failed to convert default PixelFormat to string" > << endl; > return TestFail; > } > > It would need to be written as > > std::stringstream ss; > ss << PixelFormat(); > if (ss.str() != "<INVALID>") { > ... > } > > which isn't very convenient. I can imagine more use cases in > applications, for instance in qcam where available resolutions could be > displayed in the UI. > > > > Laurent Pinchart (5): > > > libcamera: geometry: Add missing ostream header in geometry.h > > > libcamera: Add operator<<() for pixel format classes > > > libcamera: bayer_format: Add operator<<() > > > libcamera: Add operator<<() for V4L2 format classes > > > libcamera: Replace toString with operator<<() for format classes > > > > > > include/libcamera/geometry.h | 1 + > > > include/libcamera/internal/bayer_format.h | 3 + > > > include/libcamera/internal/v4l2_pixelformat.h | 3 + > > > include/libcamera/internal/v4l2_subdevice.h | 3 + > > > include/libcamera/internal/v4l2_videodevice.h | 3 + > > > include/libcamera/pixel_format.h | 3 + > > > src/android/camera_capabilities.cpp | 6 +- > > > src/android/camera_device.cpp | 4 +- > > > src/android/jpeg/encoder_libjpeg.cpp | 2 +- > > > src/android/jpeg/thumbnailer.cpp | 2 +- > > > src/android/mm/generic_camera_buffer.cpp | 3 +- > > > src/android/yuv/post_processor_yuv.cpp | 4 +- > > > src/cam/camera_session.cpp | 2 +- > > > src/cam/kms_sink.cpp | 2 +- > > > src/libcamera/bayer_format.cpp | 57 ++++++++++++------- > > > src/libcamera/pipeline/ipu3/cio2.cpp | 2 +- > > > src/libcamera/pipeline/ipu3/imgu.cpp | 6 +- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 20 +++---- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++-- > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 +- > > > src/libcamera/pipeline/simple/converter.cpp | 4 +- > > > src/libcamera/pipeline/simple/simple.cpp | 9 ++- > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +- > > > src/libcamera/pipeline/vimc/vimc.cpp | 2 +- > > > src/libcamera/pixel_format.cpp | 12 ++++ > > > src/libcamera/v4l2_pixelformat.cpp | 13 +++++ > > > src/libcamera/v4l2_subdevice.cpp | 36 ++++++++---- > > > src/libcamera/v4l2_videodevice.cpp | 16 +++++- > > > src/qcam/viewfinder_qt.cpp | 3 +- > > > src/v4l2/v4l2_camera_proxy.cpp | 2 +- > > > test/bayer-format.cpp | 24 ++++---- > > > test/camera-sensor.cpp | 2 +- > > > 32 files changed, 175 insertions(+), 95 deletions(-) > > -- > Regards, > > Laurent Pinchart