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

styles: enable built-in styles, add camera-JPEG styles #17073

Merged
merged 38 commits into from
Aug 30, 2024

Conversation

ralfbrown
Copy link
Collaborator

@ralfbrown ralfbrown commented Jun 29, 2024

This PR adds startup code to load styles from share/darktable/styles/ which are not yet present in the database, and also provides a set of styles to approximately match the look of camera JPEGs for most standalone-camera models for which samples are available from raw.pixls.us. Further, it updates the styles quick-access button in the darkroom to produce a full menu hierarchy on popup rather than collapsing everything below the top level into single huge submenus.

The provided styles are all under a common darktable camera styles top level to avoid name conflicts. This name is localizable.

This setup will allow us to accept user-contributed camera-match styles and ship them with future version of darktable.

@ralfbrown ralfbrown added the scope: image processing correcting pixels label Jun 29, 2024
@wpferguson
Copy link
Member

Tested.

It works.

Observations:

  • If you don't want them, you have to delete them one by one
  • If you don't want them and delete them they show up again the next time you start darktable because they are "system" files and you have to be a superuser to delete them. Maybe we should just copy them to the users darktable config folder and import from there? That way the user could get rid of them if they wanted to.
  • Maybe change the top level name to "Camera Styles"?
  • I tried a couple of the styles on Play Raw images and they give a result that is pretty close to the JPEG preview.

@ralfbrown
Copy link
Collaborator Author

I didn't use a name like "camera styles" because we'd want to internationalize that, which isn't possible atm. That would of course be better.

You can remove multiple styles at once by selecting them all before clicking on "remove", though you can't remove an entire virtual folder of styles by selecting just the top-level name.

Copying to the user config folder before importing would have the same issue of re-appearing, though only when clearing the folder or using a new one with --configdir. Is it really so bad to have one top-level entry in the styles list? After all, we have a lot of built-in presets which can't be removed either. Maybe a preference to hide that hierarachy? I'll have to look at the code to see if there's a reasonable way to determine whether the database is being newly created and only import then, because the only other way I can think of making them permanently deletable is not to automatically import them, in which case a majority of users will never discover them....

The results should look pretty close, as I tuned them up on the raw.pixls.us sample images. The biggest difference I'm seeing in the settings is the exposure compensation to match midtones to where the camera's tone curve puts them, which ranges from +0.3 EV to +1.6 EV, with most being +0.8 to +1.1. The styles are built with exposure, color balance rgb (saturation/vibrance), filmic rgb (global contrast), and local contrast modules.

@wpferguson
Copy link
Member

wpferguson commented Jun 29, 2024

though you can't remove an entire virtual folder of styles by selecting just the top-level name

I can vouch for that :-)

Copying to the user config folder before importing would have the same issue of re-appearing

If they are in the config folder the user owns them and should be able to delete them

Is it really so bad to have one top-level entry in the styles list?

If you are never going to use it? I could live with it (and if I got really annoyed I could fix it :-D). Others?

The results should look pretty close

Colors looked pretty much spot on in my small sampling. I did notice the exposure changes.

It should make the look like jpg crowd happy

EDIT:

I could make a lua script to go with it that runs on import and auto applies the "correct" style to the image.

@ralfbrown
Copy link
Collaborator Author

ralfbrown commented Jun 29, 2024

I could make a lua script to go with it that runs on import and auto applies the "correct" style to the image.

That would be a great addition, and would really make the look-like-jpeg crowd happy.

BTW, it's occurred to me that only importing the styles to an empty config runs into the problem that users upgrading from a previous version will never get the styles unless they manually import them from the system folder.... I think if we want them auto-installed but user-removable, we need to flag them as user-removed in the database to keep them from being re-imported.

@wpferguson
Copy link
Member

Here's a script that you can play with. Works on import and has shortcuts for apply to collection/selection

apply_camera_style.zip

@wpferguson
Copy link
Member

Updated to handle ICLE-9 series

apply_camera_style.zip

@ralfbrown
Copy link
Collaborator Author

