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

Samsung EK-GN120: re-introduce support #544

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

ge0rg
Copy link
Contributor

@ge0rg ge0rg commented Oct 30, 2023

I've uploaded a sample to RPU (but can't see it now on the website?!), but also took the occasion to just fix the cameras.xml - the EK-GN120 is using the same sensor as the NX300, and now the entries are identical.

SRW from the Galaxy NX (EK-GN120) look good in darktable now as well.

<Camera make="SAMSUNG" model="EK-GN120" decoder_version="3" supported="no-samples">
<ID make="Samsung" model="EK-GN120">Samsung EK-GN120</ID>
<Camera make="SAMSUNG" model="EK-GN120" decoder_version="3">
<ID make="Samsung" model="EK-GN120">Samsung Galaxy NX</ID>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really sure this kind of normalization is common here.
I think let's not do 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.

Well, it's officially called the Galaxy NX, but this is not a hill for me to die on :)

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, i misread the change as-if the new maker was non-Samsung. Let's leave this in. Sorry.

<Camera make="SAMSUNG" model="EK-GN120" decoder_version="3" supported="no-samples">
<ID make="Samsung" model="EK-GN120">Samsung EK-GN120</ID>
<Camera make="SAMSUNG" model="EK-GN120" decoder_version="3">
<ID make="Samsung" model="EK-GN120">Samsung Galaxy NX</ID>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<ID make="Samsung" model="EK-GN120">Samsung Galaxy NX</ID>
<ID make="Samsung" model="EK-GN120">Samsung NX U</ID>

This actually comes from UniqueCameraModel after Adobe DNG Conversion.

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've spent quite some time studying Samsung NX and the models created by them, and I never encountered a "Samsung NX U". There is an April's Fool "NX Ultra" joke, but the "NX U" only seems to exist in Adobe's DNG and websites that use it for listing the cameras they support. The official name is Samsung Galaxy NX and there were two internal model numbers: EK-GN100 and EK-GN120.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this <ID> name remangling, and there's no docs.
My best guess is that indeed, the idea is to really be 1:1 with ADC.

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 understand the urge to be consistent, but quite obviously Adobe is in a dead-end here. I'd really appreciate showing the name that's actually printed on the device ("Galaxy NX"), or at least the internal model number ("EK-GN120", I'm pretty sure it's part of the fineprint on the body label, will check on Saturday). The ADC name will most probably only confuse the users.

Copy link
Contributor

@kmilos kmilos Oct 31, 2023

Choose a reason for hiding this comment

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

TBH, I haven't seen the actual value being used for anything meaningful in rawspeed nor darktable, whereas the make and model attributes are what seems to really count for the normalisation...

The dngmeta script populates the ID value with the UniqueCameraModel of the DNG (if not native for the camera, then one produced by ADC), but I guess it's not the end of the world if we override it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I'm mistaking the ID field, and it's not supposed to contain the human readable name of the camera?

Copy link
Contributor

@kmilos kmilos Oct 31, 2023

Choose a reason for hiding this comment

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

Maybe I'm mistaking the ID field, and it's not supposed to contain the human readable name of the camera?

Yes, but at least in rawspeed tools and darktable, it is constructed from make + model ID attributes, not the ID value. 🤷

In this case it only helps converting "SAMSUNG" -> "Samsung", but you'll find more useful example pairs in the file.

@ge0rg
Copy link
Contributor Author

ge0rg commented Oct 31, 2023

Okay, given the controversy about the <ID> field, feel free to just cherry-pick the first commit and close this as done. Alternatively, I can remove the second commit from the PR to get this resolved. Thanks! :)

@LebedevRI
Copy link
Member

@ge0rg thank you!

@LebedevRI LebedevRI merged commit dc38cdf into darktable-org:develop Nov 1, 2023
32 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Resolved
Development

Successfully merging this pull request may close these issues.

3 participants