JXL export#82
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds JPEG XL encoding/transcoding and refactors SaveToFile to accept ISettings; extends ExifTool metadata helpers; updates all image/video save call sites and adds tests and a "JPEG XL" UI menu item. ChangesJPEG XL Format Support and API Unification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
PhotoLocatorTest/PictureFileFormats/JpegXlFileFormatHandlerTest.cs (1)
59-70: ⚡ Quick winDispose the decoder process and assert its exit code.
The
decoderprocess (and its redirectedStandardErrorstream) is never disposed. More importantly, the exit code isn't checked: ifdjxl.exefails,restoredJpgwon't exist and Line 69 throws aFileNotFoundException, masking the real failure behind a confusing error instead of a clear assertion.♻️ Proposed change
- var decoder = Process.Start(new ProcessStartInfo + using var decoder = Process.Start(new ProcessStartInfo { FileName = DecoderPath, Arguments = $"\"{targetJxl}\" \"{restoredJpg}\"", CreateNoWindow = true, RedirectStandardError = true, }) ?? throw new InvalidOperationException("Failed to start decoder process"); Debug.WriteLine(decoder.StandardError.ReadToEnd()); decoder.WaitForExit(); + Assert.AreEqual(0, decoder.ExitCode, "Decoder process failed");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@PhotoLocatorTest/PictureFileFormats/JpegXlFileFormatHandlerTest.cs` around lines 59 - 70, The decoder Process started via Process.Start (using DecoderPath, targetJxl, restoredJpg) is not disposed and its exit code is not checked; wrap the Process (and its redirected StandardError) in a using (or call Dispose) to ensure streams are closed, call decoder.WaitForExit(), assert decoder.ExitCode == 0 (failing the test with a clear message including the decoded StandardError content if non-zero), and only then read restoredJpg bytes and compare them to sourceBytes so a decoder failure produces a clear test assertion instead of a FileNotFoundException.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@PhotoLocator/ImageTransformCommands.cs`:
- Around line 236-245: The inline call to JpegXlFileFormatHandler.TranscodeToJxl
blocks the UI/callback; wrap the TranscodeToJxl invocation in the same offload
pattern used for other saves (e.g., run it inside Task.Run with the cancellation
token and await it) or make an async wrapper that accepts ct, so the branch for
targetType == "jxl" uses an awaited Task.Run(() =>
JpegXlFileFormatHandler.TranscodeToJxl(item.FullPath, targetFileName, ...), ct)
(or call the new async method) and propagate exceptions and cancellation
consistently with the LoadImageWithMetadataAsync /
GeneralFileFormatHandler.SaveToFile path while still applying
ExifHandler.ResetOrientation and _mainViewModel.Settings as needed.
In `@PhotoLocator/Metadata/ExifTool.cs`:
- Around line 56-59: The code in ExifTool.SetMetadataAsync currently
concatenates metadata.CameraModel into the ProcessStartInfo.Arguments string via
sb.Append("-Model=\"...\""), which allows embedded quotes to break argument
parsing; change the invocation to populate ProcessStartInfo.ArgumentList (or
otherwise properly escape the value) instead of building a single Arguments
string—replace the sb.Append usage that adds cameraModel with adding two
argument entries (the "-Model=" switch and the cameraModel value) to
ProcessStartInfo.ArgumentList or call ArgumentList.Add($"-Model={cameraModel}")
after ensuring proper quoting, so metadata.CameraModel is passed as a single
safe argument to ExifTool.
- Around line 74-76: SetMetadataAsync in ExifTool.cs currently writes the
FNumber tag using the lens aperture from ExifHandler.LensApertureQuery1/Query2,
but DecodeMetadata only reads "ApertureValue", so aperture can be lost; update
SetMetadataAsync to also write the ApertureValue tag (or write ApertureValue
instead of FNumber), and/or update DecodeMetadata to accept "FNumber" when
parsing aperture; reference the symbols SetMetadataAsync, DecodeMetadata,
ExifHandler.LensApertureQuery1, ExifHandler.LensApertureQuery2 so you add
writing of "-ApertureValue=..." (or both "-FNumber=..." and
"-ApertureValue=...") to ensure read/write symmetry.
In `@PhotoLocator/PictureFileFormats/JpegXlFileFormatHandler.cs`:
- Around line 53-68: The code can leave cjxl.exe running when
process.WaitForExit(60000) times out or when outputTask.Wait(1000, ct) is
cancelled; update the try/catch/finally around the Process (variable process)
and outputTask to observe the CancellationToken ct and to kill the process (and
its tree) before rethrowing: if process.WaitForExit timed out or
ct.IsCancellationRequested, call process.Kill(true) (or Kill with
recursive=true) and wait for exit, and when catching exceptions or cancellation
before rethrow ensure you kill the process and then propagate the exception;
also wire ct into waiting by using a CancellationTokenRegistration to call
process.Kill(true) if ct is cancelled so outputTask.Wait/WaitForExit respect ct.
Ensure the IOException thrown (the existing throw new IOException("Codec failed
with: " + output, ex)) still includes output after the process has been
terminated.
- Around line 34-35: In JpegXlFileFormatHandler.SaveToFile remove the blocking
.Wait() call and await ExifTool.SetMetadataAsync instead: change SaveToFile to
an async method (e.g., Task or Task<T> as appropriate), replace the .Wait()
usage with await ExifTool.SetMetadataAsync(targetPath, targetPath, metadata,
settings.ExifToolPath, CancellationToken.None), and propagate the async change
to callers so exceptions are not wrapped in AggregateException and you avoid
deadlocks on the WPF SynchronizationContext.
In `@PhotoLocator/VideoTransformCommands.cs`:
- Around line 1048-1050: The conditional that chooses between
process.GetResult16() and process.GetResult8() omits the ".jxl" extension,
causing averaged exports to drop to 8-bit; update the
Path.GetExtension(outFileName).ToLowerInvariant() check in the
GeneralFileFormatHandler.SaveToFile call to include ".jxl" alongside ".png",
".tif", ".tiff", and ".jxr" so that when process.Supports16BitResult() is true
and the extension is ".jxl" it uses process.GetResult16(); no other changes to
SaveToFile, CreateImageMetadata, or the _mainViewModel.Settings usage are
needed.
---
Nitpick comments:
In `@PhotoLocatorTest/PictureFileFormats/JpegXlFileFormatHandlerTest.cs`:
- Around line 59-70: The decoder Process started via Process.Start (using
DecoderPath, targetJxl, restoredJpg) is not disposed and its exit code is not
checked; wrap the Process (and its redirected StandardError) in a using (or call
Dispose) to ensure streams are closed, call decoder.WaitForExit(), assert
decoder.ExitCode == 0 (failing the test with a clear message including the
decoded StandardError content if non-zero), and only then read restoredJpg bytes
and compare them to sourceBytes so a decoder failure produces a clear test
assertion instead of a FileNotFoundException.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f13081eb-adc5-46fa-8091-16f354518572
📒 Files selected for processing (13)
PhotoLocator/ImageTransformCommands.csPhotoLocator/MainWindow.xamlPhotoLocator/Metadata/ExifHandler.csPhotoLocator/Metadata/ExifTool.csPhotoLocator/PictureFileFormats/GeneralFileFormatHandler.csPhotoLocator/PictureFileFormats/JpegTransformations.csPhotoLocator/PictureFileFormats/JpegXlFileFormatHandler.csPhotoLocator/PictureFileFormats/JpegliEncoder.csPhotoLocator/Settings/RegistrySettings.csPhotoLocator/VideoTransformCommands.csPhotoLocatorTest/Metadata/ExifHandlerTest.csPhotoLocatorTest/PictureFileFormats/JpegXlFileFormatHandlerTest.csPhotoLocatorTest/PictureFileFormats/JpegliEncoderTest.cs
| if (metadata is not null && !string.IsNullOrEmpty(settings?.ExifToolPath)) | ||
| ExifTool.SetMetadataAsync(targetPath, targetPath, metadata, settings.ExifToolPath, CancellationToken.None).Wait(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the handler file and show the relevant region
ls -la
fd -i "JpegXlFileFormatHandler.cs" .
echo "---- File context around handler ----"
FILE=$(fd -i "JpegXlFileFormatHandler.cs" . -a | head -n 1)
echo "Using: $FILE"
sed -n '1,120p' "$FILE" | nl -ba | sed -n '25,80p'
echo "---- Exact lines 34-60 (best effort) ----"
sed -n '20,60p' "$FILE" | nl -ba
# Find ExifTool and SetMetadataAsync definition/overloads
echo "---- Searching for SetMetadataAsync ----"
rg -n "SetMetadataAsync" -S . -g'*.cs'
# Show the ExifTool.SetMetadataAsync implementation(s)
echo "---- Showing implementation(s) ----"
rg -n "Task\s+SetMetadataAsync|SetMetadataAsync\s*\(" -S . -g'*.cs' | head -n 50
# Attempt to open the first implementation file(s)
# (We use sed ranges after locating filenames.)
IMPL_FILES=$(rg -l "SetMetadataAsync" -S . -g'*.cs' | tr '\n' ' ')
echo "---- Implementation files (top 10) ----"
for f in $(rg -l "SetMetadataAsync" -S . -g'*.cs' | head -n 10); do echo "$f"; done
# Show likely ExifTool class file(s)
for f in $(rg -l "class\s+ExifTool" -S . -g'*.cs' | head -n 5); do
echo "---- ExifTool class snippet: $f ----"
sed -n '1,220p' "$f" | nl -ba | head -n 220
done
# Find call sites of ExifTool.SetMetadataAsync that may also use .Wait() or .Result
echo "---- Call sites using .Wait() / .Result ----"
rg -n "SetMetadataAsync\([^)]*\)\.(Wait|Result)" -S . -g'*.cs'
rg -n "\.Wait\(\)\s*;" -S . -g'*.cs' | head -n 50
# Find where JpegXlFileFormatHandler is invoked from the WPF save flow (async vs sync)
echo "---- Searching for JpegXlFileFormatHandler usage ----"
rg -n "JpegXlFileFormatHandler" -S . -g'*.cs'Repository: meesoft/PhotoLocator
Length of output: 2051
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="PhotoLocator/PictureFileFormats/JpegXlFileFormatHandler.cs"
echo "---- Handler snippet (lines 1-120) ----"
sed -n '1,120p' "$FILE" | head -n 120
echo "---- Handler snippet (lines 30-55) ----"
sed -n '30,55p' "$FILE"
echo "---- Search for SetMetadataAsync in repo ----"
rg -n "SetMetadataAsync" PhotoLocator -S --glob='*.cs'
echo "---- Search for SetMetadataAsync(...).Wait() ----"
rg -n "SetMetadataAsync\s*\([^)]*\)\s*\.\s*Wait\s*\(\s*\)" PhotoLocator -S --glob='*.cs' || true
echo "---- Search for ExifTool class ----"
rg -n "class\s+ExifTool\b" PhotoLocator -S --glob='*.cs'
echo "---- Show ExifTool.* implementation(s) ----"
rg -n "Task\s+SetMetadataAsync|SetMetadataAsync\s*\(" PhotoLocator -S --glob='*.cs'
# List candidate implementation files and show surrounding code
IMPL_FILES=$(rg -l "SetMetadataAsync" PhotoLocator -S --glob='*.cs' | head -n 20)
echo "---- Candidate implementation files (first 20) ----"
echo "$IMPL_FILES"
for f in $IMPL_FILES; do
echo "==== $f (first 220 lines) ===="
sed -n '1,220p' "$f"
doneRepository: meesoft/PhotoLocator
Length of output: 20589
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "JpegXlFileFormatHandler\.SaveToFile" -S . --glob='*.cs'
echo "----"
rg -n "SaveToFile\s*\(" PhotoLocator -S --glob='*.cs' | head -n 200
echo "----"
# Try to find typical WPF command/save entry points mentioning "Save" and "PictureFileFormats"
rg -n "Save.*(Async|Task)|Dispatcher|SyncContext|SynchronizationContext" PhotoLocator -S --glob='*.cs' | head -n 200
echo "----"
# Search for ExifTool usage and any async caller patterns
rg -n "ExifTool\.SetMetadataAsync" -S . --glob='*.cs'Repository: meesoft/PhotoLocator
Length of output: 7226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILELIST=(
PhotoLocator/PictureFileFormats/GeneralFileFormatHandler.cs
PhotoLocator/MainViewModel.cs
PhotoLocator/ImageTransformCommands.cs
PhotoLocator/VideoTransformCommands.cs
PhotoLocator/PictureItemViewModel.cs
PhotoLocator/Helpers/ExceptionHandler.cs
PhotoLocator/App.xaml.cs
)
for f in "${FILELIST[@]}"; do
echo "==== $f (head 220) ===="
sed -n '1,220p' "$f" | nl -ba | head -n 220
done
# Print focused ranges around the identified call sites
echo "==== Focus: GeneralFileFormatHandler.cs around line ~60-120 ===="
sed -n '40,120p' PhotoLocator/PictureFileFormats/GeneralFileFormatHandler.cs | nl -ba
echo "==== Focus: MainViewModel.cs around line ~430-490 ===="
sed -n '420,500p' PhotoLocator/MainViewModel.cs | nl -ba
echo "==== Focus: ImageTransformCommands.cs around line ~60-280 ===="
sed -n '60,280p' PhotoLocator/ImageTransformCommands.cs | nl -ba
echo "==== Focus: VideoTransformCommands.cs around lines ~1030-1120 ===="
sed -n '1020,1125p' PhotoLocator/VideoTransformCommands.cs | nl -ba
echo "==== Focus: PictureItemViewModel.cs around line ~330-420 (SaveGeoTagAsync mention) ===="
sed -n '300,420p' PhotoLocator/PictureItemViewModel.cs | nl -ba
echo "==== Focus: ExceptionHandler.cs around line ~1-120 ===="
sed -n '1,120p' PhotoLocator/Helpers/ExceptionHandler.cs | nl -ba
echo "==== Focus: App.xaml.cs around dispatcher exception handler ===="
sed -n '1,120p' PhotoLocator/App.xaml.cs | nl -baRepository: meesoft/PhotoLocator
Length of output: 207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
show_range () {
local file="$1"
local start="$2"
local end="$3"
echo "==== $file ($start-$end) ===="
sed -n "${start},${end}p" "$file" | cat -n
}
show_range PhotoLocator/PictureFileFormats/GeneralFileFormatHandler.cs 1 140
show_range PhotoLocator/PictureFileFormats/GeneralFileFormatHandler.cs 140 220
show_range PhotoLocator/MainViewModel.cs 420 560
show_range PhotoLocator/ImageTransformCommands.cs 1 260
show_range PhotoLocator/ImageTransformCommands.cs 260 340
show_range PhotoLocator/VideoTransformCommands.cs 1000 1125
show_range PhotoLocator/PictureItemViewModel.cs 300 430
show_range PhotoLocator/Helpers/ExceptionHandler.cs 1 120
show_range PhotoLocator/App.xaml.cs 1 120Repository: meesoft/PhotoLocator
Length of output: 46048
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show more context around the clipboard image paste loop in MainViewModel
# (Search for the clipboard image / Clipboard.ContainsImage() block)
rg -n "Clipboard\.ContainsImage\(\)" PhotoLocator/MainViewModel.cs --glob='*.cs'
# Print a wider window around that match
MATCH_LINE=$(rg -n "Clipboard\.ContainsImage\(\)" PhotoLocator/MainViewModel.cs --glob='*.cs' | head -n1 | cut -d: -f1)
echo "Match at line: $MATCH_LINE"
start=$((MATCH_LINE-60))
end=$((MATCH_LINE+80))
sed -n "${start},${end}p" PhotoLocator/MainViewModel.cs | cat -n
# Inspect VideoTransformCommands area around a method that calls GeneralFileFormatHandler.SaveToFile
# Find the enclosing command/delegate for the snippet lines
rg -n "GeneralFileFormatHandler\.SaveToFile\(" PhotoLocator/VideoTransformCommands.cs --glob='*.cs'
# For first occurrence near earlier snippet, show surrounding context
# We'll just show around each occurrence line.
rg -n "GeneralFileFormatHandler\.SaveToFile\(" PhotoLocator/VideoTransformCommands.cs --glob='*.cs' | while read -r line; do
ln=$(echo "$line" | cut -d: -f1)
file=$(echo "$line" | cut -d: -f1)
: # noop
done
# Actually extract for each match: line number and show window
while IFS=: read -r file ln rest; do
echo "---- $file match at $ln ----"
s=$((ln-60)); e=$((ln+80))
sed -n "${s},${e}p" "$file" | cat -n | head -n 220
done < <(rg -n "GeneralFileFormatHandler\.SaveToFile\(" PhotoLocator/VideoTransformCommands.cs --glob='*.cs' | head -n 10)
# Search exception handling patterns for AggregateException / Wait usage
rg -n "AggregateException" PhotoLocator --glob='*.cs'
rg -n "\.Wait\(\)" PhotoLocator --glob='*.cs' | head -n 50
# Find any callers that don't unwrap AggregateException from tasks
rg -n "await .*SaveToFile|SaveToFile\(" PhotoLocator --glob='*.cs' | head -n 100Repository: meesoft/PhotoLocator
Length of output: 7508
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find the command/method in VideoTransformCommands that contains the SaveToFile call.
# We'll print surrounding larger region and then locate its entry point.
FILE="PhotoLocator/VideoTransformCommands.cs"
# show matches for SaveToFile
echo "---- SaveToFile call sites in VideoTransformCommands.cs ----"
rg -n "GeneralFileFormatHandler\.SaveToFile\(" "$FILE" --glob='*.cs'
# Pick first match and dump a bigger region above it
FIRST_LN=$(rg -n "GeneralFileFormatHandler\.SaveToFile\(" "$FILE" --glob='*.cs' | head -n1 | cut -d: -f1)
echo "First SaveToFile line: $FIRST_LN"
START=$((FIRST_LN-220))
END=$((FIRST_LN+120))
echo "---- Context $START-$END ----"
sed -n "${START},${END}p" "$FILE" | cat -n
# 2) Look for wrapping Task.Run/await patterns around that processing delegate.
echo "---- Search for Task.Run / RunProcessWithProgressBar in VideoTransformCommands.cs ----"
rg -n "RunProcessWithProgressBarAsync|Task\.Run\(|Dispatcher\.|await Task\.Run" "$FILE" --glob='*.cs'
# 3) Find where output file extension is determined for image sequence/video/time-slice processing.
# Search for ".jxl" usage.
echo "---- Search for jxl in VideoTransformCommands.cs ----"
rg -n "\"?\.jxl|jxl\"" "$FILE" --glob='*.cs'
# 4) Find any other direct SaveToFile calls (not inside Task.Run) in repo.
echo "---- Find SaveToFile invocations not wrapped in Task.Run ----"
# Heuristic: show lines with SaveToFile( and look at nearest few lines above for Task.Run.
# We'll just list all SaveToFile calls with 20 lines context.
rg -n "GeneralFileFormatHandler\.SaveToFile\(" PhotoLocator --glob='*.cs' -C 2
# 5) Determine if any call to JpegXlFileFormatHandler.SaveToFile happens outside Task.Run.
echo "---- Calls to JpegXlFileFormatHandler.SaveToFile ----"
rg -n "JpegXlFileFormatHandler\.SaveToFile\(" PhotoLocator --glob='*.cs' -C 2Repository: meesoft/PhotoLocator
Length of output: 21394
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- GeneralFileFormatHandler.SaveToFile call sites (with small context) ----"
rg -n "GeneralFileFormatHandler\.SaveToFile\(" PhotoLocator --glob='*.cs' -C 3
echo "---- Any direct JpegXlFileFormatHandler.SaveToFile call sites (with context) ----"
rg -n "JpegXlFileFormatHandler\.SaveToFile\(" PhotoLocator --glob='*.cs' -C 3
echo "---- Call sites of JpegXlFileFormatHandler.SaveToFile within Task.Run bodies (heuristic) ----"
rg -n "Task\.Run\([^)]*\)[\s\S]*JpegXlFileFormatHandler\.SaveToFile" PhotoLocator --glob='*.cs' || true
echo "---- Identify if any SaveToFile call happens outside Task.Run ----"
# Heuristic: list SaveToFile call lines and 5 lines above, then user can see if Task.Run is near.
rg -n "GeneralFileFormatHandler\.SaveToFile\(" PhotoLocator --glob='*.cs' | cut -d: -f1 | sort -u | while read -r file; do
:
doneRepository: meesoft/PhotoLocator
Length of output: 8668
Remove the .Wait() on ExifTool.SetMetadataAsync in JpegXlFileFormatHandler.SaveToFile.
SaveToFilecurrently doesExifTool.SetMetadataAsync(...).Wait();whileSetMetadataAsync/RunExifToolAsyncuseawaitwithoutConfigureAwait(false), so blocking on it under a WPFSynchronizationContextcan deadlock.- Blocking also wraps failures (e.g.,
UserMessageException/IOException) inAggregateException.
if (metadata is not null && !string.IsNullOrEmpty(settings?.ExifToolPath)) ExifTool.SetMetadataAsync(...).Wait();
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@PhotoLocator/PictureFileFormats/JpegXlFileFormatHandler.cs` around lines 34 -
35, In JpegXlFileFormatHandler.SaveToFile remove the blocking .Wait() call and
await ExifTool.SetMetadataAsync instead: change SaveToFile to an async method
(e.g., Task or Task<T> as appropriate), replace the .Wait() usage with await
ExifTool.SetMetadataAsync(targetPath, targetPath, metadata,
settings.ExifToolPath, CancellationToken.None), and propagate the async change
to callers so exceptions are not wrapped in AggregateException and you avoid
deadlocks on the WPF SynchronizationContext.
Summary by CodeRabbit
New Features
Bug Fixes
Tests