darktable icon indicating copy to clipboard operation
darktable copied to clipboard

New IOPs to do some composing in darktable

Open TurboGit opened this issue 2 years ago • 48 comments

I have been working some time ago (commit says Mar 9th 2022) on this and the PR #15447 gave me some energy back to again see if this is possible in darktable or not.

This POC has two modules:

  1. Module enlargecanvas allows for enlarging the current canvas on the left, top, right or bottom. The enlarged part is filled with green (famous green used on cinema), red, blue, white or black.
  2. Module overlay allows for selecting an image-id and an history number to overlay the image on top of the currently developed one. Using a drawn mask or parametric mask (on the green for example) one can easily fill the part of the canvas without image with the content of the overlay.

Another usage is to only enlarge the canvas and use retouch or liquify to fill the new part in the image. This works fine for small areas.

There is lot of thing to do and if I have been able to do some compositing with those two modules there is some more work ahead:

  • [x] The UI should be reworked (better way to select the overlay)
  • [x] The placement of the overlay module in the pipe, currently it is too low and too early then and does not work properly as it is before the color out module. This means that the overlay part of the final image does two times on the color out, which is plain wrong. It is possible to move this module after color out if needed for now as a testing workaround.
  • [x] Fix all the remaining bugs...
  • [x] More code sharing as currently implementation has copied and trimmed down code from other modules.
  • [x] Fix min boder of 1px in right / bottom (fixes #15528). (#15566, dcefc7e886ac1cc01c6a9f3cb814db3b6e8f575e, 02d3c01c88f5628f02743998eda41dcc059ac4fc)
  • [x] More code sharing on the distort routines.
  • [x] Register somehow that a picture is used as an overlay and have a warning message if removed.
  • [x] Safe handling of multiple overlay instances.

That's all for now, comments welcomed!

TurboGit avatar Oct 21 '23 16:10 TurboGit

Sonds interesting.

Another usage is to only enlarge the canvas and use retouch or liquify to fill the new part in the image.

This is a use case that happens occasionally for me. A snapshot of a scene where you don't have the time to properly target the subject. This leads to the fact that the picture must be rotated and cropped strongly afterwards with the consequence that either

  • "black corners" appear and / or
  • e.g. legs or head of persons or other significant areas of the motif are cut.

At this point I have often wished for the described function. So far I solve this by exporting to GIMP and reimporting the result into darktable, but an internal option in darktable would of course be much more convenient.

pehar1 avatar Oct 22 '23 06:10 pehar1

I may have missed it, but I don't see overlay in its current state providing anything that isn't already present in watermarks, especially once moved late in the pipe, and it is missing the PNG support that was recently added to the latter. Might it not be better for maintainability simply to add a couple of presets to watermarks to blend on green (might require a bit of tweaking by user to account for colorspace differences) and black? And perhaps a toggle for whether or not to respect the alpha channel in the overlay image.

enlargecanvas looks worthwhile even though it is a cut-down version of borders because it's at the proper location to allow use of retouch/liquify and the simplification makes it more convenient to use for enlarging the canvas than borders, even without considering the need to reorder borders in the pipe for that use. Consider refactoring the common code into a separate shared file for future maintainability.

ralfbrown avatar Oct 22 '23 07:10 ralfbrown

I may have missed it, but I don't see overlay in its current state providing anything that isn't already present in watermarks

The point of overlay is to streamline the work. You don't need to export a PNG and clutter your watermarks directory with tons of PNG that you cannot remember if it is used or not. And doing this makes the watermark list so long that it makes harder to use the simple watermark module. So the goal here is to simplify, avoid PNG and directly use the pixelpipe directly. Doing this we will be able to mark the pictures used by an overlay module in the db and add some more check when one want to remove it for example. At the end it will be safer as currently you can just delete a PNG and break an edit. The watermark module has been done for one goal and it should stick to just this.

For enlargecanvas the goal is the same, streamlining it and making it simple to create borders, we don't need double border or to chose the color or to have aspect ratio...

At this point I have often wished for the described function. So far I solve this by exporting to GIMP and reimporting the result into darktable.

Yes, that's also what I want to avoid. Doing this into darktable ensure that we will get good quality and without jumping in different software which breaks the workflow. Again I'm trying o streamline a use case that we don't cover today without resorting to "tricks".

TurboGit avatar Oct 22 '23 10:10 TurboGit

Yes, this is an interesting new feature, but also a dangerous one (in terms of future developer effort spent to finish it "correctly").

Having to select an image and history end seems fragile to me. I'm not talking about that the way to select them might be made easier (by adding a dropdown or even a full thumbnail browser, whatever) but if the image is later edited, even just history compressed, all "dependent" images break. Just writing "dependent images" scares me. Because it breaks the fundamental idea that the pipe is self contained. The pipe started out from something that was purely sequential, where you could cache intermediate results, to something with a lot of interdependencies (where you better throw out those intermediate results because they might not be dependable anymore if you toggle something in an earlier module), to something that could break if you copy/paste some modules to an image that is being used in an overlay to the current one. My prediction is that you'll get a ton of negative feedback on this, forever, from people who expect it to work "correctly" but don't understand that making that happen takes workarounds at all levels that make it so much harder to make the rest of the program behave efficiently and consistently, even for those that never use these features. Moving further away again from doing-one-thing-well to trying-to-do-many-things-but-none-of-them-good-enough.

Duplication is a large part of the problem; the difference with watermark is subtle. If something needs to be fixed or added in one, it probably should in the other too, but from experience we know it won't and future maintainers will assume that those differences are intentional and not dare to touch them/clean them up. And then of course the bugs become features that people depend on yada yada. I'm giving up on removing duplicate code because it is being added faster than one could keep up.

So concretely my suggestions would be:

  • Extend watermark to also allow selecting an image instead of svg and text (not a history end, or please explain why duplicates would not be sufficient for allowing variations). Rename it to overlay if you want. Or "watermarks and overlays"
  • Make clear that the user is responsible for flushing the cache after this module if the image needs to be recalculated (provide a refresh button for this).
  • Explain in the manual what the difference is between "opacity" in the module itself (this goes for watermarks in the same way of course) and in the blending options. If there isn't one ("one is convenient/the other takes one more click" overlooks the fact that now the user spends time wondering what the differences is and maybe even experimenting and then giving up, with remaining doubts; doesn't feel "convenient" to me). Maybe disable (hide) it if set to default 100% (just because legacy params doesn't support enabling a blending mode to replace a module param).
  • explore how messy it would be to have "enlarge canvas" directly generate a raster mask. This raster could then be selected in overlay, rather than having to configure a parametric mask. The green room approach has all the usual drawbacks (green elsewhere in the image also gets replaced, so now you need to draw a mask anyway to prevent that). Or you could still use a very specific color and then allow quickly to select a parametric mask that only selects that? (maybe I'm overlooking a way to already do that; I'm separately setting the different color channels). Maybe in "raster mask" selecting "enlarge canvas" could be added that really selects that specific parametric mask. Of course black would be a bad choice.
  • I guess for retouch, a workflow could be: enlarge image on left by 100%, overlay image and x-offset -.5 to get it to the left, retouch the right side image using bits from the left, crop left 50% margin. Presets could be added to support/suggest that workflow.

dterrahe avatar Oct 22 '23 16:10 dterrahe

@dterrahe : Thanks for the comments. I'm aware of the limitations you listed. But remember this is a POC only. There is many solutions to improve things I'm sure.

One on my list is to explore a way to snapshot a dev (as done for the snapshot module) and save this as parameters to ensure we are independent of the dev of image used as overlay.

To select an image we could request user to add a specific tag to the image for it to appear in the list as image used for overlay (with a possible descriptive tag it would even help to select the proper overlay).

Again, there is lot of questions ahead for sure, but I'm sure we can safely and collectively find good and sounds solutions.

TurboGit avatar Oct 22 '23 16:10 TurboGit

  • Extend watermark to also allow selecting an image instead of svg and text (not a history end, or please explain why duplicates would not be sufficient for allowing variations). Rename it to overlay if you want. Or "watermarks and overlays"

Because overlay is very different than watermark as very early on the pipe. We do need to strip down the history of the overlay. There is no sense to bring an overlay having colorout, color calibration and filmic for example as those are done after the pixels have been "inserted" into the pipe. In fact I think that we need to remove all modules when rendering the overlay image appearing after overlay module in the image where it is used. Again, we are really speaking about a different beast here and I don't want to mess with the simple watermark module which is well placed on the pipe (practically as last module).

TurboGit avatar Oct 22 '23 16:10 TurboGit

In this last version I have improved the pixelpipe by disabling some more module to have a least something consistent in output for now. This is a hack and will be reworked properly at some point.

TurboGit avatar Oct 22 '23 18:10 TurboGit

There is many solutions to improve things I'm sure. I'm sure we can safely and collectively find good and sounds solutions.

I wish I shared your optimism. We don't need many solutions; we need 1 that works correctly, predictably, and that doesn't introduce a lot of complexity.

One on my list is to explore a way to snapshot a dev (as done for the snapshot module) and save this as parameters to ensure we are independent of the dev of image used as overlay.

I'm not sure I understand what you mean. Wouldn't that be the same as allowing to disable normally essential modules and saving as a duplicate? Or are you thinking of actual "composite" xmps?

To select an image we could request user to add a specific tag to the image for it to appear in the list as image used for overlay

Would that be scalable? This would work if a user reuses the same set of images as, basically, watermarks. Not if they want to have a workflow where they always combine two (or more) images. People will want to use this for collages. I understand that is not what you are building this for, but what do you expect to be valid use cases?

an overlay having colorout, color calibration and filmic for example as those are done after the pixels have been "inserted" into the pipe

So you expect the same settings for those modules to be applicable to possibly wildly different images? Not sure how that would work.

But it does make me wonder whether switching back and forth between images that are being overlayed on top of each other to tweak how they respond to each other's settings is going to be the norm rather than the exception. And if that's the case, then the lack of caching (which currently happens per pipe and hence is thrown away with the temporary pipe to calc the overlay) is going to annoy anybody without an extremely fast machine.

Imho the pipe and cache already need fundamental changes and this would make that even more important. But more likely, workarounds and hacks will be introduced to make this case slightly better, while at the same time making it impossible to restructure and fix the underlying shortcomings of the current implementation (because it will just become, if it isn't already, too unwieldy to understand and mess with).

My worry is not that this thing can't be made to work as well as could possibly expected giving all the theoretical issues with processing two images simultaneously. My worry is that to do it, a price will be paid in maintainability of the rest of the program, leading to fewer improvements for features that people will actually want to keep on using.

Looking at it that way, keeping this as a separate module, even if heavily duplicated, is preferable because it can be ignored. But how do we stop it "bleeding" into the rest of the code base?

But remember this is a POC only.

That's why I'm responding now, rather than later saying "SEE, I could have told you!" and running off in a huff to push my own fork and insult people from a distance.

This is like the start of all the preset changes. Maybe those were good and necessary and maybe people use the new functionality they offer, but it definitely took a long time to find fixes for all the unforeseen (?) consequences and regressions and for me made the presets thing too scary to look at, let alone work on.

dterrahe avatar Oct 22 '23 18:10 dterrahe

So you expect the same settings for those modules to be applicable to possibly wildly different images?

No that's not what I meant.

My worry is that to do it, a price will be paid in maintainability of the rest of the program, leading to fewer improvements for features that people will actually want to keep on using.

This is probably not true as the change made outside the two new modules are very limited (at the moment).

Wouldn't that be the same as allowing to disable normally essential modules and saving as a duplicate?

Not sure I fully understand this but this will indeed require lot of messy code to be introduced to support these new kind of duplicates. So for maintenance it may be a bad idea.

TurboGit avatar Oct 22 '23 20:10 TurboGit

In your first post you describe the what, but not the why. Can you elaborate on that further? Why this feels necessary to you, what problem it solves?

My understanding may be off, but it sounds like functionality suited to gimp or krita. Other than not having to use two programs, is there any particular advantage to using darktable for this? Just curious.

SoupyGit avatar Oct 22 '23 23:10 SoupyGit

@dterrahe and @TurboGit my thoughts about the pixelpipe and it's cache.

If there is a module changing it's output depending on more than a) it's own parameters and b) the parameters of preceding modules (as you do by adding an "unspecified overlay" or whatever you name that) - that would be exactly the usecase for dt_dev_pixelpipe_disable_after(). That would mean recalculation is required only for following modules and changed parameters, would be mask-safe ... So no need for any "reset button" or alike.

From how i understand the current pr code, also absolutely no "need" for tricks like dt_dev_pixelpipe_disable(). That looks pretty dangerous / difficult to maintain to me btw.

jenshannoschwalm avatar Oct 23 '23 04:10 jenshannoschwalm

@SoupyGit : The why is explained in a following comment: https://github.com/darktable-org/darktable/pull/15465#issuecomment-1774061131

TurboGit avatar Oct 23 '23 08:10 TurboGit

, also absolutely no "need" for tricks like dt_dev_pixelpipe_disable()

Sure, the new version is cleaner. As I said this is very early work and I had done lot of shortcut to just check that my idea was possible to implement.

TurboGit avatar Oct 23 '23 08:10 TurboGit

@SoupyGit : The why is explained in a following comment: https://github.com/darktable-org/darktable/pull/15465#issuecomment-1774061131

There you say it is to streamline "the work", but don't say what "the work" is. That's what I'm wondering. Is it to be able to retouch from areas outside the crop? To outpaint into black areas that result from rotating? Something else?

SoupyGit avatar Oct 23 '23 11:10 SoupyGit

So no need for any "reset button" or alike.

By what mechanism will the module pick up changes to the overlayed image? (For example a copy/paste into the filmstrip). One can switch back and forth to another image; that would trigger param changes twice.

No that's not what I meant.

My question was an invitation to explain what you do mean. If I understand correctly, you intend to merge two images early in the pipe, at which point a lot of adjustments still need to be made. The adjustments required will probably differ between the two images. I genuinely would like to understand how this will be made possible. Not in detail; at an abstract level.

dterrahe avatar Oct 23 '23 12:10 dterrahe

Is it to be able to retouch from areas outside the crop?

I would say that the overall goal is to add some borders to "fix" the composition. So:

  1. Have the possibility to enlarge the canvas
  2. Use Retouch or Liquify to fill the added areas (borders) from step 1
  3. Use another image or a copy of the current one to fill the added areas

TurboGit avatar Oct 23 '23 13:10 TurboGit

By what mechanism will the module pick up changes to the overlayed image

Indeed, I missed that scenario. Probably we could use the change timestamp of the overplayed img?

jenshannoschwalm avatar Oct 23 '23 13:10 jenshannoschwalm

By what mechanism will the module pick up changes to the overlayed image? (For example a copy/paste into the filmstrip). One can switch back and forth to another image; that would trigger param changes twice.

I'm pretty sure we have a misunderstanding as I don't see/understand the question.

To me there is no need for a reset button, the work is done in process() routine as for all other IOP when the pipe request for the module to be run. I bet this is not what you are talking about?

TurboGit avatar Oct 23 '23 13:10 TurboGit

Indeed, I missed that scenario. Probably we could use the change timestamp of the overplayed img?

Ok, so that's me... Can you explain the scenario ?

TurboGit avatar Oct 23 '23 13:10 TurboGit

By what mechanism will the module pick up changes to the overlayed image?

Maybe you mean how to select the image to use into the overlay module? This to replace the current image-id entry in this PR?

TurboGit avatar Oct 23 '23 14:10 TurboGit

Scenario: I have two images in the filmstrip in darkroom, A and B. I process A and compress history, note number of last history item. Press <space> to go to image B. Do some processing. Add and overlay module that contains A with the history item I want. Now I notice A is too dim relative to B. So I go back to A (<backspace>). Adjust exposure. Compress history. Go to B again. Now B hasn't changed in the mean time; the overlay module still refers to the same image and history item. It could still have results for the current history/pipe in cache (at the moment it seems it is getting flushed when switching images, but that's something I intended to look into, understand and possibly fix). So now I might need to force processing of overlay to pick up the changes.

Another scenario could be ctrl+shift+c/v ing some module settings from B to A (using the filmstrip). The overlay module in B won't know that the underlying image has changed/the cache will prevent its process from being run, so it can't check and notice. So there'd need to be some other mechanism to force a recalc.

dterrahe avatar Oct 23 '23 14:10 dterrahe

@dterrahe exactly as you described. Apply a style was my original thought.

As we have to insert the other image in process runtime..

jenshannoschwalm avatar Oct 23 '23 14:10 jenshannoschwalm

@dterrahe : I see, note that in later version of this PR here there is no more history end. What is done for the image to be used as overlay (A in your scenario):

  • We always take full history of A (no more history end)
  • But all modules of A that are part of B and above the overlay module are skipped

This is needed as we don't want double colorin, colorout or whatever non dev done in B above overlay. Double Flimic won't have any mean.

Another option would be to always create the overlay image with very minimal history (rawprepare, temperature, rawdenoise, demosaic, maybe some others). So using only mandatory modules, but there is some shortcoming hence the implementation above.

  • A is using an exposure of +1EV
  • In A you add a small border on top (sky) using enlarge canvas
  • In A you use a overlay module and fill the top with some more blue sky

The sky of the overlay area will miss the exposure that comes before the overlay module. That's why I currently keep all modules coming before the overlay module itself.

After experimenting with the current PR it seems to work quite nicely in different scenario.

TurboGit avatar Oct 23 '23 15:10 TurboGit

So it's mostly about outpainting. To me, a job for gimp or Ai stable diffusion (which I think can be used within krita now). Thanks.

SoupyGit avatar Oct 23 '23 22:10 SoupyGit

To me, a job for gimp or Ai stable diffusion (which I think can be used within krita now).

I understand that you wrote "to me", but that is the kind of comment that is no way helpful. We can do everything outside darktable. We had comments about removing slideshow, removing tethering, removing the print module (he! you export and print with Gimp), some have even been asking for a way to remove the lighttable... So at the end what's the point?

The project darktable we are trying to make moving is a all-in-one software trying to avoid breaking the workflow by exporting, using some other tools and importing again. This is for speed and to get high end quality of the result.

So if you prove me that this PR will break darktable in a way that no one will be able to use it or that is will never be made to work properly, ok I'll scratch it. Otherwise? Just two new modules that could be useful for people having the need of them.

I do even see the Enlarge Canvas module as a first step to create panorama directly in darktable. Why not? A huge work but plugin some nice external library could do the work in some years and if someone come with the motivation.

TurboGit avatar Oct 24 '23 15:10 TurboGit

Just to be sure there is no miss-understanding. I do not plan this for 4.6, in fact I have not yet planned anything about this PR. Again it is a POC to get constructive feedback about the best way to move forward... or not.

TurboGit avatar Oct 24 '23 15:10 TurboGit

I understand that you wrote "to me", but that is the kind of comment that is no way helpful.

It was not meant to be either helpful or not. Just to courteously say thanks for your response and explanation. I included my own opinion in case you were wondering what I thought of it now I know what it is about, not to suggest it shouldn't be done. There are likely many opinions on that, but I know in open source world that what gets done is generally what gets coded. As you say, so long as it doesn't break darktable, it won't bother users who don't use it.

SoupyGit avatar Oct 25 '23 00:10 SoupyGit

I do even see the Enlarge Canvas module as a first step to create panorama directly in darktable. Why not? A huge work but plugin some nice external library could do the work in some years and if someone come with the motivation.

Oh yes, please..!!!

HansBull avatar Nov 01 '23 05:11 HansBull

A few months ago I did a test with the watermark module without leaving darktable to GIMP (which is where this actually needs to be done) but I did it to test the potential of overlaying one image with another, not only in the case of putting a signature or a logo and don't have to import the composite result from GIMP to darktable again.

I don't think there are many interested in such a function (maybe I'm wrong), as this kind of functions are more feasible in compositing programs like GIMP or Krita.

As @TurboGit says, the watermark module is not the most suitable for that function, since png files are saved and if deleted destroy the edit.

https://github.com/darktable-org/darktable/assets/67672868/7dc61e5e-40e6-4ab6-b8fa-1eb6da6dd627

difrkaguilar avatar Nov 08 '23 13:11 difrkaguilar

BTW speaking about modules that gave a plus to darktable, frame module is far beyond any alternative in Lightroom or Capture One. And this is one of my top modules for creating content for Instagram.

difrkaguilar avatar Nov 08 '23 14:11 difrkaguilar