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 noise profiles for Panasonic Lumix DC-S9. #17460

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sarunasb
Copy link
Contributor

Here are sensor noise profiles for DC-S9 (issue #17458).

@TurboGit TurboGit added this to the 5.0 milestone Sep 10, 2024
@TurboGit TurboGit added scope: noise profile adding noise profiles for new cameras depends: rawspeed cannot merge before rawspeed updated labels Sep 10, 2024
@kmilos kmilos added depends: libraw for libraw related issues and removed depends: rawspeed cannot merge before rawspeed updated labels Oct 23, 2024
@TurboGit
Copy link
Member

@kmilos : Same question for libraw here? What to expect for 5.0? There is many changes waiting a bump of libraw, is there any chance or should we reschedule this for 5.2? TIA.

@sarunasb
Copy link
Contributor Author

Not sure I understand the question, but S9 RW2s “just worked” with Libraw pulled into darktable tree from Libraw git, whatever it was on September 10th. I don't have the camera anymore, but do have RW2s saved, if any testing is needed.

@victoryforce
Copy link
Collaborator

@kmilos : Same question for libraw here? What to expect for 5.0? There is many changes waiting a bump of libraw, is there any chance or should we reschedule this for 5.2? TIA.

The fact is that even if darktable is built with Libraw, which does not support certain models, it makes sense to add such support as noise profiles and white balance presets for such models BEFORE support in Libraw.

Reasons:

  • The darktable release may be later (re)built in distributions with a later released version of Libraw.
  • The user can work in darktable with an unsupported camera by converting images to DNG using Adobe DNG Converter.

The second reason applies to rawspeed as well.

@kmilos
Copy link
Contributor

kmilos commented Nov 21, 2024

it makes sense to add such support as noise profiles and white balance presets for such models BEFORE support in Libraw

I'm strongly against this. We should first declare/ensure base support (even in the workaround/fallback case), then everything else. People willing to experiment should have no problem editing a text file to add the WB preset or a noise profile from a PR.

Re S9: last time I tried the workaround, it didn't work on anything newer than G9M2 that was declared as supported in the last LibRaw snapshot, I got a black image... Might have been before Sept 10...

I don't have any hopes that a new snapshot or a release will happen soon, but just in case, what would be the last date to merge a submodule in? @TurboGit

@sarunasb
Copy link
Contributor Author

I'm strongly against this. We should first declare/ensure base support (even in the workaround/fallback case), then everything else

I don't see how this would be wrong. Though “dormant,” but valid data, which becomes useful the moment when RAW support (libraw, rawspeed or whatever magic decoder of the future) comes in.

@sarunasb
Copy link
Contributor Author

Re S9: last time I tried the workaround, it didn't work on anything newer than G9M2 that was declared as supported in the last LibRaw snapshot, I got a black image... Might have been before Sept 10...

This was processed in darktable built with Libraw submodule pulled from Libraw git master sometime between August 28 (picture taken) and September 3 (uploaded to Flickr).

@kmilos
Copy link
Contributor

kmilos commented Nov 22, 2024

I don't see how this would be wrong.

Never underestimate the capacity of people to assume something unexpected. In this case, someone would assume the camera is listed as supported, which is not something we'd want to communicate in any way.

While you might get an image out, one can't be sure what input color matrix is used until it is explicitly added by LibRaw, so - not supported.

@TurboGit
Copy link
Member

I don't have any hopes that a new snapshot or a release will happen soon, but just in case, what would be the last date to merge a submodule in? @TurboGit

I'd say early December at the latest on Dec 6th.

@kmilos
Copy link
Contributor

kmilos commented Nov 22, 2024

Ta, let's see what happens till then. I'll also double-check what matrix LibRaw currently returns for the S9...

@victoryforce
Copy link
Collaborator

I don't have any hopes that a new snapshot or a release will happen soon, but just in case, what would be the last date to merge a submodule in?

@kmilos BTW, the rawspeed submodule must be updated before the darktable release to include the clang 19 compilation fix. Without it, we cannot release darktable.

@kmilos
Copy link
Contributor

kmilos commented Nov 22, 2024

the rawspeed submodule must be updated ... Without it, we cannot release darktable.

Sure, but that's not up to me...

@kmilos
Copy link
Contributor

kmilos commented Nov 22, 2024

FWIW, raw-identify -v from LibRaw git master returns

XYZ->CamRGB matrix:
0.0000  0.0000  0.0000
0.0000  0.0000  0.0000
0.0000  0.0000  0.0000

so I guess you're getting some fallback input matrix currently (one would need to build in debug and figure out what exactly).

For comparison, the supported G9M2 gives:

XYZ->CamRGB matrix:
0.8325  -0.3456 -0.0623
-0.4330 1.2089  0.2528
-0.0860 0.2646  0.5984

Under these circumstances, I wouldn't feel comfortable to enable/encourage people to use the workaround and advertise any level of support yet.

@victoryforce
Copy link
Collaborator

Under these circumstances, I wouldn't feel comfortable to enable/encourage people to use the workaround and advertise any level of support yet.

@kmilos This PR is not about workaround, advertising support or encouraging people. This PR is about adding noise profile to improve the user experience when the time comes (or for those using ADC). So I see no reason to undermine efforts to improve user experience.

@kmilos
Copy link
Contributor

kmilos commented Nov 22, 2024

@victoryforce IMHO a big part of that user experience is the user being 100% clear on what is supported and what they can expect. This camera model is unfortunately not supported in any way currently.

P.S. By workaround I meant the libraw_extensions route. Users shouldn't be using it (or be lead to believe it is an option) for a model that LibRaw doesn't support. Adding the noise profile (which would then appear in the automatically generated release notes and main website camera support table) would lead them to believe there is some partial support.

If LibRaw doesn't enable this by Dec 6, I have nothing further to add on the topic.

@sarunasb
Copy link
Contributor Author

Interesting. In Libraw tree I used to build S9-supporting version of darktable 4.8.1:

ereliai@storas:~/darktable.org/src/external/LibRaw$ grep -r -I -i -A1 dc-s9 *
src/tables/colordata.cpp:    {LIBRAW_CAMERAMAKER_Panasonic, "DC-S9", 0, 0,
src/tables/colordata.cpp-      { 9983,-3890,-841,-4180,12164,2263,-249,1139,5766 } },
--
src/tables/cameralist.cpp:      "Panasonic DC-S9",

In current Libraw git master:

ereliai@storas:~/LibRaw$ grep -r -I -i -A1 dc-s9 *

i.e. nothing...

@kmilos
Copy link
Contributor

kmilos commented Nov 22, 2024

Must've been a locally patched version then, which explains why I was getting the black image...

@sarunasb
Copy link
Contributor Author

Might be beside the point here now, but Libraw does/did support S9 since at least late August. At that time I corresponded with their team and helped them test and verify that support with my RW2s and software utils provided by them for that purpose.

@kmilos
Copy link
Contributor

kmilos commented Nov 22, 2024

Be that as it may, they haven't publicly released it yet.

@victoryforce
Copy link
Collaborator

@victoryforce IMHO a big part of that user experience is the user being 100% clear on what is supported and what they can expect. This camera model is unfortunately not supported in any way currently.

Yes, I completely agree with you, we need to be very careful and clear about what we support. But, again, this is not what this PR is about.

P.S. By workaround I meant the libraw_extensions route. Users shouldn't be using it (or be lead to believe it is an option) for a model that LibRaw doesn't support. Adding the noise profile (which would then appear in the automatically generated release notes and main website camera support table) would lead them to believe there is some partial support.

"automatically generated release notes and main website camera support table" is something that is completely under our control. There is no big problem in making sure that incorrect information does not appear there. It is just something to keep in mind. Not improving the user experience just because you need to fix a couple of scripts / be careful when creating release notes is really strange.

If LibRaw doesn't enable this by Dec 6, I have nothing further to add on the topic.

What we are writing here is not about adding to the topic. It is an exchange of arguments to make the best decision. And your argument simply does not hold water.

The only way I can explain such differences in positions is that you are talking about something completely different, you are opposing @sarunasb's desire to get not yet released support for this camera and reasonably pointing out the risks associated with it. This is totally understandable and I share your caution. I explain that in general, adding WB presets and noise profiles for cameras not yet supported in rawspeed and libraw is desirable (once again, I repeat the reason: ADC and (re)build in distributions with the new libraw). I do not understand how you can argue with this.

@kmilos
Copy link
Contributor

kmilos commented Nov 23, 2024

I do not understand how you can argue with this.

It's simple, really. Either dt provides base decoding support for a device (via RawSpeed, LibRaw, or the extension workaround), or it doesn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depends: libraw for libraw related issues scope: noise profile adding noise profiles for new cameras
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants