-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
iop-order: fix lut3d order (after colorin). #3075
Conversation
And I have checked that the only changes with this patch are the swap for: from:
to:
|
On-the-fly v3->v(master) :-) ??? |
Yes, no way around that :) |
Also v4->v(master) ? |
b9e82e7
to
3ad56f7
Compare
@aurelienpierre : new order proposed is:
|
Well v4 -> (master) ... not sure this is just a minor issue. No crash and how many people have been using v4 ? |
It's probably safer to put tonemap after toneequalizer, although these two don't really fit in the same workflow anyway, so I'm not sure it's important to bother. Then, from colorin, the order should be:
|
@aurelienpierre I couldn't find such a description about the rationale of a good/suggested pipeline order (with user readable module names) in the manual yet? Something like that should be in a section about iop reordering. Or maybe we ask some people about their personal workflow and could have that as an introduction? |
Most people's workflow is just an accidental habit, better not ask them. A rational pipeline is based on the meaning of the operations we do, past the maths of it. For example, the beginning of the pipeline is pure signal processing, where we revert by calculation the flaws left by the sensor and the optical media in the opposite order they are applied in reality. See #2905 for further explainations. At that point, you have recovered the latent image, aka the theoriticaly perfect original image, exempt of any optical artifact. Then, we aim at matching whatever sensor RGB space (that is not colour, RGB doesn't mean colour, it means some spectrum got split into some tristimulus) to sensible, human-defined colours, through the input colour profile and the colour checkr modules. After that point only, RGB means colour in our perceptual reference (but is still scene-linear encoded). After that come the frequential operations, high-pass, low-pass and band-pass filters, which theory is grounded in Fourier's spectral analysis. They aim at affecting local or global contrast without changing the average lightness. You can see them as a modulation around a neutral value that is left unchanged. Then come the scene-referred colour manipulations, that aim at adjusting colours as if we were physically playing with light emissions. The huge benefit of these operations is the are using physically-accurate models, so they can tolerate dramatic edits with no weird stuff happening. It's really like having a virtual camera in-computer allowing you to redo the shot a posteriori. Then comes the camera transform, that does something similar to what film and brain do the scene-referred tristimulus : raise midtones, add contrast. Either filmic or basecurve in darktable. At that point, your signal becomes display-referred, meaning you have lost the physical relationship with light and whatever operation you might perform after might blow up in your face. Here come most of darktable 2.6 operations, using Lab colour space, and working not so well for dramatic edits (witness colour shifts and saturation issues while pushing lightness or contrast). This is a legacy workflow inherited from the 1990's where dynamic range was not an issue, because it was pretty low. You may find that series of posts useful too: https://medium.com/the-hitchhikers-guide-to-digital-colour I would be grateful if someone else had time to make a nice summary of all this info for dt's manual, because my nights are quite short already. |
Take the opportunity to set a clean state as discussed on darktable-org#3075 with Aurélien Pierre. For darktable-org#3062.
3ad56f7
to
5b5a61f
Compare
Ok, I have update the iop-order to v5 with a clean flat list has proposed by Aurélien (@aurelienpierre thanks a lot again!). I do hope that v5 is the definite iop-order for next release. As hinted by @jenshannoschwalm I have also enabled v3->v5 conversion to ensure no more v4 are remaining on earth! @aurelienpierre : can you double check I have not messed up the order ? |
src/common/iop_order.c
Outdated
@@ -1433,11 +1558,11 @@ int dt_ioppr_convert_onthefly(const int imgid) | |||
// already latest | |||
if (my_iop_order_version == DT_IOP_ORDER_VERSION) return my_iop_order_version; | |||
|
|||
// ??? we handle only iop-version 3 (which has been broken) and move | |||
// it to new v4. this routine will be reused later when dt will | |||
// ??? we handle only iop-version 3/4 (which has been broken) and move |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iop-version 3/4 (which has been broken) --> (which have been broken)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Yes go ahead. (5 times finger crossing makes ten, not many left ...) |
Take the opportunity to set a clean state as discussed on darktable-org#3075 with Aurélien Pierre. For darktable-org#3062.
5b5a61f
to
04a4972
Compare
Thanks! I'll wait for Aurélien review to be sure all is good and avoid introducing a v6 :) |
From #3071
Yes. BTW i also found 0 as the iop_version in my images database. ?? Didn't know about that. If i develop such an image in darkroom and add a new module like filmicrgb, the iop_order version is changed in the database. This part of the code is heavy stuff. I have done lots of real time audio processing with live reordering of filters but this is driving mad. Keep going! There is one more thing i am suspicious about. Copy/merge/paste of history. There might be more problems ... i will look into that tomorrow and ping you about surprises. |
0 means not yet developed, just discard history from lighttablle and look at the database, iop_order_version should be 0. Such mage when edited will get the latest iop_order version. |
In general, any sort of 'history insertion' from one image to another of does not take care of different iop_order versions. It could be done but would mean endless changes in all parts of the history stack prone to introducing new bugs i guess. One suggestion, after looking at the onthefly code again, i think it is factored in a wrong way for re-usability. The test for version like |
@jenshannoschwalm : just saw your comment, look at my new implementation it should be cleaner and handles more cases v1 -> v2 and v3-4 -> v5. I hope we are seeing the light at the end of the tunnel :) |
@TurboGit will do a thorough look later today - currently at work. BTW if we are here, i played around with iop rordering to do a lot of tests. Mostly fine. |
I will review that later today too. |
We probably need more. But at this stage I'd like to avoid that to not introduce more regressions. This is a delicate area (fences) and I do not master this part of code yet. So I want to be conservative. |
About fences. Yes we need more later. But as there are many issues and discussions about a modified iop_order. If i try to think of module reordering in an absolutely non-technical way a user might think the layout is only a layout change and not a reordering of the pipeline which can give at least strange results. Could we add a button to reorder the pipeline to default or did i miss that? |
No, you did not miss that there is none. But that was my idea when I said that later we will use the code you've implemented to migrate to last IOP order version. That means also that we will somehow revert to latest has changed the order manually. All this for after 3.0 anyway. |
src/common/iop_order.c
Outdated
// returns the history version of imgid | ||
int dt_ioppr_convert_onthefly(const int imgid) | ||
// migrate to another iop_order version the given image | ||
// not that this is actually a non exported routine but it will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- migrate the given image to another iop_order version
- note that this is :-) First i misunderstood
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
// it to new v5. this routine will be reused later when dt will | ||
// propose in GUI a possibility to migrate old edits to a new | ||
// version of iop-order. | ||
return _ioppr_migrate_iop_order(imgid, my_iop_order_version, DT_IOP_ORDER_VERSION); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably DT_IOP_ORDER_VERSION is right here instead of 5. Maybe leave a comment ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
That refactoring seems to be perfect. |
This is needed as new modules (filmic rgb, levels rgb, ...) are only known starting with v2 which is the first iop-order version supported. The v1 is a snapshot of the original static order hardcoded into dt. Fixes darktable-org#3071.
35c79b8
to
4934bbf
Compare
Thanks for the review! |
btw, there is one superflucious undef at the end of file. I took the last hours running master with this pr included and tested a lot of images having log on. The migrations all went fine! I had one dt crash, that was a v2 history with wrong iop_orders. I don't bother. Some thousands worked just fine. There is one thing i observed here, some v2 files have these warnings. [_ioppr_check_rules] found rule flip clipping module clipping (17,000000) is before flip (20,000000) image 0 (dt_dev_pop_history_items_ext end)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IOP order is good to me. Not sure I understand the dt_ioppr_convert_onthefly, I trust you on this one.
Ok, let's merge this now to avoid issues propagating more. Thanks all for review! |
For #3062.
Another iop-order bug!
@aurelienpierre : I'd like to review the iop-order after this patch to ensure we are ok to avoid multiplying the iop-order version (already 5 of them just for a dev version :).