I took a look at the code (currently don't have Lua setup in dt).

When I was generating and naming the styles, I wasn't thinking in terms of automatic name matching. It makes sense for us to discuss this so that the script won't need updating any time new styles are added... The main issue is of course the X-series styles; it wouldn't be difficult to just duplicate them for each known model, though that would mean potentially more maintenance in the future. I was also thinking of adding a "generic" style for each manufacturer (like I have at the top level) once I have a better sense of the typical settings for the maker. That would be a useful fallback when available.

Other than the Ricoh/Pentax normalization, the maker normalization could be handled by storing the lowercase version of the maker as the key in the lookup table (the original case is already present in the stored value at the bottom level).

Finally, shouldn't the required API version be the current 9.x rather than 7.0.0?

@wpferguson
Copy link
Member

wpferguson commented Jun 30, 2024

I'm letting the style naming/camera mapping thing rattle around in my head to see if I come up with a better way to handle it

Here's a dump of model and maker info from my play raw collection (nice diverse set of images for testing).

modelmaker.zip

Implementing a generic for each maker should be easy enough, just an else to the style check.

Finally, shouldn't the required API version be the current 9.x rather than 7.0.0?

Maybe. Right now it would run on anything greater than or equal to 7.0.0 (dt 3.6.0). I didn't look at the styles to see if they are using any modules that require a specific version of darktable. Here's the API <->dt version list

// 1.6 was 2.0.1
// 1.6.1 was 2.0.2
// 2.0.0 was 3.0.0
// 2.2.0 was 4.0.0 ( removed the ugly yield functions make scripts incompatible)
// 2.4.0 was 5.0.0 (going to lua 5.3 is a major API bump)
// 3.2.0 was 6.0.0 (removed facebook, flickr, and picasa from types.dt_imageio_storage_module_t)
// 3.6.0 was 7.0.0 (added naming to events, selections, and actions)
// 3.8.0 was 8.0.0 (moved to lua 5.4 and added some events)
// 4.2.0 was 9.0.0 (view toolbox functions and snapshot filename removed)
// 4.4.0 was 9.1.0 (added mimic and dt_lua_image_t changes)
// 4.6.0 was 9.2.0 (added change_timestamp to dt_image_t)
// 4.8.0 was 9.3.0 (added button and box widget enhancements)

Let me know where to set it.

EDIT:

If we're not using any really new modules, then users from earlier versions could just install the styles and still benefit from the scripts.

@wpferguson
Copy link
Member

wpferguson commented Jun 30, 2024

Idea: Use the model name as a search specification, i.e.:

EOS R series becomes EOS R? series
D7000 series becomes D7?00 series

brackets around multiple choice

EOS [7D,7DmkII] or EOS 5D Mark [I,II,III,IV]

Specifics become before generics, i.e.:

ILCE-9 Mark 3
ILCE-9? series

I should be able to read that and build the lookup table from it. That way whenever the styles change I should just be able to pick it up without a code change.

EDIT:

Keep the naming to what image.exif_model returns or query the models table in the database. I'll use the models table to figure out who likes to put their name on the model so I strip it out.

@wpferguson
Copy link
Member

Here's a sorted model list from my play raws

sorted_model_list.txt

@ralfbrown
Copy link
Collaborator Author

Idea: Use the model name as a search specification, i.e.:

Done. I verified that brackets and question marks in the name don't cause problems (they get replaced by underscores when generating the export filename). The bracket notation might cause a bit of confusion for less technical users, but it shouldn't be a big deal.

Specifics become before generics, i.e.:

Standard procedure :)

Keep the naming to what image.exif_model returns or query the models table in the database. I'll use the models table to figure out who likes to put their name on the model so I strip it out.

Should be OK for the majors (Nikon never had a D60 despite using D40/D50/D70/D80/D90, presumably because Canon had a D60 first). Might be one or two collisions among the smaller manufacturers.

@ralfbrown
Copy link
Collaborator Author

Maybe. Right now it would run on anything greater than or equal to 7.0.0 (dt 3.6.0). I didn't look at the styles to see if they are using any modules that require a specific version of darktable. Here's the API <->dt version list

If we're not using any really new modules, then users from earlier versions could just install the styles and still benefit from the scripts.

It's not an issue of how new the module is, but of the module's parameter block version. So far, I've only used local contrast, filmic rgb, color balance rgb, and exposure. Filmic is using v7, which is definitely newer than dt 3.6 - 4.2 or 4.4? But even if I were using e.g. v5, dt would be writing v7-compatible parameter blocks.

@wpferguson
Copy link
Member

filmic V7 was dt 4.4. Set API to 9.1.0

