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 user defined exif_data when capturing files #786

Closed
wants to merge 2 commits into from

Conversation

Amudtogal
Copy link
Contributor

When capturing to EXIF-capable files, the user can provide the
dictionary exif_data which is merged with the auto-generated EXIF-data
before writing into the image file.

Changes affect the capture_file_ method and all derivatives.

The commit is based on this old discussion #71 and adds the requested
functionality.

Documentation in docstrings has been provided where applicable.

As I have no access to a working picamera environment right now, I can not test
the functionality.

The original pull request was submitted for the main branch, and is superseded
by this one (/pull/779).

Fixed the linter errors accordingly.

@Amudtogal
Copy link
Contributor Author

Hm, I have had time to check some of the functionality.

Unfortunately, the | or update merge of Python is only first level.
For merging nested dictionaries we would have to write a function...

Let me know if this is wanted, or whether we just overwrite the existing piexif data.

@davidplowman
Copy link
Collaborator

Hi, thanks for the PR. It seems like a reasonable feature to add. I'm not quite sure why the automatic tests don't seem to have run, but I can look into that.

Can you say why you changed the default value of the exif_data in the second commit? I'm wondering if it would be better to squash the two together, we normally prefer to avoid "fix the previous commit" commits! Thanks.

@Amudtogal
Copy link
Contributor Author

I have squashed the commits.

I changed the default values, because the linter check highlighted it. Apparently only immutable types are recommended for keyword arguments...

However, this still doesn't resolve the issue of merging nested dictionaries.
Is this still wanted, or are we happy with just a first-level merge of the user-defined exif data?

@davidplowman
Copy link
Collaborator

Thanks for the updates. Unluckily you've caught us at a slight awkward time as everything on the "next" branch is being updated for Pi 5 and Bookworm - hence the conflicts. You'll probably find you can't even run this version without building the "next" branch of raspberrypi/libcamera.git as well. But bear with us, this should all get sorted out in the next few days! Looks like I can approve the automatic tests, so I'll do that.

When capturing to EXIF-capable files, the user can provide the
dictionary `exif_data` which is merged with the auto-generated EXIF-data
before writing into the image file.

Changes affect the `capture_file_` method and all derivatives.

Signed-off-by: Simon Lenz <[email protected]>
@Amudtogal
Copy link
Contributor Author

There has been an issue with merging into the updated branch, which should be resolved now.

@Amudtogal
Copy link
Contributor Author

Is there an interest to merge this still?

@davidplowman
Copy link
Collaborator

Yes, sorry. There's been a bit of a run of hectic stuff going on, like Pi 5, but I'll try to get round to this in the next couple of days!

@davidplowman
Copy link
Collaborator

Hi again, sorry for all the delay with this. I've taken your PR, removed the merge commit and added a test for this functionality (which will stop it getting broken in future by accident!). You can find it here #890

Let me know if that still looks OK to you in which case I'll merge that one and close the original here. If there's anything further you'd like to add in the way of tests, please let me know. Thanks for all your patience!

@davidplowman
Copy link
Collaborator

I've merged the PR derived from this one, so closing it now.

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