| Message ID | 20230901150215.11585-13-jacopo.mondi@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On 01/09/2023 18:02, Jacopo Mondi via libcamera-devel wrote: > Add a '--orientation|-o' option to the Python version of the cam test > application to set an orientation to the image stream. > > Supported values are: > - Rot180: Rotate 180 degrees > - Flip: vertical flip > - Mirror: horizontal flip > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/py/cam/cam.py | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py > index a2a115c164b4..84d8ca1716fd 100755 > --- a/src/py/cam/cam.py > +++ b/src/py/cam/cam.py > @@ -23,6 +23,7 @@ class CameraContext: > opt_metadata: bool > opt_save_frames: bool > opt_capture: int > + opt_orientation: str > > stream_names: dict[libcam.Stream, str] > streams: list[libcam.Stream] > @@ -146,6 +147,20 @@ class CameraContext: > if 'pixelformat' in stream_opts: > stream_config.pixel_format = libcam.PixelFormat(stream_opts['pixelformat']) > > + if self.opt_orientation is not None: > + orientation_map = { > + 'Rot180': libcam.Orientation.Rotate180, > + 'Mirror': libcam.Orientation.Rotate0Flip, > + 'Flip': libcam.Orientation.Rotate180Flip, > + } Any reason to not support all orientations? In python you can get the name of the enum as a string, and you could just compare that directly to the string from the user. Also, you could lower-case both before comparison, so that the user could say "-o rotate270". > + > + orient = orientation_map.get(self.opt_orientation, None) > + if orient is None: > + print('Bad orientation: ', self.opt_orientation) > + sys.exit(-1) > + > + camconfig.orientation = orient > + > stat = camconfig.validate() > > if stat == libcam.CameraConfiguration.Status.Invalid: > @@ -385,6 +400,7 @@ def main(): > parser.add_argument('--metadata', nargs=0, type=bool, action=CustomAction, help='Print the metadata for completed requests') > parser.add_argument('--strict-formats', type=bool, nargs=0, action=CustomAction, help='Do not allow requested stream format(s) to be adjusted') > parser.add_argument('-s', '--stream', nargs='+', action=CustomAction) > + parser.add_argument('-o', '--orientation', help='Desired image orientation (Rot180, Mirror, Flip)') Hmm, you need to use the action=CustomAction to get the option to apply to a camera context. And then I think you can do this below: ctx.opt_orientation = args.orientation.get(cam_idx, None) > args = parser.parse_args() > > cm = libcam.CameraManager.singleton() > @@ -408,6 +424,10 @@ def main(): > ctx.opt_metadata = args.metadata.get(cam_idx, False) > ctx.opt_strict_formats = args.strict_formats.get(cam_idx, False) > ctx.opt_stream = args.stream.get(cam_idx, ['role=viewfinder']) > + if args.orientation is not None: > + ctx.opt_orientation = args.orientation > + else: > + ctx.opt_orientation = None > contexts.append(ctx) > > for ctx in contexts:
Hi Tomi On Mon, Sep 04, 2023 at 08:18:11PM +0300, Tomi Valkeinen wrote: > On 01/09/2023 18:02, Jacopo Mondi via libcamera-devel wrote: > > Add a '--orientation|-o' option to the Python version of the cam test > > application to set an orientation to the image stream. > > > > Supported values are: > > - Rot180: Rotate 180 degrees > > - Flip: vertical flip > > - Mirror: horizontal flip > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > src/py/cam/cam.py | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py > > index a2a115c164b4..84d8ca1716fd 100755 > > --- a/src/py/cam/cam.py > > +++ b/src/py/cam/cam.py > > @@ -23,6 +23,7 @@ class CameraContext: > > opt_metadata: bool > > opt_save_frames: bool > > opt_capture: int > > + opt_orientation: str > > stream_names: dict[libcam.Stream, str] > > streams: list[libcam.Stream] > > @@ -146,6 +147,20 @@ class CameraContext: > > if 'pixelformat' in stream_opts: > > stream_config.pixel_format = libcam.PixelFormat(stream_opts['pixelformat']) > > + if self.opt_orientation is not None: > > + orientation_map = { > > + 'Rot180': libcam.Orientation.Rotate180, > > + 'Mirror': libcam.Orientation.Rotate0Flip, > > + 'Flip': libcam.Orientation.Rotate180Flip, > > + } > > Any reason to not support all orientations? In python you can get the name > of the enum as a string, and you could just compare that directly to the > string from the user. Also, you could lower-case both before comparison, so > that the user could say "-o rotate270". > The three supported options are the ones supporte by the C++ version of cam, and I like the two to behave the same. The reason why the C++ version of cam only supports the three above options is because they're usually the most common features as user expects. The list could indeed be expanded, but for now I would like the two versions to behave the same. Is this ok with you ? > > + > > + orient = orientation_map.get(self.opt_orientation, None) > > + if orient is None: > > + print('Bad orientation: ', self.opt_orientation) > > + sys.exit(-1) > > + > > + camconfig.orientation = orient > > + > > stat = camconfig.validate() > > if stat == libcam.CameraConfiguration.Status.Invalid: > > @@ -385,6 +400,7 @@ def main(): > > parser.add_argument('--metadata', nargs=0, type=bool, action=CustomAction, help='Print the metadata for completed requests') > > parser.add_argument('--strict-formats', type=bool, nargs=0, action=CustomAction, help='Do not allow requested stream format(s) to be adjusted') > > parser.add_argument('-s', '--stream', nargs='+', action=CustomAction) > > + parser.add_argument('-o', '--orientation', help='Desired image orientation (Rot180, Mirror, Flip)') > > Hmm, you need to use the action=CustomAction to get the option to apply to a > camera context. Uh right, orientation is indeed per camera > > And then I think you can do this below: > > ctx.opt_orientation = args.orientation.get(cam_idx, None) > Thanks j > > args = parser.parse_args() > > cm = libcam.CameraManager.singleton() > > @@ -408,6 +424,10 @@ def main(): > > ctx.opt_metadata = args.metadata.get(cam_idx, False) > > ctx.opt_strict_formats = args.strict_formats.get(cam_idx, False) > > ctx.opt_stream = args.stream.get(cam_idx, ['role=viewfinder']) > > + if args.orientation is not None: > > + ctx.opt_orientation = args.orientation > > + else: > > + ctx.opt_orientation = None > > contexts.append(ctx) > > for ctx in contexts: >
On 04/09/2023 22:07, Jacopo Mondi wrote: > Hi Tomi > > On Mon, Sep 04, 2023 at 08:18:11PM +0300, Tomi Valkeinen wrote: >> On 01/09/2023 18:02, Jacopo Mondi via libcamera-devel wrote: >>> Add a '--orientation|-o' option to the Python version of the cam test >>> application to set an orientation to the image stream. >>> >>> Supported values are: >>> - Rot180: Rotate 180 degrees >>> - Flip: vertical flip >>> - Mirror: horizontal flip >>> >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>> --- >>> src/py/cam/cam.py | 20 ++++++++++++++++++++ >>> 1 file changed, 20 insertions(+) >>> >>> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py >>> index a2a115c164b4..84d8ca1716fd 100755 >>> --- a/src/py/cam/cam.py >>> +++ b/src/py/cam/cam.py >>> @@ -23,6 +23,7 @@ class CameraContext: >>> opt_metadata: bool >>> opt_save_frames: bool >>> opt_capture: int >>> + opt_orientation: str >>> stream_names: dict[libcam.Stream, str] >>> streams: list[libcam.Stream] >>> @@ -146,6 +147,20 @@ class CameraContext: >>> if 'pixelformat' in stream_opts: >>> stream_config.pixel_format = libcam.PixelFormat(stream_opts['pixelformat']) >>> + if self.opt_orientation is not None: >>> + orientation_map = { >>> + 'Rot180': libcam.Orientation.Rotate180, >>> + 'Mirror': libcam.Orientation.Rotate0Flip, >>> + 'Flip': libcam.Orientation.Rotate180Flip, >>> + } >> >> Any reason to not support all orientations? In python you can get the name >> of the enum as a string, and you could just compare that directly to the >> string from the user. Also, you could lower-case both before comparison, so >> that the user could say "-o rotate270". >> > > The three supported options are the ones supporte by the C++ version > of cam, and I like the two to behave the same. The reason why the C++ > version of cam only supports the three above options is because > they're usually the most common features as user expects. The list > could indeed be expanded, but for now I would like the two versions to > behave the same. Is this ok with you ? Well... cam.py resembles the C++ version, but it's not identical. I initially thought I'd write a Python clone of the C++ cam, but then I realized that perhaps it doesn't make sense, and instead it's better to write a cam.py that is more Pythonic, does things differently than the C++ version when it makes sense, and, when possible, does extra things that are easy to do in Python. Generally speaking, I'd rather support all features, as otherwise will they ever be tested? If you use the enum names directly, you can just do: self.opt_orientation.lower() in [x.lower() for x in libcam.Orientation.__members__] However, I now realized you used different naming in the option, compared to the enum (e.g. mirror vs rotate0flip). For that you, of course, need a mapping there. And if that's the target, then I think the above code is fine. However, and this goes a bit outside the topic here, why do you expose the enums via different names? Or, if those names are better, then why not use those "better" names for the enum? And if you specifically want to use EXIF spec names for the enum, then wouldn't it be better to expose the orientation in cam/cam.py also with the EXIF spec names? Tomi
Hi Tomi On Tue, Sep 05, 2023 at 10:51:51AM +0300, Tomi Valkeinen via libcamera-devel wrote: > On 04/09/2023 22:07, Jacopo Mondi wrote: > > Hi Tomi > > > > On Mon, Sep 04, 2023 at 08:18:11PM +0300, Tomi Valkeinen wrote: > > > On 01/09/2023 18:02, Jacopo Mondi via libcamera-devel wrote: > > > > Add a '--orientation|-o' option to the Python version of the cam test > > > > application to set an orientation to the image stream. > > > > > > > > Supported values are: > > > > - Rot180: Rotate 180 degrees > > > > - Flip: vertical flip > > > > - Mirror: horizontal flip > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > > > src/py/cam/cam.py | 20 ++++++++++++++++++++ > > > > 1 file changed, 20 insertions(+) > > > > > > > > diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py > > > > index a2a115c164b4..84d8ca1716fd 100755 > > > > --- a/src/py/cam/cam.py > > > > +++ b/src/py/cam/cam.py > > > > @@ -23,6 +23,7 @@ class CameraContext: > > > > opt_metadata: bool > > > > opt_save_frames: bool > > > > opt_capture: int > > > > + opt_orientation: str > > > > stream_names: dict[libcam.Stream, str] > > > > streams: list[libcam.Stream] > > > > @@ -146,6 +147,20 @@ class CameraContext: > > > > if 'pixelformat' in stream_opts: > > > > stream_config.pixel_format = libcam.PixelFormat(stream_opts['pixelformat']) > > > > + if self.opt_orientation is not None: > > > > + orientation_map = { > > > > + 'Rot180': libcam.Orientation.Rotate180, > > > > + 'Mirror': libcam.Orientation.Rotate0Flip, > > > > + 'Flip': libcam.Orientation.Rotate180Flip, > > > > + } > > > > > > Any reason to not support all orientations? In python you can get the name > > > of the enum as a string, and you could just compare that directly to the > > > string from the user. Also, you could lower-case both before comparison, so > > > that the user could say "-o rotate270". > > > > > > > The three supported options are the ones supporte by the C++ version > > of cam, and I like the two to behave the same. The reason why the C++ > > version of cam only supports the three above options is because > > they're usually the most common features as user expects. The list > > could indeed be expanded, but for now I would like the two versions to > > behave the same. Is this ok with you ? > > Well... cam.py resembles the C++ version, but it's not identical. I > initially thought I'd write a Python clone of the C++ cam, but then I > realized that perhaps it doesn't make sense, and instead it's better to > write a cam.py that is more Pythonic, does things differently than the C++ > version when it makes sense, and, when possible, does extra things that are > easy to do in Python. > > Generally speaking, I'd rather support all features, as otherwise will they > ever be tested? I understand this point, but the only other Orientation that does not include a transpose is: rotate0 (Identity) (which is equivalent to not specifying a --orientation). All the other ones require a Transpose, which none of our pipelines can do. > > If you use the enum names directly, you can just do: > > self.opt_orientation.lower() in [x.lower() for x in > libcam.Orientation.__members__] > > However, I now realized you used different naming in the option, compared to > the enum (e.g. mirror vs rotate0flip). For that you, of course, need a > mapping there. And if that's the target, then I think the above code is > fine. > > However, and this goes a bit outside the topic here, why do you expose the > enums via different names? Or, if those names are better, then why not use > those "better" names for the enum? And if you specifically want to use EXIF > spec names for the enum, then wouldn't it be better to expose the > orientation in cam/cam.py also with the EXIF spec names? EXIF specs do not define names but only the numerical identifiers, and I considered 'mirror' more intuitive than 'rotate0Flip' for users. > > Tomi >
diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py index a2a115c164b4..84d8ca1716fd 100755 --- a/src/py/cam/cam.py +++ b/src/py/cam/cam.py @@ -23,6 +23,7 @@ class CameraContext: opt_metadata: bool opt_save_frames: bool opt_capture: int + opt_orientation: str stream_names: dict[libcam.Stream, str] streams: list[libcam.Stream] @@ -146,6 +147,20 @@ class CameraContext: if 'pixelformat' in stream_opts: stream_config.pixel_format = libcam.PixelFormat(stream_opts['pixelformat']) + if self.opt_orientation is not None: + orientation_map = { + 'Rot180': libcam.Orientation.Rotate180, + 'Mirror': libcam.Orientation.Rotate0Flip, + 'Flip': libcam.Orientation.Rotate180Flip, + } + + orient = orientation_map.get(self.opt_orientation, None) + if orient is None: + print('Bad orientation: ', self.opt_orientation) + sys.exit(-1) + + camconfig.orientation = orient + stat = camconfig.validate() if stat == libcam.CameraConfiguration.Status.Invalid: @@ -385,6 +400,7 @@ def main(): parser.add_argument('--metadata', nargs=0, type=bool, action=CustomAction, help='Print the metadata for completed requests') parser.add_argument('--strict-formats', type=bool, nargs=0, action=CustomAction, help='Do not allow requested stream format(s) to be adjusted') parser.add_argument('-s', '--stream', nargs='+', action=CustomAction) + parser.add_argument('-o', '--orientation', help='Desired image orientation (Rot180, Mirror, Flip)') args = parser.parse_args() cm = libcam.CameraManager.singleton() @@ -408,6 +424,10 @@ def main(): ctx.opt_metadata = args.metadata.get(cam_idx, False) ctx.opt_strict_formats = args.strict_formats.get(cam_idx, False) ctx.opt_stream = args.stream.get(cam_idx, ['role=viewfinder']) + if args.orientation is not None: + ctx.opt_orientation = args.orientation + else: + ctx.opt_orientation = None contexts.append(ctx) for ctx in contexts:
Add a '--orientation|-o' option to the Python version of the cam test application to set an orientation to the image stream. Supported values are: - Rot180: Rotate 180 degrees - Flip: vertical flip - Mirror: horizontal flip Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- src/py/cam/cam.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)