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 more fujifilm tags #1949

Merged
merged 17 commits into from
Aug 3, 2022
Merged

add support for more fujifilm tags #1949

merged 17 commits into from
Aug 3, 2022

Conversation

VJSchneid
Copy link
Contributor

I made some tests with the Fujifilm X-T4 and added/improved some missing makernote tags.

The following Fujifilm tags where added:

  • ColorTemperature
  • WhiteBalanceFineTune
  • HighIsoNoiseReduction
  • Clarity
  • FocusArea
  • FocusPoint
  • LensModulationOptimizer
  • GrainEffectRoughness
  • GrainEffectSize
  • ColorChromeEffect
  • ColorChromeFXBlue
  • ImageNumber

Additionally the print values for the following Fujifilm tags where extended:

  • Sharpness
  • WhiteBalance
  • Color
  • FlashMode
  • Shadow/Highlight Tone
  • Film Mode

@postscript-dev postscript-dev added enhancement feature / functionality enhancements makerNote Anything related to one of the various supported MakerNote formats prettyPrinter Anything related to the output formatting of a value labels Oct 6, 2021
{ 4, N_("Hard mode 1") },
{ 5, N_("Hard mode 2") }
{ 0, N_("-4") },
{ 1, N_("Soft mode 1/-3") },
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if there is nicer way to present this now, e.g. keep quantity consistently on the left like "-3 (Soft mode 1)" since +/-1 and +/-4 are now added without a label; or add "Soft/Hard mode (0)" and "Soft/Hard mode 3" labels... How is it presented in the camera menu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. The X-T4 (and probably all other recent Fujifilm cameras) display the sharpness as number (-4,-3,-2,-1,0,+1,+2,+3,+4). I think older cameras display this setting as text and support fewer options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer something like "-3 (Soft mode 1)" because the labels might be a bit out of date instead of the numbers.

src/fujimn_int.cpp Outdated Show resolved Hide resolved
{ 771, N_("Monochrome + G Filter") },
{ 784, N_("Sepia") },
{ 768, N_("Monochrome") } // To silence compiler warning
{ 0, N_("Normal/0") },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above, is there a more consistent way to present the value+label combo?

src/fujimn_int.cpp Outdated Show resolved Hide resolved
src/fujimn_int.cpp Outdated Show resolved Hide resolved
src/fujimn_int.cpp Outdated Show resolved Hide resolved
@kmilos
Copy link
Collaborator

kmilos commented Oct 6, 2021

Thanks for your contribution!

@postscript-dev
Copy link
Collaborator

Thank you for taking time to contribute this.

Exiv2 collaborates with another open source project called ExifTool. Their website contains a more complete list of tags and values that have been discovered for the different camera types. ExifTool's FujiFilm tags can be found here and the source code (containing all the conditions, extra info and pretty printing) here. There is some additional information on the websites, that you may consider including (e.g., extra entries in the arrays).

{ 256, N_("F1/Studio Portrait") },
{ 272, N_("F1a/Studio Portrait Enhanced Saturation") },
{ 288, N_("F1b/Studio Portrait Smooth Skin Tone (Astia)") },
{ 288, N_("F1b/Studio Portrait Smooth Skin Tone (ASTIA)") },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking about changing this notation too because the labels "F0/Standard", "F1b/Studio Portrait..." are also coming from older fujifilm cameras like the FinePix S5 Pro. Newer cameras display a name like PROVIA, ASTIA, etc.

Maybe this can be made consistently with the Color and Sharpness Tag.

@VJSchneid
Copy link
Contributor Author

Thank you for taking time to contribute this.

Exiv2 collaborates with another open source project called ExifTool. Their website contains a more complete list of tags and values that have been discovered for the different camera types. ExifTool's FujiFilm tags can be found here and the source code (containing all the conditions, extra info and pretty printing) here. There is some additional information on the websites, that you may consider including (e.g., extra entries in the arrays).

Thanks for the information. I can extend the tags with the code from exiftool but I am currently unable to verify all these settings with my hardware...

Comment on lines 57 to 58
{ 1, N_("-3 (Soft mode 1)") },
{ 2, N_("-2 (Soft mode 2)") },
Copy link
Collaborator

@kmilos kmilos Oct 6, 2021

Choose a reason for hiding this comment

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

This is a bit controversial (not symmetrical): soft 1 = -3 but hard 1 = +2; I would expect soft/hard 1 = -/+2 and soft/hard 2 = -/+3... any way to confirm these?

Or ditch the mode 1/2 altogether for a scheme similar to exiftool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure where these labels "soft/hard modes" come from. Maybe I can find the origin of that with git. I will investigate that...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These modes are from here and where added 18 years ago to exiv2^^ Back then they where just called High and Low. I can not find any information on why the "mode 1/2" suffix was added...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we can ditch "mode 1" and "mode 2" and stick with exiftool labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the labels and extended some more Tags from exiftool with my last commit

{ 128, N_("+1") },
{ 192, N_("+3") },
{ 224, N_("+4") },
{ 256, N_("High/+2") },
{ 256, N_("+2 (High)") },
{ 384, N_("-1") },
{ 512, N_("Low") },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this "-2 (Low)" now to anti-reflect "+2 (High)"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I now see there is a -2 below where (Low) should be added (cf. exiftool)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my understanding "Low" and "High" are color levels from older/different cameras. The X-T4 for example does not show "High" or "Low" labels, but numbers. Older cameras like the FinePix S5-Pro have the High and Low level as it is stated in the manual on page 109.
So "-2" and "Low" do not share a common tag value contrary to "+2" and "High" as it seems to be different implemented in these cameras. Therefore "High" is not an alternative representation of "+2" but a legacy value. Pretty printing dependent on the camera model might be better but I am currently unaware whether this is easy to implement. Do you know how this is handled with other manufacturers where old and new tags overlap?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, maybe just follow exiftool here as well?

Copy link
Contributor Author

@VJSchneid VJSchneid Oct 8, 2021

Choose a reason for hiding this comment

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

I applied the labels from exiftool and additionally changed the label of value 512 to "-2 (low)" to unify it with the corresponding high value (256). In exiftool this was just "Low".

{ 304, N_("F1c/Studio Portrait Increased Sharpness") },
{ 512, N_("F2/Fujichrome (Velvia)") },
{ 512, N_("Velvia (F2/Fujichrome)") },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just out of curiosity, you know why there are 2 Velvia entries (F2 and F4)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, but I think they come from here.

Copy link
Contributor Author

@VJSchneid VJSchneid Oct 7, 2021

Choose a reason for hiding this comment

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

Value 512 translates on modern Fujifilm cameras to Velvia. The F1/2/... notation comes again from older cameras like the FinePix S5 Pro.

src/fujimn_int.cpp Outdated Show resolved Hide resolved
src/fujimn_int.cpp Outdated Show resolved Hide resolved
src/fujimn_int.cpp Outdated Show resolved Hide resolved
@piponazo
Copy link
Collaborator

Hi everybody. Thanks @VJSchneid for the contribution. I am just taking a look to some open PRs and I was wondering if we could give the final push to this PR to get it merged. I am not familiar with the Fujifilm part of the code, but it looks like you added support for many new tags. It would be great to incorporate these changes into Exiv2.

Could you please rebase your branch in top of main so that the CI pipeline runs again? It looks like some of the CI jobs are in red because some hiccup in Github actions. Maybe we get those CI checks into "green" state just by rebasing your branch.

@piponazo
Copy link
Collaborator

Hi @VJSchneid again, and sorry for taking so long to come back to you. After you rebased the branch, most of the CI jobs failed. There has been lot of activity in the last month in main and there seems to be some conflicts now. Do you need any help with this? I just would not like to see this contribution getting stuck for not giving it enough attention (from our side).

@VJSchneid
Copy link
Contributor Author

Hi @piponazo, sorry that this response took so long. I rebased my branch again and hopefully everything is in sync now.
Additionally some missing testfiles were also updated. Unfortunately I get some errors when runnung ctest with Exif.CanonCs.LensType but this should be independent of these changes. The fujifilm stuff seems to pass :)

@VJSchneid VJSchneid requested a review from kmilos May 13, 2022 18:39
@VJSchneid
Copy link
Contributor Author

Hi @piponazo, I have merged the latest changes with this PR again. Is there anything more I can do to speed things up?

//! DriveSetting, tag 0x1103
std::ostream& printFujiDriveSetting(std::ostream& os, const Value& value, const ExifData*) {
auto byte1 = value.toInt64() & 0xff;
auto byte2 = (value.toInt64() >> 8) & 0xff;

Check warning

Code scanning / CodeQL

Signed shift

This signed shift could cause undefined behavior if the value is negative. Type of lhs: int64_t
std::ostream& printFujiDriveSetting(std::ostream& os, const Value& value, const ExifData*) {
auto byte1 = value.toInt64() & 0xff;
auto byte2 = (value.toInt64() >> 8) & 0xff;
auto byte3 = (value.toInt64() >> 16) & 0xff;

Check warning

Code scanning / CodeQL

Signed shift

This signed shift could cause undefined behavior if the value is negative. Type of lhs: int64_t
auto byte1 = value.toInt64() & 0xff;
auto byte2 = (value.toInt64() >> 8) & 0xff;
auto byte3 = (value.toInt64() >> 16) & 0xff;
auto fps = value.toInt64() >> 24;

Check warning

Code scanning / CodeQL

Signed shift

This signed shift could cause undefined behavior if the value is negative. Type of lhs: int64_t
@codecov
Copy link

codecov bot commented Aug 1, 2022

Codecov Report

Merging #1949 (d62220a) into main (1d0530f) will decrease coverage by 0.09%.
The diff coverage is 26.92%.

@@            Coverage Diff             @@
##             main    #1949      +/-   ##
==========================================
- Coverage   63.53%   63.43%   -0.10%     
==========================================
  Files         118      118              
  Lines       19595    19647      +52     
  Branches     9576     9608      +32     
==========================================
+ Hits        12450    12464      +14     
- Misses       5079     5112      +33     
- Partials     2066     2071       +5     
Impacted Files Coverage Δ
src/fujimn_int.cpp 29.62% <26.92%> (-70.38%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@piponazo
Copy link
Collaborator

piponazo commented Aug 3, 2022

Hi @piponazo, I have merged the latest changes with this PR again. Is there anything more I can do to speed things up?

Sorry for my late reply, I am not participating much in the project lately.

Everything looks green, let's merge it!

@piponazo piponazo merged commit 0256775 into Exiv2:main Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature / functionality enhancements makerNote Anything related to one of the various supported MakerNote formats prettyPrinter Anything related to the output formatting of a value
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants