RFC: .dtdata sidecar for per-pixel edit data, first used by external raster masks - #22291
andriiryzhkov wants to merge 6 commits into
Conversation
a2e6dcc to
9d25a78
Compare
TurboGit
left a comment
There was a problem hiding this comment.
A first quick pass on the code.
There was a problem hiding this comment.
🟡 Changes recommended
Critical resource-safety and sidecar consistency issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Introduces .dtdata sidecar files for per-pixel edit data, initially integrating external raster masks with history and image lifecycle operations.
Changes:
- Adds libarchive-backed ZIP/PNG sidecar storage, references, merging, cleanup, and tests.
- Integrates sidecars with raster masks, history, renames, local copies, and deletion.
- Updates dependencies, packaging, documentation, and release notes.
File summaries
| File | Reviewed changes and final findings |
|---|---|
src/views/darkroom.c |
Adds sidecar sweeping during darkroom transitions. No final findings. |
src/tests/unittests/common/test_dtdata.c |
Adds sidecar unit tests. No final findings. |
src/tests/unittests/common/CMakeLists.txt |
Registers sidecar tests. No final findings. |
src/lua/image.c |
Handles sidecar cleanup during Lua history resets. No final findings. |
src/iop/rasterfile.c |
Integrates raster masks with sidecars. Moderate (3 votes): preserve 16-bit PNG depth. Nit (1 vote): regenerate the translation catalog. |
src/control/jobs/control_jobs.c |
Adds sidecar deletion with duplicate cleanup. Moderate (2 votes): independently enumerate .dtdata paths when deleting unique images. |
src/common/image.c |
Handles sidecar moves and local copies. Moderate (2 votes): make sidecar moves transactional at lines 2422, 2900, and 3023. Moderate (1 vote): move local-copy sidecars during rename. |
src/common/history.c |
Integrates sidecars with history operations. Moderate (1 vote): use cache fallback during merge. Critical (1 vote): retain sidecar data for undo. Moderate (2 votes): handle merge failures before committing history. |
src/common/dtdata.h |
Defines sidecar APIs and reference types. No final findings. |
src/common/dtdata.c |
Implements sidecar storage and lifecycle operations. Critical (1 vote): bound decoded PNG dimensions and allocations. Moderate (2 votes): validate sources before copying at lines 619 and 639. Critical (1 vote): recheck archive contents under the write lock. Moderate (1 vote): use cache fallback during merge. |
src/CMakeLists.txt |
Adds libarchive build integration. No final findings. |
RELEASE_NOTES.md |
Documents user-visible sidecar changes. No final findings. |
README.md |
Documents the required dependency. No final findings. |
packaging/nix/flake.nix |
Adds the Nix libarchive dependency. No final findings. |
packaging/macosx/BUILD.txt |
Adds the macOS libarchive dependency. No final findings. |
packaging/macosx/BUILD-ARM64.txt |
Adds the ARM64 macOS libarchive dependency. No final findings. |
dev-doc/README.md |
Links the sidecar developer documentation. No final findings. |
dev-doc/dtdata_sidecar.md |
Documents the sidecar format and lifecycle. No final findings. |
Review details
Suppressed comments (7)
src/common/dtdata.c:639
- A failed
_zip_read()means the source entry is missing or damaged, but continuing leavesoktrue. Callers treat this merge as successful, so history paste can create references to absent bytes and local-copy reset can discard the only sidecar copy.
if(!data) continue;
src/common/dtdata.c:848
dt_dtdata_merge()resolves only the primary path, unlikedt_dtdata_read_gray()which falls back to_cache_path(). If a local copy has a sidecar entry created while the original was offline and the original becomes reachable before reset, this selects the empty original path;dt_dtdata_file_merge()treats the missing source as success and pasted history references a missing entry. Use the same cache fallback when selecting the merge source.
dt_dtdata_path(src_imgid, src, sizeof(src));
dt_dtdata_path(dst_imgid, dst, sizeof(dst));
if(!src[0] || !dst[0]) return FALSE;
return dt_dtdata_file_merge(src, dst);
src/common/history.c:969
dt_dtdata_merge()resolves onlydt_dtdata_path(), which prefers the original as soon as it exists; unlikedt_dtdata_read_gray(), it does not fall back to a local-copy sidecar. If a mask was imported while the original was offline and history is pasted after the original reappears, the destination gets params for a cache-only entry that was never copied. Merge the cache sidecar as well, or make merge use the same fallback resolution.
// params travel verbatim, so raster entries they reference must too
dt_dtdata_merge(imgid, dest_imgid);
src/common/image.c:2904
- A sidecar copy failure is only logged, but the image is still marked as a local copy. If the original then goes offline,
dtdata_path()resolves the history reference in the cache and the missing copy makes the imported mask disappear. Treat this as a failed local-copy operation or retain a usable fallback before setting the flag.
if(!g_file_copy(src, dest, G_FILE_COPY_OVERWRITE, NULL, NULL, NULL, &gerror))
{
dt_print(DT_DEBUG_ALWAYS, "[dt_image_local_copy_set] cannot copy '%s' to '%s': %s",
srcdata, destdata, gerror ? gerror->message : "");
g_clear_error(&gerror);
src/common/image.c:3027
- If the cache-to-original merge fails, this branch only logs the error; execution then clears the local-copy state, so the remaining cache sidecar is no longer used while the original sidecar may not contain the referenced entries. Do not complete the reset until the merge succeeds, or preserve a fallback that remains addressable.
if(dt_dtdata_file_merge(locdata, origdata))
{
dest = g_file_new_for_path(locdata);
if(g_file_test(locdata, G_FILE_TEST_EXISTS)) g_file_delete(dest, NULL, NULL);
g_object_unref(dest);
src/common/image.c:2404
- The new code moves the sidecar beside the original image, but
dt_image_rename()also moves an existing local-copy image later in this function and does not move its.dtdata. After renaming an image with a local copy, taking the original offline makes the cache fallback look at the new cache path and loses the raster entry.
// the .dtdata sidecar follows the xmp wherever it goes
gchar olddata[PATH_MAX] = { 0 }, newdata[PATH_MAX] = { 0 };
dt_dtdata_path_for_image(oldxmp, olddata, sizeof(olddata));
dt_dtdata_path_for_image(newxmp, newdata, sizeof(newdata));
src/iop/rasterfile.c:401
- the new translatable msgids introduced in this module, including
missing from sidecarandmask %.8s, are absent frompo/darktable.pot; that catalog still contains only the pre-sidecar rasterfile entries even thoughPOTFILES.inlists the source. Regenerate the catalog in this change so these strings are available to translators.
gchar *full = g_strdup_printf("%s (%s)", text, _("missing from sidecar"));
- Files reviewed: 18/18 changed files
- Comments generated: 8
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
15843ba to
324d3c9
Compare
bafe112 to
c96b172
Compare
| - A new `.dtdata` sidecar next to the XMP holds per-pixel edit data | ||
| that does not fit an XMP. It is a plain zip of PNG entries, follows | ||
| the image when it is moved, renamed, duplicated or deleted, and is | ||
| written only when sidecar writing is enabled. Entries no history |
There was a problem hiding this comment.
That came from a review fix (discard history no longer deletes the file, undo could lose masks). Orphaned entries are still cleaned up: on darkroom leave or history compress, entries that no history item references any more are dropped, and the file is removed once empty. Details are in dev-doc/dtdata_sidecar.md, the release note now just says what the file is and that only images using such data get one.
The external raster masks module referenced a PFM/PNG by path inside a root folder set in preferences, so a mask did not follow the image when it was moved, copied, duplicated or its history was pasted. Add common/dtdata.c: a flat zip next to the XMP, same duplicate suffix, one gray PNG per entry named by the SHA-1 of its bytes and never modified. History rows point at entries by name only, so the sidecar is found by the same rule as the XMP. It follows the XMP on rename, move, delete and local copies, and entries are merged when history is pasted. Entries no stored history row references are dropped when the darkroom is left and by the lighttable compress job, never on an undoable path; discarding the history deletes the file. The external raster masks module imports a file into the sidecar (params version 2, old folder references still honored). With sidecar writing set to "never" the root-folder chooser remains. libarchive is now required unconditionally instead of only with AI. Related: darktable-org#22226
The xmp is rewritten from the database on the next write, the masks in the sidecar exist nowhere else. A missing xmp is not a failure. Also note in dtdata.c that the volatile locals exist because libpng reports errors with a longjmp.
Bound the decoded size of an entry before allocating and decode one row at a time. Sweep under the write lock. Fail a merge on an entry that cannot be read. Leave the sidecar alone on discard history, the next sweep removes the orphans. Find sidecars by name when deleting from disk, since a lazy xmp may not exist yet. Copy a sidecar that cannot be moved on rename. Merge before deleting on local-copy reset and keep the copy if that fails. Store 16-bit PNG imports at 16 bit.
c96b172 to
367db65
Compare
First practical implementation of the sidecar approach proposed in #22226, with the external raster masks module as its first user.
The module used to reference a PFM/PNG by path inside a root folder set in preferences, so a mask did not follow the image when it was moved, duplicated or had its history pasted. The sidecar is a flat zip beside the XMP with the same duplicate suffix, one gray PNG per entry, named by the SHA-1 of its bytes and never modified. A history row stores only the entry name plus size, bit depth, origin and producer (
dt_dtdata_ref_t, inline in the module params), and the file is found by the same rule that finds the XMP. Readers ignore entries they do not know, writers keep them.The sidecar follows the XMP on rename, move, delete, local copies and history paste. Unreferenced entries are dropped only where darkroom undo can no longer restore them – darkroom leave, image switch, the lighttable compress job – and the lighttable discard deletes the file. Modules register a scanner from
init_globalso the sweep can read which entry their params reference. One gap, recorded indev-doc/dtdata_sidecar.md: an older build opening an image with a newer rasterfile params version loses the reference, because the history loader replaces newer params with defaults before the sweep runs, as it does for every module.Existing v1 edits keep working, and with "write sidecar files" set to never the old root-folder chooser remains.
libarchivebecomes a required dependency instead of only withUSE_AI; CI already had it, the MacPorts and nix lists are updated here.New strings use
_(). No new preference. Release note in its own commit. Written with AI assistance.