Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for passing parameters to imwrite() #225

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

mstenta
Copy link
Contributor

@mstenta mstenta commented Aug 7, 2024

This adds a new CLI argument for passing parameters into OpenCV's imwrite() function.

This allows, among other things, to customize compression of the output image file. For example, currently a TIFF will always be output with LZW compression as a default. This allows exporting uncompressed TIFFs. JPEG quality can also be configured.

For the full list of imwrite() parameters, see: https://docs.opencv.org/4.10.0/d8/d6a/group__imgcodecs__flags.html

See related discussion for more detail: #224

Starting this as a "draft" pull request so that we can see if tests pass, and determine if anything else is necessary.

@mstenta
Copy link
Contributor Author

mstenta commented Aug 7, 2024

Force-pushed my commit with the auto-fixes merged.

@mstenta mstenta marked this pull request as ready for review August 7, 2024 17:37
@@ -263,6 +263,13 @@ def create_parser():
choices=Timelapser.TIMELAPSE_CHOICES,
type=str,
)
parser.add_argument(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to call it output_params and put it after --output to have a closer connection here.

I would like to extend the help a bit. Maybe just describe your example, how cv.IMWRITE_TIFF_COMPRESSION, cv.IMWRITE_TIFF_COMPRESSION_NONE leads to 259 1 (the integers to be used in the CLI)

@@ -295,6 +302,9 @@ def main():
preview = args_dict.pop("preview")
output = args_dict.pop("output")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name this output_params and put it directly below output, we don't need the comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use output_params = args_dict.pop("output_params", [])
It removes the param from the dict since stitcher dont accept it. If its not there we use an empty list

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I dont know If output_params is not in the dict (then it would work) or None (then it would not work)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just set an empty list as Default as its done with the Feature masks:

parser.add_argument(
"--feature_masks",
nargs="*",
default=[],
help="Masks for selecting where features should be detected.",
type=str,
)

@@ -44,6 +44,7 @@ class Stitcher:
"blend_strength": Blender.DEFAULT_BLEND_STRENGTH,
"timelapse": Timelapser.DEFAULT_TIMELAPSE,
"timelapse_prefix": Timelapser.DEFAULT_TIMELAPSE_PREFIX,
"imwrite_params": None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this since stitcher never calls imwrite

@mstenta
Copy link
Contributor Author

mstenta commented Aug 8, 2024

Thanks for the feedback @lukasalexanderweber. I've made all the requested changes and force-pushed my commit. Hopefully the help text makes sense.

Copy link
Member

@lukasalexanderweber lukasalexanderweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks for the updates and the detailed help string. Its a bit long but I don't know where else to put it xD

@@ -294,6 +309,7 @@ def main():
verbose_dir = args_dict.pop("verbose_dir")
preview = args_dict.pop("preview")
output = args_dict.pop("output")
output_params = args_dict.pop("output_params", [])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove the empty list here, since its already the default in the argument parser

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! (force pushed)

@mstenta
Copy link
Contributor Author

mstenta commented Aug 8, 2024

lgtm, thanks for the updates and the detailed help string. Its a bit long but I don't know where else to put it xD

Great! Yea I had the same thought about the length of the help string. But it has the potential to be confusing, so it's probably worth a little more verbosity in this particular case. :-)

@lukasalexanderweber lukasalexanderweber merged commit 4ab9bf7 into OpenStitching:main Aug 8, 2024
2 checks passed
@mstenta mstenta deleted the imwrite_params branch August 8, 2024 14:06
@lukasalexanderweber
Copy link
Member

Thank you for your contribution and also very much for your donation over opencollective! :) it was the very first one and put a big smile on my face :)

@mstenta
Copy link
Contributor Author

mstenta commented Aug 8, 2024

Thank you for making and maintaining this project! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants