Fix neural denoise/upscale grouping logic when input image is not the group leader

[jeffc]

This PR fixes a bug in the neural_restore functions (raw denoise, denoise, and upscale) where the result of the operation would not be grouped with the input image unless the input image was its group leader. Image groups in darktable are identified by their group leader (see src/common/grouping.c). Previously, the "group with original" logic was just calling dt_grouping_add_to_group() with the input image's ID, which isn't necessarily the same as the input image's group ID.

disclosure: bug identified and fixed in part by AI tools with close human supervision

[wpferguson]

What happens when you ungroup? Does it break the "image leader" away from the group that it already belonged to?

Image groups in darktable are identified by their group leader

The group ID is the image ID of the group leader. All ungrouped images are basically groups of 1 since the image id and group id are the same.

I'm concerned about side effects of grouping an image in a separate group with a member of another group. While the current situation may not be the most "correct" grouping, the generated image is in the same group as the original image and in most cases (dependent on sorting) will appear next to it in lighttable.

[jeffc]

@wpferguson Based on my read of the code, the situation you're describing (grouping an image in a separate group with a member of another group) is the current state, and this PR fixes that behavior.

Say image id 001 and 002 are in a group, with 001 being the leader. That means that the group id for that group would be 001. If we run neural denoise on image 002 to create 003, the current code assigns that image to a group with ID 002, which isn't the same group as the original two images. This fix looks up the group ID for the source image (in this case, the source image being 002 with a group ID of 001) and assigns the new image to the correct group, 001.

Ungrouping then would work as normal (if we remove 003 from the group, it becomes its own group with ID 003)

Testing the current behavior confirms that this is the case -- on master, I have two images (a JPEG and a CR3) grouped with the JPEG being the group leader. When I run raw denoise on the CR3, it creates a denoised DNG that doesn't group with the other two. With this fix applied, denoising the CR3 properly adds the new DNG to the group with the other two (but leaves the JPEG as the group leader).

Am I missing something?

[wpferguson]

I think the correct behavior is to group the denoised image with the source image.

Let's say you are shooting the interior of a house with only RAWs. You group the images by room. You pick one of the images in a group to denoise and it gets grouped with the "source" image. You ungroup the "source" image from the room group. The source and the denoised result should be grouped together, IMO. If we make the denoised image part of the "parent" group, then the source image could be removed and the denoised image would still be part of the group, but with no "source" image.

[jeffc]

That reasoning makes sense to me, but that would mean a different bug fix -- basically, it would mean allowing nested groups in darktable, which is a bigger conceptual change.

As things stand now, the behavior is broken in a really unintuitive way:

• Images A and B are in a group, with A as the leader. The group is identified with A as the group id
• We denoise B to create C. The new image is given group id B (which is its source)
• The UI shows two groups: Group A (containing images A and B) and Group B (containing only C).
  • This renders in the UI as A and B in a group (with A as the leader) and C ungrouped
• If we now remove B from the group, it gets its group ID reset to its own ID (B)
• Now, right after we hit "ungroup" on B, it renders as grouped with C (since they both have B as their group id)

This is a really confusing flow for users, especially considering that the behavior if B (the image being denoised) is the group leader. In that case, everything works as documented (the new image C is added to the group and made the leader).

I see a few different options here:

1. Always adds the new image to the group containing its source (in my example above, adds C to group A). If the source was the leader, the new image becomes the leader instead. That's what this PR does.
2. Update the darktable grouping logic to allow for nested groups, and form a nested group with the source and denoised image within whatever group previously contained the denoised image. This would let the source remain in its current group, but the denoised version would follow it if ungrouped. I think this would be a pretty significant change to the way that groups are handled, both in terms of the logic behind it and the UI -- we'd have to find an intuitive way to render "these images are in a subgroup", and also figure out the semantics of what the "group" and "ungroup" buttons do. If I select a group of images [A,B] and an ungrouped image C, should "group" add C to the group and make [A,B,C]? or should it make a subgroup [[A,B],C]? Should pressing "ungroup" on [[A,B], C] lead to [A,B] and C, or A,B,C (ie, do we dissolve one group level or every group level)?
3. Move the source image out of its current group and form a new group with the denoised version
4. Don't group the newly generated image anywhere
5. Add a drop-down selector to let the user choose between these behaviors.

What I'm doing in this PR (option 1 above) feels to me like the best solution. If you'd like I can open a bug and move this discussion there.

[wpferguson]

If you'd like I can open a bug and move this discussion there.

I think this probably needs to be an RFC.

I don't know if we really want to mess with rewriting grouping (or trying to get a consensus of what it should look like 😁 ). However, if we do decide to redo grouping, I would suggest functional tagging (darktable|group|UUID) which would allow groups of groups and persistence (via XMP sidecars) across database instances.

EDIT: I can live with this PR. Maybe for 5.8 we need to do an RFC about grouping and what it should look like and how it should work.

[jeffc]

EDIT: I can live with this PR. Maybe for 5.8 we need to do an RFC about grouping and what it should look like and how it should work.

Sounds good to me. I think there's an interesting discussion to be had about the "intended" function of groups (and obviously, people are going to use them however they want). Personally, I shoot RAW+JPEG, and I rely on grouping to keep the two formats together for each picture. I can see a world where there's the idea of "a shot" -- which could have different formats, denoised+original, copies with different edit histories, etc) -- independent of "a group" (which could contain multiple shots). It's definitely worth discussing more though.

[andriiryzhkov]

@wpferguson looks like there is a real UX trade-off. As I read it, there are three options:

1. What the PR does: denoised joins source's existing group. If the user later pulls source out of that group, denoised is left behind in it. Conservative – doesn't touch any grouping the user already made.
2. What you're suggesting: pull source out of its existing group and form a [source, denoised] pair. Source-denoised always stay together, but we're silently splitting source from a group the user deliberately created.
3. Promote source to leader of its current group, then add denoised. Both groups stay coherent and denoised follows source automatically. Downside: we override whoever the user picked as the group leader.

One thing worth noting: the pre-PR code (dt_grouping_add_to_group(source_imgid, newid)) looked like it was trying to do something like 2 by passing source as the would-be leader, but it silently broke when source wasn't a leader – the denoised image got orphaned out of any group entirely. So the underlying bug is "denoised disappears from groups when source isn't the leader," not strictly "denoised joins the wrong group." The PR's change addresses the mechanical bug by picking 1; your concern is a separate (and fair) UX question about which of 1/2/3 is the right behavior.

My two cents: 1 is the smallest fix to the silent-orphan case. 2 and 3 are behavior changes worth their own discussion. Probably, would be good to have it noted in the source so the next person who looks at this knows it's a deliberate choice rather than the easiest path.

[wpferguson]

@andriiryzhkov I agree, 1 is the way to go for now. Anything thing else is going to take a lot of thought trying to cover all the bases.