| Message ID | 20230901150215.11585-13-jacopo.mondi@ideasonboard.com |
|---|---|
| State | Accepted |
| 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 >
On Wed, Sep 06, 2023 at 09:10:10AM +0200, Jacopo Mondi via libcamera-devel wrote: > 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: > > > 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 ? Could we at least have the same case for the C++ and Python applications ? Having to write '-o mirror' for one and '-o Mirror' for the other isn't nice. And I also wonder if we should also support a rot0 value. It would allow users of the application to unconditionally pass the -o option, which may make usage of cam/cam.py easier from scripts. > > 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 wish EXIF had names. As far as I can tell, there's no standardized names for orientations, which leads to everybody using their own naming scheme :-( > I considered 'mirror' more intuitive than 'rotate0Flip' for users. Are users of cam and cam.py that different from applications developers ? If not, maybe libcamera::Orientation::Mirror would also be a more intuitive name than rotate0Flip ?
On Thu, Oct 19, 2023 at 12:40:34AM +0300, Laurent Pinchart wrote: > On Wed, Sep 06, 2023 at 09:10:10AM +0200, Jacopo Mondi via libcamera-devel wrote: > > 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: > > > > 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 ? > > Could we at least have the same case for the C++ and Python applications > ? Having to write '-o mirror' for one and '-o Mirror' for the other > isn't nice. > Yes, I don't remeber why I went for capital letter for the py implementation.. > And I also wonder if we should also support a rot0 value. It would allow > users of the application to unconditionally pass the -o option, which > may make usage of cam/cam.py easier from scripts. > True, I'll add it! > > > 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 wish EXIF had names. As far as I can tell, there's no standardized > names for orientations, which leads to everybody using their own naming > scheme :-( > > > I considered 'mirror' more intuitive than 'rotate0Flip' for users. > > Are users of cam and cam.py that different from applications developers > ? If not, maybe libcamera::Orientation::Mirror would also be a more > intuitive name than rotate0Flip ? > True, but that would break the enumeration naming rule.. true, if rotation is 0 it could be omitted, but then how would you name Orientation::Rotate0 ? > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Thu, Oct 19, 2023 at 01:58:43PM +0200, Jacopo Mondi wrote: > On Thu, Oct 19, 2023 at 12:40:34AM +0300, Laurent Pinchart wrote: > > On Wed, Sep 06, 2023 at 09:10:10AM +0200, Jacopo Mondi via libcamera-devel wrote: > > > 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: > > > > > 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 ? > > > > Could we at least have the same case for the C++ and Python applications > > ? Having to write '-o mirror' for one and '-o Mirror' for the other > > isn't nice. > > Yes, I don't remeber why I went for capital letter for the py > implementation.. > > > And I also wonder if we should also support a rot0 value. It would allow > > users of the application to unconditionally pass the -o option, which > > may make usage of cam/cam.py easier from scripts. > > True, I'll add it! > > > > > 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 wish EXIF had names. As far as I can tell, there's no standardized > > names for orientations, which leads to everybody using their own naming > > scheme :-( > > > > > I considered 'mirror' more intuitive than 'rotate0Flip' for users. > > > > Are users of cam and cam.py that different from applications developers > > ? If not, maybe libcamera::Orientation::Mirror would also be a more > > intuitive name than rotate0Flip ? > > True, but that would break the enumeration naming rule.. true, if > rotation is 0 it could be omitted, but then how would you name > Orientation::Rotate0 ? You're right. I don't have a better name, so let's keep it as-is.
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(+)