Support reading from stdin, writing to stdout in HEIC2PNG #5
Conversation
…xperimentation at the REPL.
| eprint('Please report this issue with the full traceback.') | ||
| eprint('-> https://github.com/NatLee/HEIC2PNG/issues') | ||
|
|
||
| def eprint(*args, file=sys.stderr, **kwds): |
There was a problem hiding this comment.
We could consider moving this function to execute before the function of cli.
There was a problem hiding this comment.
Not sure what you're suggesting, move the definition of eprint elsewhere, or the various checks for problems.
There was a problem hiding this comment.
This is not a problem; however, I recommend positioning the eprint function before the CLI function.
|
|
||
| parser = argparse.ArgumentParser(description="Convert HEIC images to PNG.") | ||
| parser.add_argument("-i", "--input_path", required=True, help="Path to the input HEIC image.") |
There was a problem hiding this comment.
Could you clarify the rationale behind removing the required flag?
There was a problem hiding this comment.
If the input comes from sys.stdin, there's not going to be an explicit input file, so requiring one doesn't make a ton of sense.
I'm not going to hold it up as best practice, but I have another package on GitHub, csvprogs which goes the other way. I had always supported using stdin and stdout for IO, only supporting filled in a hit-or-miss fashion. It has grown organically though, so different tools were organized differently. In a fit of virtual housecleaning, I tackled this inconsistency. I was able to come up with a relatively clean way to support both styles of consuming input and generating output (see the openio function and its usage).
There was a problem hiding this comment.
Understood. I believe it is necessary to add a check within the CLI function to verify whether input_path is None. Without this validation, the program might fail to locate the file and become unresponsive.
| open(outputfile, "rb") as outp): | ||
| # I don't really know how to compare images. If we assume that PIL | ||
| # does its thing properly, and PNG files will always be larger than | ||
| # their corresponding HEIC files, hopefully that's good enough. |
There was a problem hiding this comment.
I think this should be sufficient for comparing these two different images.
| # TODO: Not sure this is correct, there's probably a better mapping | ||
| # between image file extensions and file formats. For example, see the | ||
| # output of `python -m PIL`. | ||
| self.image.save(output_path, format=extension.replace(".", "").upper()) |
There was a problem hiding this comment.
We could refer to this link to retrieve a list of supported formats with PIL: https://stackoverflow.com/a/71114152
There was a problem hiding this comment.
I tested it in Python 3.12, but I'm unsure if this function works across all versions of PIL.
>>> from PIL import Image
>>> exts = Image.registered_extensions()
>>> supported_extensions = {ex for ex, f in exts.items() if f in Image.OPEN}
>>> supported_extensions
{'.wmf', '.icns', '.gif', '.grib', '.pnm', '.ftu', '.jpe', '.rgb', '.dib', '.tif', '.jfif', '.emf', '.jpc', '.pxr', '.ppm', '.dds', '.fit', '.icb', '.cur', '.pcx', '.bmp', '.h5', '.pfm', '.qoi', '.dcx', '.apng', '.iim', '.pbm', '.png', '.jpx', '.bw', '.psd', '.pcd', '.sgi', '.vda', '.im', '.mpeg', '.bufr', '.mpg', '.rgba', '.xpm', '.fli', '.fits', '.ps', '.flc', '.gbr', '.msp', '.j2c', '.jpg', '.tga', '.ico', '.xbm', '.pgm', '.j2k', '.jpf', '.jpeg', '.vst', '.hdf', '.ras', '.tiff', '.ftc', '.jp2', '.eps', '.webp', '.blp'}There was a problem hiding this comment.
Would be nice if there was a mapping from file extensions to formats. I think ultimately, that's what really matters.
There was a problem hiding this comment.
I believe we can select a few from within and create a map like this:
class HEIC2PNG:
FORMAT_MAPPING: Dict[str, str] = {
'.png': 'PNG',
'.jpg': 'JPEG',
'.jpeg': 'JPEG',
'.webp': 'WEBP',
'.bmp': 'BMP',
'.gif': 'GIF',
'.tiff': 'TIFF',
}
def __init__(...):
...We can make adjustments at this point.
self.image.save(output_path, format=extension.replace(".", "").upper())This line can be adjusted using the following approach: the variable image_format is assigned based on the file extension mapping (self.FORMAT_MAPPING[ext]), and the image is subsequently saved to the specified output_path in the determined format.
The snippet appears as follows:
...
ext = extension.lower()
if ext not in self.FORMAT_MAPPING:
supported_formats = ', '.join(self.FORMAT_MAPPING.keys())
raise ValueError(f"Unsupported format: {extension}. Supported formats are: {supported_formats}")
image_format = self.FORMAT_MAPPING[ext]
self.image.save(output_path, format=image_format)
...There was a problem hiding this comment.
This is just a method I came up with on short notice. It may not be the best approach, but feel free to use it as a reference.
|
|
||
| # TODO: Skip this step if the output format isn't PNG or pngquant can't | ||
| # be found? |
There was a problem hiding this comment.
Does the other format specify parameters like quality or compression ratio? If not, we can skip this check.
Here's a first cut at my proposed changes. I don't do Windows (Mac & Linux–so far only tested on Mac), so you'll have to take care of seeing if things still work there. I will check to make sure it works on Linux.
I wouldn't assume I have this all correct. We should have a bit of back-and-forth in the review/comment section of the PR to make sure I haven't unwittingly broken something. At any rate, the current tests do pass.