@ralfbrown ralfbrown changed the title styles: enable built-in styles, add sampling of camera-JPEG styles styles: enable built-in styles, add camera-JPEG styles Jul 4, 2024
@wpferguson
Copy link
Member

wpferguson commented Jul 4, 2024

Canon EOS RP needs moved above Canon EOS R? series

EDIT

The Sony I:CE-7 Mark 2 shows up as ILCE-7M2 (and 3, and 4, etc).

ILCE-7
ILCE-7M3
ILCE-7M2
ILCE-6000
ILCE-7RM2
ILCE-6400
ILCE-7RM3
ILCE-7RM4
ILCE-7C
ILCE-9
ILCE-6300
ILCE-6500
ILCE-7S
ILCE-7M4
ILCE-6600
ILCE-5100

Do you want to rename or do you want me to fix it in normalize_model()/

@wpferguson
Copy link
Member

Tested the on import case and it took 20 seconds to process 1044 images.

@ralfbrown
Copy link
Collaborator Author

The ordering you get is just whatever order the retrieval spits out, which is either lexical or whatever the ordering of the database records is. I haven't done anything to force a particular order. Lexical would make sense to see "R? series" before 'RP" since ASCII code for question mark is less than ASCII for P.

I've been using whatever model name shows up in the image information module while editing the sample image from RPU to generate the style. I still have around 90 Sony models to generate styles for, so I'll take another look while doing them.

I've realized that the per-manufacturer submenus are still unreasonably large for the major makers, so I'm planning on adding an intermediate level where appropriate (e.g. 'DC-TZ...' as parent for all the DC-TZ???? models). This won't be something that can be matched automatically, but it also will have no effect if youe script just ignores it. For example,

   if parts[3] then
      acs.styles[parts[2]][parts[3]] = style
   end

becomes

   if parts[3] then
      if parts[4] then
        acs.styles[parts[2]][parts[4]] = style
      else
        acs.styles[parts[2]][parts[3]] = style
      end
   end

and the rest of the script should work unchanged.

@wpferguson
Copy link
Member

Lexical would make sense to see "R? series" before 'RP"

The problem is when you cycle through the matches, RP matches R? before it gets to RP. I'll figure out a way to reorder it. Probably a look back at the previous value and an insert above if necessary.

@ralfbrown
Copy link
Collaborator Author

ralfbrown commented Jul 8, 2024

Rebased and partially squashed. Done, at least for now.

@TurboGit
Copy link
Member

TurboGit commented Aug 6, 2024

@ralfbrown : Stupid question maybe... But how those styles have been generated?

@TurboGit TurboGit added this to the 5.0 milestone Aug 6, 2024
@ralfbrown
Copy link
Collaborator Author

By hand - I went through the raw.pixls.us sample images and visually matched against an extracted copy of the embedded preview JPEG (or TIFF, in a few cases).

BTW, RPU has been down for at least a few days now....

@TurboGit
Copy link
Member

TurboGit commented Aug 8, 2024

You've made around 500 edits manually!!!!! That's a huge work. Not sure how close we are from the embedded jpeg, can't test but probably a nice first step for some users.

@ralfbrown
Copy link
Collaborator Author

Once I got a rhythm going, it took 2-3 minutes per camera. Most use I've ever made of snapshots :).

For a lot of cameras, we'd need to address saturated blues being too dark to get closer to the JPEG (thanks to the camera matrix having a negative coefficient for blue's contribution to luma). Likely via an instance of rgb primaries that attenuates blue. A large fraction of the sample images simply don't have strong enough blues to verify and tune settings.

add code to do localization of the vertical bar-separated portions of
a style's name when displaying the style name and when building the
menu or collapsible browser entries.

Prefixing a name's component with "_l10n_" will request localization
of the text following that tag.
The style quick-access button in darkroom view currently only supports
two-level menus, which is completely impractical when the new
'darktable camera styles' hierarchy runs to hundreds of styles.

This commit generalizes the menu building to go as many levels deep
as the treeview which is built in the styles module in lighttable,
so that the longest style menu is only as many items as a single
camera manufacturer has models.
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.

Tested on some images. The result is clearly better than the default dt rendering but not always close to the camera jpeg. But that's not the point at this stage, this can be improved later anyway if needed.

