Pipeline improvements: brightness check after crop, alignment mandatory.

This commit is contained in:
Arnaud_Cayrol 2026-02-04 19:31:04 +01:00
parent 9554c6d2fb
commit 77e05d663e
5 changed files with 114 additions and 170 deletions

View file

@ -38,7 +38,6 @@
keep_intermediates: false,
},
alignment: {
enabled: true,
eye_y_position: 0.35,
inter_eye_distance: 0.30,
},
@ -117,7 +116,6 @@
keep_intermediates: false,
},
alignment: {
enabled: true,
eye_y_position: 0.35,
inter_eye_distance: 0.30,
},
@ -367,47 +365,42 @@
<div class="setting-section">
<div class="section-header">
<span class="section-title">Face Alignment</span>
<input
type="checkbox"
bind:checked={config.processing.alignment.enabled}
/>
<span class="setting-hint" style="font-style: italic; color: var(--text-secondary);">Always enabled</span>
</div>
{#if config.processing.alignment.enabled}
<div class="setting-row sub-setting">
<label>
<span class="setting-label">Eye Y Position</span>
<span class="setting-hint">Vertical position of eyes (% from top)</span>
</label>
<div class="setting-control">
<input
type="range"
bind:value={config.processing.alignment.eye_y_position}
min="0.2"
max="0.5"
step="0.01"
/>
<span class="value">{(config.processing.alignment.eye_y_position * 100).toFixed(0)}%</span>
</div>
<div class="setting-row sub-setting">
<label>
<span class="setting-label">Eye Y Position</span>
<span class="setting-hint">Vertical position of eyes (% from top)</span>
</label>
<div class="setting-control">
<input
type="range"
bind:value={config.processing.alignment.eye_y_position}
min="0.2"
max="0.5"
step="0.01"
/>
<span class="value">{(config.processing.alignment.eye_y_position * 100).toFixed(0)}%</span>
</div>
</div>
<div class="setting-row sub-setting">
<label>
<span class="setting-label">Inter-eye Distance</span>
<span class="setting-hint">Distance between eyes (% of width)</span>
</label>
<div class="setting-control">
<input
type="range"
bind:value={config.processing.alignment.inter_eye_distance}
min="0.2"
max="0.5"
step="0.01"
/>
<span class="value">{(config.processing.alignment.inter_eye_distance * 100).toFixed(0)}%</span>
</div>
<div class="setting-row sub-setting">
<label>
<span class="setting-label">Inter-eye Distance</span>
<span class="setting-hint">Distance between eyes (% of width)</span>
</label>
<div class="setting-control">
<input
type="range"
bind:value={config.processing.alignment.inter_eye_distance}
min="0.2"
max="0.5"
step="0.01"
/>
<span class="value">{(config.processing.alignment.inter_eye_distance * 100).toFixed(0)}%</span>
</div>
{/if}
</div>
</div>
</fieldset>
{:else if activeTab === 'output'}

View file

@ -298,11 +298,10 @@ impl EyeFilterConfig {
///
/// Aligns faces based on eye positions detected from facial landmarks,
/// ensuring consistent eye placement across all images for smoother timelapses.
///
/// Note: Face alignment is always enabled and is a core part of the pipeline.
#[derive(Debug, Clone, Serialize, Deserialize)]
pub struct AlignmentConfig {
/// Whether face alignment is enabled.
pub enabled: bool,
/// Target Y position for eyes as percentage from top (0.0-1.0).
/// Default 0.35 places eyes at 35% from the top.
pub eye_y_position: f32,
@ -315,7 +314,6 @@ pub struct AlignmentConfig {
impl Default for AlignmentConfig {
fn default() -> Self {
Self {
enabled: true,
eye_y_position: 0.35,
inter_eye_distance: 0.30,
}
@ -325,32 +323,30 @@ impl Default for AlignmentConfig {
impl AlignmentConfig {
/// Validate the configuration values.
pub fn validate(&self) -> Result<()> {
if self.enabled {
if self.eye_y_position <= 0.0 || self.eye_y_position >= 1.0 {
return Err(Error::Config(
"Alignment eye_y_position must be between 0.0 and 1.0 (exclusive)".to_string(),
));
}
if self.eye_y_position < 0.2 || self.eye_y_position > 0.5 {
return Err(Error::Config(
"Alignment eye_y_position should be between 0.2 and 0.5 for best results".to_string(),
));
}
if self.inter_eye_distance <= 0.0 {
return Err(Error::Config(
"Alignment inter_eye_distance must be greater than 0 to prevent division by zero".to_string(),
));
}
if self.inter_eye_distance >= 1.0 {
return Err(Error::Config(
"Alignment inter_eye_distance must be less than 1.0".to_string(),
));
}
if self.inter_eye_distance < 0.2 || self.inter_eye_distance > 0.5 {
return Err(Error::Config(
"Alignment inter_eye_distance should be between 0.2 and 0.5 for best results".to_string(),
));
}
if self.eye_y_position <= 0.0 || self.eye_y_position >= 1.0 {
return Err(Error::Config(
"Alignment eye_y_position must be between 0.0 and 1.0 (exclusive)".to_string(),
));
}
if self.eye_y_position < 0.2 || self.eye_y_position > 0.5 {
return Err(Error::Config(
"Alignment eye_y_position should be between 0.2 and 0.5 for best results".to_string(),
));
}
if self.inter_eye_distance <= 0.0 {
return Err(Error::Config(
"Alignment inter_eye_distance must be greater than 0 to prevent division by zero".to_string(),
));
}
if self.inter_eye_distance >= 1.0 {
return Err(Error::Config(
"Alignment inter_eye_distance must be less than 1.0".to_string(),
));
}
if self.inter_eye_distance < 0.2 || self.inter_eye_distance > 0.5 {
return Err(Error::Config(
"Alignment inter_eye_distance should be between 0.2 and 0.5 for best results".to_string(),
));
}
Ok(())
}

View file

@ -16,9 +16,9 @@
mod crop_utils;
mod debug_utils;
mod orientation;
pub mod steps;
mod traits;
mod types;
pub mod steps;
pub use crop_utils::{crop_face_with_intermediate, CropResult};
pub use debug_utils::draw_simple_text;
@ -57,14 +57,9 @@ pub enum PipelineResult {
debug_images: Vec<(String, DebugImage)>,
},
/// Processing error.
Error {
asset_id: String,
error: String,
},
Error { asset_id: String, error: String },
/// Cancelled by user.
Cancelled {
asset_id: String,
},
Cancelled { asset_id: String },
}
/// Processing pipeline that executes steps sequentially.
@ -75,7 +70,10 @@ pub struct Pipeline {
impl std::fmt::Debug for Pipeline {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("Pipeline")
.field("steps", &self.steps.iter().map(|s| s.id()).collect::<Vec<_>>())
.field(
"steps",
&self.steps.iter().map(|s| s.id()).collect::<Vec<_>>(),
)
.finish()
}
}
@ -100,12 +98,12 @@ impl Pipeline {
/// Pipeline order:
/// 1. FaceResolutionStep - Validate face size from Immich metadata (if enabled)
/// 2. DecodeImageStep - Load and orient the image (always)
/// 3. BrightnessStep - Filter by luminance (if enabled)
/// 4. CropFaceStep - Extract face region with padding (always)
/// 3. CropFaceStep - Extract face region with padding (always)
/// 4. BrightnessStep - Filter by luminance on cropped face (if enabled)
/// 5. HeadPoseStep - Filter non-frontal faces (if enabled)
/// 6. LandmarksStep - Detect 68 facial landmarks (if eye_filter or alignment enabled)
/// 6. LandmarksStep - Detect 68 facial landmarks (always)
/// 7. EyeFilterStep - Filter closed eyes by EAR (if enabled)
/// 8. AlignmentStep - Align face based on eye positions (if enabled)
/// 8. AlignmentStep - Align face based on eye positions (always)
/// 9. ResizeStep - Final resize to output size (always)
pub fn with_steps_from_config(config: &Config) -> Self {
use steps::*;
@ -120,36 +118,29 @@ impl Pipeline {
// Core: Always decode the image
pipeline.add_step(Box::new(DecodeImageStep));
// Optional: Brightness validation
// Core: Always crop the face
pipeline.add_step(Box::new(CropFaceStep));
// Optional: Brightness validation (on cropped face region)
if config.processing.brightness.enabled {
pipeline.add_step(Box::new(BrightnessStep));
}
// Core: Always crop the face
pipeline.add_step(Box::new(CropFaceStep));
// Optional: Head pose validation
if config.processing.head_pose.enabled {
pipeline.add_step(Box::new(HeadPoseStep));
}
// Landmarks are needed if eye_filter or alignment is enabled
let needs_landmarks = config.processing.eye_filter.enabled
|| config.processing.alignment.enabled;
if needs_landmarks {
pipeline.add_step(Box::new(LandmarksStep));
}
// Core: Always detect landmarks (required for alignment)
pipeline.add_step(Box::new(LandmarksStep));
// Optional: Eye filter (requires landmarks)
if config.processing.eye_filter.enabled {
pipeline.add_step(Box::new(EyeFilterStep));
}
// Optional: Alignment (requires landmarks)
if config.processing.alignment.enabled {
pipeline.add_step(Box::new(AlignmentStep));
}
// Core: Always align face based on eye positions
pipeline.add_step(Box::new(AlignmentStep));
// Core: Always resize to output size
pipeline.add_step(Box::new(ResizeStep));
@ -165,8 +156,8 @@ impl Pipeline {
let mut pipeline = Self::new();
pipeline.add_step(Box::new(FaceResolutionStep));
pipeline.add_step(Box::new(DecodeImageStep));
pipeline.add_step(Box::new(BrightnessStep));
pipeline.add_step(Box::new(CropFaceStep));
pipeline.add_step(Box::new(BrightnessStep));
pipeline.add_step(Box::new(HeadPoseStep));
pipeline.add_step(Box::new(LandmarksStep));
pipeline.add_step(Box::new(EyeFilterStep));
@ -219,7 +210,11 @@ impl Pipeline {
}
}
}
StepOutcome::Skip { mut ctx, reason, detail } => {
StepOutcome::Skip {
mut ctx,
reason,
detail,
} => {
// Update skip stats with the step ID as reason
skip_stats.increment(&reason);
@ -239,7 +234,8 @@ impl Pipeline {
);
// Collect and save debug images before returning
let debug_images: Vec<(String, DebugImage)> = ctx.debug_images.into_iter().collect();
let debug_images: Vec<(String, DebugImage)> =
ctx.debug_images.into_iter().collect();
if let Some(debug_base) = debug_dir {
save_debug_images(debug_base, &debug_images, &timestamp, &asset_id).await;
@ -261,7 +257,8 @@ impl Pipeline {
);
// Collect debug images from error context
let debug_images: Vec<(String, DebugImage)> = ctx.debug_images.into_iter().collect();
let debug_images: Vec<(String, DebugImage)> =
ctx.debug_images.into_iter().collect();
// Save debug images if enabled
if let Some(debug_base) = debug_dir {
@ -326,7 +323,13 @@ async fn save_debug_images(
let filename = format!("{}_{}.jpg", timestamp, asset_id)
.chars()
.map(|c| if c.is_alphanumeric() || c == '-' || c == '_' || c == '.' { c } else { '_' })
.map(|c| {
if c.is_alphanumeric() || c == '-' || c == '_' || c == '.' {
c
} else {
'_'
}
})
.collect::<String>();
let debug_path = step_dir.join(&filename);
@ -372,16 +375,17 @@ mod tests {
config.processing.brightness.enabled = false;
config.processing.head_pose.enabled = false;
config.processing.eye_filter.enabled = false;
config.processing.alignment.enabled = false;
let pipeline = Pipeline::with_steps_from_config(&config);
let ids = pipeline.step_ids();
// Only core steps should be present
// Core steps: decode, crop, landmarks, alignment, resize
assert!(ids.contains(&"decode"));
assert!(ids.contains(&"crop"));
assert!(ids.contains(&"landmarks"));
assert!(ids.contains(&"alignment"));
assert!(ids.contains(&"resize"));
assert_eq!(ids.len(), 3);
assert_eq!(ids.len(), 5);
}
#[test]
@ -391,17 +395,16 @@ mod tests {
config.processing.brightness.enabled = false;
config.processing.head_pose.enabled = false;
config.processing.eye_filter.enabled = true;
config.processing.alignment.enabled = false;
let pipeline = Pipeline::with_steps_from_config(&config);
let ids = pipeline.step_ids();
// Should include landmarks (dependency) and eye_filter
// Core steps (decode, crop, landmarks, alignment, resize) plus eye_filter
assert!(ids.contains(&"decode"));
assert!(ids.contains(&"crop"));
assert!(ids.contains(&"landmarks"));
assert!(ids.contains(&"eye_filter"));
assert!(ids.contains(&"alignment")); // Alignment is now always enabled
assert!(ids.contains(&"resize"));
assert!(!ids.contains(&"alignment"));
}
}

View file

@ -29,21 +29,18 @@ impl ProcessingStep for AlignmentStep {
}
async fn execute(&self, mut ctx: PipelineContext, config: &Config) -> StepOutcome {
// Skip if alignment is disabled
if !config.processing.alignment.enabled {
return StepOutcome::Continue(ctx);
}
// Get landmarks from previous step
// Get landmarks from previous step (required)
let landmarks: crate::pipeline::Landmarks = match ctx
.get_computed(computed_keys::LANDMARKS)
.and_then(|v| v.as_landmarks())
{
Some(l) => l.clone(),
None => {
// If landmarks aren't available, skip alignment but continue
tracing::warn!("Landmarks not available, skipping alignment");
return StepOutcome::Continue(ctx);
// Landmarks should always be available since LandmarksStep is mandatory
return StepOutcome::Error {
ctx,
error: "Landmarks not available for alignment - pipeline misconfigured".to_string(),
};
}
};
@ -176,38 +173,20 @@ mod tests {
}
#[tokio::test]
async fn test_disabled_skips_alignment() {
async fn test_no_landmarks_errors() {
let step = AlignmentStep;
let ctx = make_test_ctx();
let mut config = Config::default();
config.processing.alignment.enabled = false;
// Create a dummy image
let img = DynamicImage::ImageRgb8(RgbImage::new(100, 100));
let ctx = ctx.with_image(img);
match step.execute(ctx, &config).await {
StepOutcome::Continue(new_ctx) => {
assert!(new_ctx.image.is_some());
}
other => panic!("Expected Continue when disabled, got {:?}", other),
}
}
#[tokio::test]
async fn test_no_landmarks_continues() {
let step = AlignmentStep;
let ctx = make_test_ctx();
let mut config = Config::default();
config.processing.alignment.enabled = true;
let config = Config::default();
// Create a dummy image but no landmarks
let img = DynamicImage::ImageRgb8(RgbImage::new(100, 100));
let ctx = ctx.with_image(img);
match step.execute(ctx, &config).await {
StepOutcome::Continue(_) => {} // Expected - continues without alignment
other => panic!("Expected Continue without landmarks, got {:?}", other),
StepOutcome::Error { error, .. } => {
assert!(error.contains("Landmarks not available"));
}
other => panic!("Expected Error without landmarks, got {:?}", other),
}
}
}

View file

@ -28,15 +28,8 @@ impl ProcessingStep for LandmarksStep {
"Landmarks"
}
async fn execute(&self, mut ctx: PipelineContext, config: &Config) -> StepOutcome {
// We always need landmarks if alignment is enabled, even if eye filter is disabled
let need_landmarks =
config.processing.alignment.enabled || config.processing.eye_filter.enabled;
if !need_landmarks {
return StepOutcome::Continue(ctx);
}
async fn execute(&self, mut ctx: PipelineContext, _config: &Config) -> StepOutcome {
// Landmarks are always needed for face alignment
let image = match ctx.require_image("landmark detection") {
Ok(img) => img,
Err(e) => return StepOutcome::Error { ctx, error: e },
@ -114,7 +107,6 @@ impl ProcessingStep for LandmarksStep {
mod tests {
use super::*;
use crate::immich_api::FaceData;
use image::{DynamicImage, RgbImage};
fn make_test_ctx() -> PipelineContext {
let face_data = FaceData {
@ -128,30 +120,11 @@ mod tests {
PipelineContext::new("test".to_string(), "2024-01-01".to_string(), face_data)
}
#[tokio::test]
async fn test_disabled_skips_check() {
let step = LandmarksStep;
let ctx = make_test_ctx();
let mut config = Config::default();
config.processing.alignment.enabled = false;
config.processing.eye_filter.enabled = false;
// Create a dummy image
let img = DynamicImage::ImageRgb8(RgbImage::new(100, 100));
let ctx = ctx.with_image(img);
match step.execute(ctx, &config).await {
StepOutcome::Continue(_) => {} // Expected
other => panic!("Expected Continue when disabled, got {:?}", other),
}
}
#[tokio::test]
async fn test_no_image_error() {
let step = LandmarksStep;
let ctx = make_test_ctx();
let mut config = Config::default();
config.processing.alignment.enabled = true;
let config = Config::default();
match step.execute(ctx, &config).await {
StepOutcome::Error { error, .. } => {