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

Fix flash firing detection #18415

Merged

Conversation

victoryforce
Copy link
Collaborator

"Yes" at the beginning of the string does not mean that the flash fired, but that the flash function is present (for example, there is a built-in flash or an external flash is connected). There is a string from exiv2 "Yes, did not fire" for which checking only the first letter gave a false detection of the flash firing.

Similarly, not only "Y" as the first letter indicates the flash firing, the string obtained from exiv2 can also begin with the word "Fired".

Also removed "n/a" as the content of the text variable that indicates "flash firing". In fact, we always know whether the flash fired or not, "n/a" is meaningless.

Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@TurboGit TurboGit added this to the 5.2 milestone Feb 13, 2025
@TurboGit TurboGit added the scope: DAM managing files, collections, archiving, metadata, etc. label Feb 13, 2025
@TurboGit TurboGit merged commit 296eb87 into darktable-org:master Feb 13, 2025
6 checks passed
@zisoft
Copy link
Collaborator

zisoft commented Feb 14, 2025

The exiv2 strings are localized, I think this will not work in other languages than english.

Bildschirmfoto 2025-02-14 um 06 59 30

@TurboGit
Copy link
Member

TurboGit commented Feb 14, 2025

Strange, on my side I do not have it translated:

image

For Flash I have "Yes, compulsory", "No, compulsory", or "No flash"

@TurboGit
Copy link
Member

And I do not see those strings in fr.po... Maybe the exiv2 has no support for French?

@zisoft
Copy link
Collaborator

zisoft commented Feb 14, 2025

exiv2/po

When building the macOS installation package I explicitly copy the exiv2 translations to share/locale, where exiv2 expects them to be.

@TurboGit
Copy link
Member

So I don't see how to solve this. The exiv2 strings may be translated or not and darktable has no way to check for possible translation has it has no idea about the strings in exiv2.

@victoryforce victoryforce deleted the fix-flash-firing-detection branch February 14, 2025 21:23
@zisoft
Copy link
Collaborator

zisoft commented Feb 15, 2025

I think this should be reverted, see #18424

@victoryforce
Copy link
Collaborator Author

I think this should be reverted, see #18424

I'd say I should find a better solution instead of reverting. Just reverting will put us back in the state with other errors in flash detection.

The new issue is the result of the fact that I forgot when writing the detection function that the string from exiv2 can be empty. And that I tested on an insufficiently diverse set of images... :(

Simply adding this check makes no sense, as a completely different solution is needed. Both my PR and the code before it are incorrect in a situation where strings from exiv2 change depending on the language.

@parafin
Copy link
Member

parafin commented Feb 15, 2025

that exif tag is actually a number, so that raw value should be used instead of string, which is also an easier test: https://stackoverflow.com/questions/7076958/read-exif-and-determine-if-the-flash-has-fired

@victoryforce
Copy link
Collaborator Author

@parafin Yes, I am testing this very method right now. This was exactly the solution I wrote about.

@victoryforce
Copy link
Collaborator Author

The promised correct solution: #18437

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: pending scope: DAM managing files, collections, archiving, metadata, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants