Replies: 4 comments 17 replies
-
|
If I understand correctly, keep a low-level ExifTool class which passes raw parameters to the process, and package up the interesting but "lots of boiler plate arguments" features into a higher-level API that makes interacting a bit friendlier. I'm on board with this. 👍 |
Beta Was this translation helpful? Give feedback.
-
|
It's not unfortunately, but an open source release is on the road map. In the mean time, I think I can extract the relevant parts into an indicative example. |
Beta Was this translation helpful? Give feedback.
-
|
Per my last post, here's my relevant code. Background is that I have two programs. One is control software for a multi-camera rig which simultaneously captures from some number of cameras. These images get written, and then I use ExifTool to add common information (rig make-model, software version, user-defined tags, etc.) and image specific metadata (camera make-model, focal length, etc.). Because a capture session can run for more than one instantaneous capture, I keep an ExifTool object around that I have to manually start and cleanup. That code looks like this: ### During construction of the capture manager ###
# setup common exif tags
self._default_exif_flags = ['-overwrite_original',
f'-Software={self._metadata["software"]}',
f'-Format={self.image_format}']
# This is just a list of key=value pairs, where key is predefined to be an ExifTool flag. Value is user-defined.
for tag in self.info.exif_tags():
if len(tag[1]) > 0:
self._default_exif_flags.append(f'-{tag[0]}={tag[1]}')
### At capture time, for each image ###
# Add image specific tags
exif_flags = self._default_exif_flags.copy()
capture_time = dt.now(datetime.timezone.utc).strftime("%Y:%m:%d %H:%M:%SZ")
exif_flags.append(f'-xmp:dateTimeOriginal={capture_time}')
if cam.make:
exif_flags.append(f'-Make={cam.make}')
if cam.model:
exif_flags.append(f'-Model={cam.model}')
if cam.focal_length > 0.0:
exif_flags.append(f'-FocalLength={cam.focal_length}')
if cam.f_num > 0.0:
exif_flags.append(f'-FNumber={cam.f_num}')
# File path is always last
exif_flags.append(str(file_path))
# Encode the flags to bytes
exif_flags = [flag.encode() for flag in exif_flags]
# Start exiftool if it's not running
if not self._exif_tool.running:
self._exif_tool.start()
# Run the command
self._exif_tool.execute(*exif_flags)My second program consumes the files generated by the capture program. It's much more straightforward: glob all of the files in the capture directory and batch read their metadata into a dict: # Reading metadata
images = list(dir.glob(f'{prefix}*.{ext}'))
images.sort()
files = [str(i) for i in images]
if len(files) == 0:
print('Error: Provided scan metadata specifies file pattern, but no files match.')
raise RuntimeError()
with exiftool.ExifTool() as et:
img_metadata = et.get_metadata_batch(files)Hope that helps. |
Beta Was this translation helpful? Give feedback.
-
|
It occurs to me that you don't have to roll back to that commit. You could just remove that functionality from the raw class as you implement it in the helper class/classes. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for the relevant code. I'll look at it a bit later. The problem with some of those usage like I'm not planning to do a Still debating whether i want to break things that I feel could be improved... like There's other little things like that. |
Beta Was this translation helpful? Give feedback.
-
Yeah, that's a tough problem. I have lots of thoughts and theories, but it's all academic at this point, so I'll resist 😛. I'm happy to test whatever you come up with!
Ahh, I see. I misunderstood. Good deal!
My natural inclination would be to follow the language of the I'm pretty excited about how things are moving here! |
Beta Was this translation helpful? Give feedback.
-
|
ok @csparker247 , I took your suggestion into account and renamed start() to run(). I like that idea, match what As I've been stepping through different test cases, I've found that when no files are processed, the JSON decode blows up... since it can't decode an empty string. I'm trying to decide... what to return from My idea is...
What do you think? As I went about it, I realized that the original code redirected I'm going to see if I can come up with test cases to look at stderr output... and also test my code on Linux |
Beta Was this translation helpful? Give feedback.
-
|
If the output is completely empty (no files processed) or some hard error occur, I'd I think it's trickier when at least one file does get processed. The returned output is valid json, but it's missing the entries for the failed files and there's no placeholders (i.e. empty dicts) for the missed files. For that case, the output becomes harder to use, since there's no way of knowing which files didn't get processed without iterating the list. In this case, my inclination would be to raise an exception by default, but maybe provide an argument that lets the user override that behavior and still get the output. This would be similar to the Not sure how familiar you are with Github actions, but testing across multiple Python versions on Ubuntu is fairly easy. |
Beta Was this translation helpful? Give feedback.
-
|
Hmm, I'm not familiar with Actions at all, I saw that advertised a few weeks ago, will look into it next week. Thanks for the link! Yeah, the Alright, anyways... I'll think about it. Thanks for your input! |
Beta Was this translation helpful? Give feedback.
-
|
Hmm, on my build, ExifTool prints a message to stderr indicating the file as it encounters a non-existent file, then summarizes the statistics at the end as you describe. On the command line, the messages are interspersed with the json output, so I missed it at first. |
Beta Was this translation helpful? Give feedback.
-
|
alright, I figured out how to do read stderr now without accidentally blocking on windows. I found the exiftool option to do a Btw, I'm renaming the |
Beta Was this translation helpful? Give feedback.
-
|
It is often good to streamline to make it more maintainable, extensible, and usable. As long as you communicate it clearly (including semantic versioning, documentation) and provide a path for people to easily adapt to the new interface, it should be good. |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, I'm going to have to work hard on documentation when this is all done. I haven't done a CHANGELOG yet as I do commits to the v0.5 branch because, well, the interfaces aren't finalized yet. (I'm trying to write tests that get full code coverage though to cover all the new functionality in tests so I don't inadvertently break past functionality) Updating all the doc strings will be a pain :/ Current status
More thinkingRight now I'm thinking about the |
Beta Was this translation helpful? Give feedback.
-
|
That said (making it easy for people to port over the new interface), from my limited experience, it is more important that the interface is useful and make sense to you. It is nice to have other people using your package, but since this is a small open source project, the most important user is the maintainer (you). As long as you find it useful and enjoy it, that keeps it alive. For me, I care about the ability of batch processing a ton of files at once. The primary reason I use the original library is that it uses the native batch mode of exiftool in Perl (kind of opening a server and keep sending it commands.) I’m not sure it is related to the things you’re modifying, but I can keep my own fork for that purpose. P.S. By my own test, that batch mode is still much slower than using the native batch mode of exiftool on the command line. |
Beta Was this translation helpful? Give feedback.
-
oh, no, I'm not removing the native batch mode. That's the best part! The I'm good at breaking things :). Just filed an issue with CPython earlier today because I can't figure out why ExifTool class hangs the interpreter on
how are you using the batch mode on the command line. Can you write a simple benchmark for me to test against? I actually noticed how smarnach optimized memory and resource management lately as I've been trying to tweak the code. I might be able to look into optimizing it if it's much slower. I wonder why it would be... Example of the _batch stuff I'm talking aboutEdit: If you want to see what I mean by removing the This combines the functionality of the original def get_tags_batch(self, tags, filenames) and the original def get_tags(self, tags, filename) into one very usable (and error-checked to death) function with a consistent return value |
Beta Was this translation helpful? Give feedback.
-
|
I can copy the exact command I used in the past if you need, but the command line batch mode I mentioned means calling exiftool on a glob pattern and with a certain arg that I don’t remember right now, ask it to write the exif data as a JSON sidecar. Since I read a lot of exif (100,000 or more) I found that caching them as a JSON sidecar first is much quicker. Eg you can call exiftool directly to write those files first, and later in your script reading from those JSON, is much faster than calling it through pyexiftool batch mode directly, even when doing exactly the same thing. (That’s also why I submitted a patch that fix a bug preventing pyexiftool to writes to JSON sidecar.) however, you may not need to worry about this, as it might be an inherent speed limitation in running exiftool in “server mode”. Often, calling across programming interface adds latency that is especially noticeable in tight loop (Ie the latency is amplified by the no of times you call it, and is a significant faction of the total run time.) |
Beta Was this translation helpful? Give feedback.
-
|
Alright, I have a draft PR created #13 to start the discussion with direct looks at the code. Could you take some time to see if PyExifTool is moving in a direction you'd be happy with? 😊 The base The |
Beta Was this translation helpful? Give feedback.
-
|
Great! I've got a big deadline this week, but will put it on my list for review next week. |
Beta Was this translation helpful? Give feedback.
-
I'm planning to roll back some of the PR that I merged really early on from this project. (specifically the commits around 9c15d42)
I'm going to re-design a little bit, and put all "Helper" functions which wrap around raw Exiftool in another class. This should help clean up the code and make it easier to extend later
The original Exiftool class was more of a "raw" class where it had all the Exiftool low level functionality. The addition of methods that made it easier to use into the class will make it harder to maintain over time.
... etc
I believe these should be in a separate class, which either uses Exiftool directly, or extends it. It should make it easier to fork and add functionality, without making the Exiftool class unmanageably big with a bunch of helper functions
In fact, down the line, should be able to make even more streamlined functionality available... say, if you wanted to "rotate a picture by 90 degrees" right now, you have to raw pass into Exiftool class all the parameters to make it happen. Extending a helper class could make it doable in a few lines
What do you all think?
Beta Was this translation helpful? Give feedback.
All reactions