@TurboGit TurboGit merged commit 009a1e8 into darktable-org:master Aug 30, 2024
6 checks passed
@TurboGit TurboGit added priority: low core features work as expected, only secondary/optional features don't documentation-pending a documentation work is required release notes: pending labels Aug 30, 2024
@wpferguson
Copy link
Member

I'll put the final touches on the script, test it, and add it.

@ralfbrown
Copy link
Collaborator Author

not always close to the camera jpeg. But that's not the point at this stage, this can be improved later anyway if needed.

I'm not surprised, given a quick adjustment on a single example image. An updated style is something which users can very easily generate and contribute even if they aren't technically-inclined.

@ralfbrown ralfbrown deleted the cam_styles branch August 30, 2024 16:30
@TurboGit
Copy link
Member

@ralfbrown : Just noticed that now, the Style combo in the Export module is now unusable as proposing a flat list with all entries. Can you add a hierarchical view as you've done for the other parts in the UI?

@ralfbrown
Copy link
Collaborator Author

Will take a look, but probably won't get a chance until mid-week.

@wpferguson
Copy link
Member

Tested the script and cleaned up a couple of issues.

Build a lookup table based on https://en.wikipedia.org/wiki/List_of_Canon_products to map styles to equivalent cameras

local function mangle_model(model)
  if string.match(model, "eos") then
    log.msg(log.debug, "mangle model got " .. model)
    model = string.gsub(model, "r6m2", "r6 mark ii")
    model = string.gsub(model, "eos 350d digital", "eos kiss digital n")
    model = string.gsub(model, "eos 500d", "eos rebel t1")
    model = string.gsub(model, "eos 550d", "eos rebel t2")
    model = string.gsub(model, "eos 600d", "eos rebel t3i")
    model = string.gsub(model, "eos 650d", "eos rebel t4i")
    model = string.gsub(model, "eos 700d", "eos rebel t5")
    model = string.gsub(model, "eos 750d", "eos rebel t6i")
    model = string.gsub(model, "eos 760d", "eos rebel t6s")
    model = string.gsub(model, "eos 100d", "eos rebel t6")
    model = string.gsub(model, "eos 1100d", "eos rebel t3")
    model = string.gsub(model, "eos 1200d", "eos rebel t5")
    model = string.gsub(model, "eos 1300d", "eos rebel t6")
    model = string.gsub(model, "eos 2000d", "eos rebel t7")
    log.msg(log.debug, "mandle model returning " .. model)
  end
  return model
end

A little more cleanup and it should be ready to add

@ralfbrown
Copy link
Collaborator Author

the Style combo in the Export module is now unusable as proposing a flat list with all entries. Can you add a hierarchical view as you've done for the other parts in the UI?

I just took a quick look at the existing code in the export module. Unless there's some easy way to have sub-pulldowns in a combobox (which doesn't feel like something that would be supported), a different mechanism will have to do.

I'm thinking of putting the currently-selected style in a label and having a clickable button that will pop up the menu hierarchy for the styles. We could use either a simple down-arrow as is used on comboboxes or the same icon used for the styles quick-access button in darkroom. The rest of the code should then basically just be re-using the existing multi-level menus attached to the quick-access button.

@TurboGit
Copy link
Member

TurboGit commented Sep 2, 2024

I just took a quick look at the existing code in the export module. Unless there's some easy way to have sub-pulldowns in a combobox (which doesn't feel like something that would be supported), a different mechanism will have to do.

Indeed, never seen that done.

I'm thinking of putting the currently-selected style in a label and having a clickable button that will pop up the menu hierarchy for the styles.

Or just pop-up a menu for entries in the combo that are menu entries (that is, entry with sub-values. So in the combo we could have "darktable camera style", and by clicking on it we would open a menu (with sub-menu) as done for the darkroom style button bottom-left. Just thinking out loud...

@ralfbrown
Copy link
Collaborator Author

I'm not sure there's any non-hacky way to accomplish that, because it seems that Gtk's combobox functions just store the index of the selected entry, which then gets read in the callback function. Besides, the code to do the full menu hierarchy starting from a button is already in place....

@ralfbrown
Copy link
Collaborator Author

That should only happen once unless you delete data.db.
But darktable-clttest really should skip all the code for opening the library anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation-pending a documentation work is required priority: low core features work as expected, only secondary/optional features don't scope: image processing correcting pixels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants