From 5ec756e3a959a7fa7eb5bc043189f52a63c9fe6a Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Tue, 10 Nov 2026 14:38:40 +1100 Subject: [PATCH 1/2] fix(preview): draw the processed frame at the shape the encode will have (#149) The comparison drew both sides at the source's display aspect, so any pass that reshaped the frame (resize, borders, crop, rotate, square pixels, a forced DAR) previewed distorted. A 720x576 PAL 4:3 capture resized to 1440x1080 and bordered out to 1920x1080 came out squashed into 4:3. - Worker: ProcessingPipeline::output_aspect is the single aspect decision, used by the encode's setsar/setdar and reported by the preview on its PREVIEW_INFO line as sar=N/D or dar=N/D. - App: PreviewFrameInfo parses it. The processed aspect is stored with the processed image on the QueueItem. BeforeAfterComparisonWidget takes a separate before/after aspect, and 1:1 is measured against the processed one. - The preview job now carries inputSar (via the shared FieldOrderDetector.normalizeSar). Without it {{SOURCE_SAR}} fell back to 1 and square-pixel fitting previewed a 1350-wide picture where the encode makes 1440. Tests: worker unit tests, test_185, preview_frame_info_test, preview_aspect_test, before_after_comparison_test, and a heavy preview group in integration_aspect_ratio_test comparing the preview's picture extent with the encode's. --- CLAUDE.md | 26 +++ app/lib/models/preview_frame_info.dart | 61 ++++++- app/lib/models/queue_item.dart | 10 ++ app/lib/services/field_order_detector.dart | 16 +- app/lib/services/preview_generator.dart | 9 ++ app/lib/viewmodels/main_viewmodel.dart | 19 +++ app/lib/views/preview_panel.dart | 39 +++-- app/lib/widgets/before_after_comparison.dart | 33 +++- app/test/before_after_comparison_test.dart | 58 ++++++- app/test/integration_aspect_ratio_test.dart | 161 ++++++++++++++++++- app/test/preview_aspect_test.dart | 70 ++++++++ app/test/preview_frame_info_test.dart | 49 ++++++ worker/src/models/crop_resize_parameters.rs | 28 ++++ worker/src/models/processing_pipeline.rs | 58 +++++++ worker/src/pipeline_executor.rs | 86 ++++++++-- worker/tests/filter_integration_test.rs | 64 ++++++++ 16 files changed, 736 insertions(+), 51 deletions(-) create mode 100644 app/test/preview_aspect_test.dart diff --git a/CLAUDE.md b/CLAUDE.md index fac29752..4023f345 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -835,6 +835,32 @@ Three things decide the shape of the output, and they live in different places: else so a typo falls back to the source's own aspect rather than reaching ffmpeg as a broken filter argument. +**The preview has to show the same shape (issue #149).** The preview PNG carries +no aspect, so the app used to draw the processed frame at the **source's** +display aspect. Anything a pass reshaped (a 720x576 4:3 source resized and +padded to 1920x1080, say) came out squashed. Three invariants now hold: + +- **One decision, two consumers.** `ProcessingPipeline::output_aspect(input_sar)` + (rotation-adjusted SAR → `aspect_declaration`) is used by the encode's + `setsar`/`setdar` **and** by the preview. The preview reports it on its + `PREVIEW_INFO:` stderr line as `sar=N/D` or `dar=N/D` (nothing for square + pixels). `PreviewFrameInfo.displayAspectFor` turns that plus the PNG's size into + the shape to draw. +- **Each side has its own aspect.** `BeforeAfterComparisonWidget` takes + `beforeAspectRatio` (source) and `afterAspectRatio` (processed). 1:1 is + measured against the processed one. The processed aspect is stored on the + `QueueItem` next to the image (`processedPreviewAspect`), not read from + `previewFrameInfo`, which goes null as soon as the scrubber moves. +- **The preview job must carry `inputSar`.** `{{SOURCE_SAR}}` falls back to 1 + without it, so square-pixel fitting previewed a different frame from the + one encoded (1350 vs 1440 wide in the #149 setup). `PreviewGenerator` reads it + through the same `FieldOrderDetector.normalizeSar` as the encode job. + +Covered by `test_185` (Rust), `preview_frame_info_test`, `preview_aspect_test`, +`before_after_comparison_test`, and the heavy `preview aspect (issue #149)` +group in `integration_aspect_ratio_test.dart`, which compares the preview's +picture extent with the encode's. + ### Borders (issue #86) Two ways to add bars, sharing one step at the **very end of both templates** diff --git a/app/lib/models/preview_frame_info.dart b/app/lib/models/preview_frame_info.dart index 5f3f2342..e6cfc24c 100644 --- a/app/lib/models/preview_frame_info.dart +++ b/app/lib/models/preview_frame_info.dart @@ -1,25 +1,46 @@ +import 'dart:ui' show Size; + /// What the worker says it rendered for a preview: its `PREVIEW_INFO:` line. /// /// A source frame can have several output frames — two under a ×2 frame rate /// or double-rate deinterlacing, two or three at 24 → 60. [subFrame] is which /// of them this is, and [generated] whether it was made by interpolation /// rather than taken from a source frame (issue #129). +/// +/// The line also carries the aspect the encode will declare for the frame — +/// `sar=16/15` or `dar=16/9`, absent for square pixels — because the PNG +/// carries none (issue #149). class PreviewFrameInfo { final int frame; final int subFrame; final int subFrames; final bool generated; + /// The sample aspect ratio the encode stamps on the output, or null for + /// square pixels. + final double? outputSar; + + /// A forced display aspect the encode stamps instead of a SAR, or null. + final double? outputDar; + const PreviewFrameInfo({ required this.frame, this.subFrame = 0, this.subFrames = 1, this.generated = false, + this.outputSar, + this.outputDar, }); /// Whether there is more than one output frame to step through. bool get hasSubFrames => subFrames > 1; + /// The shape the rendered frame has on screen, given its stored size: the + /// display aspect the encode will have. A worker that reports no aspect + /// reads as square pixels — the frame's own shape. + double displayAspectFor(Size pixels) => + outputDar ?? pixels.width / pixels.height * (outputSar ?? 1.0); + /// The last `PREVIEW_INFO:` line in a worker's stderr, or null if there is /// none. A worker from before sub-frames reports only `frame=`, which reads /// as a single real frame. @@ -28,20 +49,46 @@ class PreviewFrameInfo { .split('\n') .where((l) => l.trimLeft().startsWith('PREVIEW_INFO:')); if (lines.isEmpty) return null; - final fields = {}; + final fields = {}; for (final pair in lines.last.trim().substring('PREVIEW_INFO:'.length).split(' ')) { final kv = pair.split('='); - final value = kv.length == 2 ? int.tryParse(kv[1]) : null; - if (value != null) fields[kv[0]] = value; + if (kv.length == 2) fields[kv[0]] = kv[1]; } - final frame = fields['frame']; + int? integer(String key) { + final v = fields[key]; + return v == null ? null : int.tryParse(v); + } + + final frame = integer('frame'); if (frame == null) return null; - final subFrames = (fields['sub_frames'] ?? 1).clamp(1, 1 << 20); + final subFrames = (integer('sub_frames') ?? 1).clamp(1, 1 << 20); return PreviewFrameInfo( frame: frame, - subFrame: (fields['sub_frame'] ?? 0).clamp(0, subFrames - 1), + subFrame: (integer('sub_frame') ?? 0).clamp(0, subFrames - 1), subFrames: subFrames, - generated: fields['generated'] == 1, + generated: integer('generated') == 1, + outputSar: parseRatio(fields['sar']), + outputDar: parseRatio(fields['dar']), ); } + + /// A ratio written `16/9`, `16:9` or `1.7778`, or null for anything that is + /// not a finite positive number — so a malformed token falls back to the + /// frame's own shape instead of drawing it zero-wide. + static double? parseRatio(String? text) { + if (text == null) return null; + final t = text.trim(); + if (t.isEmpty) return null; + final parts = t.split(RegExp('[:/]')); + double? ratio; + if (parts.length == 2) { + final num = double.tryParse(parts[0].trim()); + final den = double.tryParse(parts[1].trim()); + if (num == null || den == null || den == 0) return null; + ratio = num / den; + } else if (parts.length == 1) { + ratio = double.tryParse(t); + } + return ratio != null && ratio.isFinite && ratio > 0 ? ratio : null; + } } diff --git a/app/lib/models/queue_item.dart b/app/lib/models/queue_item.dart index dcfddb09..fc330308 100644 --- a/app/lib/models/queue_item.dart +++ b/app/lib/models/queue_item.dart @@ -69,6 +69,12 @@ class QueueItem { double scrubberPosition; Uint8List? currentFrame; Uint8List? processedPreview; + + /// The display aspect [processedPreview] is drawn at: the shape the encode + /// will have, from the aspect the worker reported for it. Kept with the + /// image rather than read from the frame info, which is dropped as soon as + /// the scrubber moves while this image is still on screen (issue #149). + double? processedPreviewAspect; double timelineZoom; double timelineViewStart; @@ -96,6 +102,7 @@ class QueueItem { this.scrubberPosition = 0.0, this.currentFrame, this.processedPreview, + this.processedPreviewAspect, this.timelineZoom = 1.0, this.timelineViewStart = 0.0, this.thumbnailRangeStart = 0.0, @@ -182,6 +189,7 @@ class QueueItem { double? scrubberPosition, Uint8List? currentFrame, Uint8List? processedPreview, + double? processedPreviewAspect, double? timelineZoom, double? timelineViewStart, double? thumbnailRangeStart, @@ -200,6 +208,8 @@ class QueueItem { scrubberPosition: scrubberPosition ?? this.scrubberPosition, currentFrame: currentFrame ?? this.currentFrame, processedPreview: processedPreview ?? this.processedPreview, + processedPreviewAspect: + processedPreviewAspect ?? this.processedPreviewAspect, timelineZoom: timelineZoom ?? this.timelineZoom, timelineViewStart: timelineViewStart ?? this.timelineViewStart, thumbnailRangeStart: thumbnailRangeStart ?? this.thumbnailRangeStart, diff --git a/app/lib/services/field_order_detector.dart b/app/lib/services/field_order_detector.dart index afa0288c..8f20ae1a 100644 --- a/app/lib/services/field_order_detector.dart +++ b/app/lib/services/field_order_detector.dart @@ -91,8 +91,7 @@ class FieldOrderDetector { final pixelFormat = videoStream['pix_fmt'] as String?; // Extract sample aspect ratio (SAR) — null if square pixels or unavailable - final rawSar = videoStream['sample_aspect_ratio'] as String?; - final sar = (rawSar != null && rawSar != 'N/A' && rawSar != '1:1') ? rawSar : null; + final sar = normalizeSar(videoStream['sample_aspect_ratio'] as String?); // Get metadata field order for context final metadataFieldOrder = videoStream['field_order'] as String?; @@ -447,6 +446,19 @@ class FieldOrderDetector { // UTILITIES // ============================================================================ + /// ffprobe's `sample_aspect_ratio` as the job carries it: null for square + /// pixels or when there is none to read (`N/A`, `1:1`, and `0:1`, which is + /// ffprobe's "unknown"). The encode and the preview jobs both go through + /// this, so the preview fits the frame with the same SAR as the encode + /// (issue #149). + static String? normalizeSar(String? raw) { + final sar = raw?.trim(); + if (sar == null || sar.isEmpty || sar == 'N/A' || sar == '1:1') return null; + final parts = sar.split(':'); + if (parts.length == 2 && (double.tryParse(parts[0]) ?? 0) <= 0) return null; + return sar; + } + /// Picks the true picture frame rate from the container's `r_frame_rate` /// (the stream's base/tick rate) and `avg_frame_rate` (the average displayed /// rate). diff --git a/app/lib/services/preview_generator.dart b/app/lib/services/preview_generator.dart index 0836df93..5c669b96 100644 --- a/app/lib/services/preview_generator.dart +++ b/app/lib/services/preview_generator.dart @@ -79,6 +79,12 @@ class PreviewGenerator { String? _colorTransfer; String? _colorRange; + /// Source sample aspect ratio ("16:15"), null for square pixels. Sent with + /// the preview job so the script fits the frame by the same display aspect + /// as the encode — without it `{{SOURCE_SAR}}` fell back to 1, and Square + /// pixel aspect previewed a different frame from the one encoded (#149). + String? _sar; + /// The display tone-map for the loaded source, or null for SDR. Applied to /// every picture of the source the app draws itself. String? get _toneMap => HdrDisplay.toneMapFilter( @@ -317,6 +323,7 @@ class PreviewGenerator { inputColorPrimaries: _colorPrimaries, inputColorTransfer: _colorTransfer, inputColorRange: _colorRange, + inputSar: _sar, // Not a trim: the worker's preview only uses it as the point the // encode's retime starts from, so a generated frame is previewed at // the moment the encode makes it. @@ -565,6 +572,8 @@ class PreviewGenerator { _colorPrimaries = colorTag(videoStream, 'color_primaries'); _colorTransfer = colorTag(videoStream, 'color_transfer'); _colorRange = colorTag(videoStream, 'color_range'); + _sar = FieldOrderDetector.normalizeSar( + videoStream['sample_aspect_ratio'] as String?); } /// Parses an ffprobe rational frame-rate string (e.g. "25/1") to a double. diff --git a/app/lib/viewmodels/main_viewmodel.dart b/app/lib/viewmodels/main_viewmodel.dart index a308f160..abdd732b 100644 --- a/app/lib/viewmodels/main_viewmodel.dart +++ b/app/lib/viewmodels/main_viewmodel.dart @@ -24,6 +24,7 @@ import '../services/disc_detector.dart'; import '../services/dvd_service.dart'; import '../services/field_order_detector.dart'; import '../services/frame_math.dart'; +import '../services/image_dimensions.dart'; import '../services/timeline_math.dart'; import '../services/preset_service.dart'; import '../services/preview_generator.dart'; @@ -34,6 +35,18 @@ import '../services/system_shutdown.dart'; import '../services/worker_manager.dart'; import '../widgets/comparison_zoom_controller.dart'; +/// The display aspect a processed preview is drawn at: its stored size with +/// the aspect the worker reported for it applied, i.e. the shape the encode +/// will have. Null — draw it at its own pixel shape — when there is no image, +/// its size can't be read, or the worker reported nothing (issue #149). +@visibleForTesting +double? processedDisplayAspect(Uint8List? image, PreviewFrameInfo? info) { + if (image == null || info == null) return null; + final pixels = encodedImageSize(image); + if (pixels == null || pixels.isEmpty) return null; + return info.displayAspectFor(pixels); +} + /// Main view model managing application state. class MainViewModel extends ChangeNotifier { /// Runs jobs. Injectable so the queue state machine can be driven from a @@ -672,6 +685,10 @@ class MainViewModel extends ChangeNotifier { List get thumbnails => selectedItem?.thumbnails ?? []; Uint8List? get currentFrame => selectedItem?.currentFrame; Uint8List? get processedPreview => selectedItem?.processedPreview; + + /// The display aspect of [processedPreview] — the shape the encode will + /// have — or null when it is drawn at its own pixel shape. + double? get processedPreviewAspect => selectedItem?.processedPreviewAspect; double get scrubberPosition => selectedItem?.scrubberPosition ?? 0.0; bool get isGeneratingPreview => _isGeneratingPreview; double get videoDuration => _previewGenerator.duration; @@ -1419,6 +1436,8 @@ class MainViewModel extends ChangeNotifier { if (generation == _previewGeneration && !cancelToken.isCancelled) { item.processedPreview = preview; _previewFrameInfo = preview == null ? null : _previewGenerator.lastFrameInfo; + item.processedPreviewAspect = + processedDisplayAspect(preview, _previewFrameInfo); // The worker clamps to the frames there are, e.g. after the frame // rate pass is switched off; step on from what it showed. if (_previewFrameInfo != null) _previewSubFrame = _previewFrameInfo!.subFrame; diff --git a/app/lib/views/preview_panel.dart b/app/lib/views/preview_panel.dart index 9ef3951a..e39aaeca 100644 --- a/app/lib/views/preview_panel.dart +++ b/app/lib/views/preview_panel.dart @@ -7,6 +7,7 @@ import 'package:provider/provider.dart'; import '../models/app_shortcut.dart'; import '../models/preview_frame_info.dart'; +import '../services/field_order_detector.dart' show VideoInfo; import '../services/timeline_math.dart'; import '../viewmodels/main_viewmodel.dart'; import '../widgets/before_after_comparison.dart'; @@ -46,23 +47,19 @@ class _PreviewPanelState extends State { ); } + /// The display aspect of the source frame, `width / height * SAR`, or null + /// for square pixels or an unreadable SAR. + static double? _sourceDisplayAspect(VideoInfo? videoInfo) { + if (videoInfo == null || videoInfo.height <= 0) return null; + final sar = PreviewFrameInfo.parseRatio(videoInfo.sar); + if (sar == null) return null; + return videoInfo.width / videoInfo.height * sar; + } + Widget _buildPreviewComparison(BuildContext context, MainViewModel viewModel) { - // Compute display aspect ratio for SAR correction. - // DAR = (width / height) * (sarNum / sarDen) - // Only needed when SAR is non-square (e.g. anamorphic DVD 32:27). - double? displayAspectRatio; - final videoInfo = viewModel.videoInfo; - if (videoInfo != null && videoInfo.sar != null) { - final parts = videoInfo.sar!.split(':'); - if (parts.length == 2) { - final sarNum = double.tryParse(parts[0]); - final sarDen = double.tryParse(parts[1]); - if (sarNum != null && sarDen != null && sarDen > 0) { - displayAspectRatio = - (videoInfo.width / videoInfo.height) * (sarNum / sarDen); - } - } - } + // The source's display aspect, for SAR correction of the "before" + // frame: DAR = (width / height) * SAR. Null for square pixels. + final sourceAspect = _sourceDisplayAspect(viewModel.videoInfo); // The scope reads the processed preview, falling back to the source frame // before any pass has rendered — that is what the user is looking at. @@ -78,7 +75,15 @@ class _PreviewPanelState extends State { afterImage: scopeImage, isBeforeLoading: viewModel.isAnalyzing, isAfterLoading: viewModel.isGeneratingPreview, - displayAspectRatio: displayAspectRatio, + beforeAspectRatio: sourceAspect, + // The processed frame is drawn at the shape the encode will + // have, which a resize, pad, crop, rotation or squaring makes + // different from the source's (issue #149). Until a processed + // frame exists the source frame stands in for it, at its own + // aspect. + afterAspectRatio: viewModel.processedPreview != null + ? viewModel.processedPreviewAspect + : sourceAspect, zoomController: viewModel.previewZoom, ), ), diff --git a/app/lib/widgets/before_after_comparison.dart b/app/lib/widgets/before_after_comparison.dart index 17dc3216..496bf533 100644 --- a/app/lib/widgets/before_after_comparison.dart +++ b/app/lib/widgets/before_after_comparison.dart @@ -35,9 +35,18 @@ class BeforeAfterComparisonWidget extends StatefulWidget { /// Whether the after image is loading. final bool isAfterLoading; - /// Display aspect ratio for SAR correction (e.g. 16/9 for anamorphic DVD). - /// When null, the image's native pixel aspect ratio is used (square pixels). - final double? displayAspectRatio; + /// Display aspect ratio of the "before" image, for SAR correction (e.g. + /// 4/3 for a 720x576 PAL source). When null, the image's native pixel + /// aspect ratio is used (square pixels). + final double? beforeAspectRatio; + + /// Display aspect ratio of the "after" image — the shape the processed + /// frame will have once encoded. Each side has its own: a pass that + /// resizes, pads, crops, rotates or squares the pixels gives the processed + /// frame a different shape from the source, and drawing it at the source's + /// squashed it (issue #149). When null, the image's native pixel aspect + /// ratio is used. + final double? afterAspectRatio; /// Lets keyboard shortcuts drive Fit / 1:1 from outside the widget. final ComparisonZoomController? zoomController; @@ -49,7 +58,8 @@ class BeforeAfterComparisonWidget extends StatefulWidget { this.initialDividerPosition = 0.5, this.isBeforeLoading = false, this.isAfterLoading = false, - this.displayAspectRatio, + this.beforeAspectRatio, + this.afterAspectRatio, this.zoomController, }); @@ -166,6 +176,12 @@ class _BeforeAfterComparisonWidgetState ); } + /// Display aspect of the frame [_imagePixels] measures: the processed + /// frame's when there is one, else the source's. + double? get _measuredAspect => widget.afterImage != null + ? widget.afterAspectRatio + : widget.beforeAspectRatio; + /// Pixel size of the frame the 1:1 scale is measured against: the /// processed frame, which is what is being inspected, else the source. Size? get _imagePixels { @@ -182,7 +198,7 @@ class _BeforeAfterComparisonWidgetState Size? _fittedSize(Size box) { final pixels = _imagePixels; if (pixels == null || box.isEmpty) return null; - final aspect = widget.displayAspectRatio ?? pixels.width / pixels.height; + final aspect = _measuredAspect ?? pixels.width / pixels.height; final width = math.min(box.width, box.height * aspect); return Size(width, width / aspect); } @@ -398,6 +414,7 @@ class _BeforeAfterComparisonWidgetState ..scale(_scale, _scale), child: _buildImageLayer( imageData: widget.afterImage, + displayAspectRatio: widget.afterAspectRatio, isLoading: widget.isAfterLoading, colorScheme: colorScheme, label: 'Processed', @@ -415,6 +432,7 @@ class _BeforeAfterComparisonWidgetState ..scale(_scale, _scale), child: _buildImageLayer( imageData: widget.beforeImage, + displayAspectRatio: widget.beforeAspectRatio, isLoading: widget.isBeforeLoading, colorScheme: colorScheme, label: 'Original', @@ -554,6 +572,7 @@ class _BeforeAfterComparisonWidgetState Widget _buildImageLayer({ required Uint8List? imageData, + required double? displayAspectRatio, required bool isLoading, required ColorScheme colorScheme, required String label, @@ -564,11 +583,11 @@ class _BeforeAfterComparisonWidgetState // FittedBox scales its child uniformly to fit the parent while // preserving the SizedBox's aspect ratio — works even under the // tight constraints from StackFit.expand. - if (widget.displayAspectRatio != null) { + if (displayAspectRatio != null) { return FittedBox( fit: BoxFit.contain, child: SizedBox( - width: widget.displayAspectRatio! * 1000, + width: displayAspectRatio * 1000, height: 1000, child: Image.memory( imageData, diff --git a/app/test/before_after_comparison_test.dart b/app/test/before_after_comparison_test.dart index 83833b8c..ba3eee47 100644 --- a/app/test/before_after_comparison_test.dart +++ b/app/test/before_after_comparison_test.dart @@ -66,6 +66,8 @@ void main() { double height = 300, double devicePixelRatio = 1, double? displayAspectRatio, + Uint8List? beforeImage, + double? beforeAspectRatio, }) async { tester.view.devicePixelRatio = devicePixelRatio; tester.view.physicalSize = @@ -78,9 +80,12 @@ void main() { width: width, height: height, child: BeforeAfterComparisonWidget( - beforeImage: image, + beforeImage: beforeImage ?? image, afterImage: image, - displayAspectRatio: displayAspectRatio, + beforeAspectRatio: beforeImage != null + ? beforeAspectRatio + : displayAspectRatio, + afterAspectRatio: displayAspectRatio, zoomController: controller, ), ), @@ -150,6 +155,55 @@ void main() { expect(scaleOf(tester), closeTo(480 / fittedHeight, 1e-9)); }); + // Issue #149: a 720x576 PAL 4:3 source upscaled and padded to a + // 1920x1080 canvas was drawn squashed into the source's 4:3 shape. + testWidgets('each side is drawn at its own display aspect', + (tester) async { + final source = await tester.runAsync(() => _png(720, 576)); + final processed = await tester.runAsync(() => _png(1920, 1080)); + await pump( + tester, + processed!, + displayAspectRatio: 16 / 9, + beforeImage: source!, + beforeAspectRatio: 4 / 3, + ); + + // Stack order: the processed layer, then the source layer. + final images = find.descendant( + of: find.byType(BeforeAfterComparisonWidget), + matching: find.byType(Image), + ); + expect(images, findsNWidgets(2)); + final after = tester.getRect(images.at(0)); + final before = tester.getRect(images.at(1)); + expect(after.width / after.height, closeTo(16 / 9, 1e-6)); + expect(before.width / before.height, closeTo(4 / 3, 1e-6)); + // Both fit the 398x298 panel: the processed one by width, the source + // by height. + expect(after.width, closeTo(398, 1e-6)); + expect(before.height, closeTo(298, 1e-6)); + }); + + testWidgets('1:1 is measured against the processed frame\'s aspect', + (tester) async { + final source = await tester.runAsync(() => _png(720, 576)); + final processed = await tester.runAsync(() => _png(1920, 1080)); + await pump( + tester, + processed!, + displayAspectRatio: 16 / 9, + beforeImage: source!, + beforeAspectRatio: 4 / 3, + ); + controller.showActualSize(); + await tester.pumpAndSettle(); + + // Fitted 398 wide at 16:9; 1:1 puts its 1080 rows on 1080 pixels. + final fittedHeight = 398 / (16 / 9); + expect(scaleOf(tester), closeTo(1080 / fittedHeight, 1e-9)); + }); + testWidgets('the frame is drawn on the pixel grid at 1:1', (tester) async { // An odd-sized frame centred in an even panel would straddle pixels. final image = await tester.runAsync(() => _png(201, 101)); diff --git a/app/test/integration_aspect_ratio_test.dart b/app/test/integration_aspect_ratio_test.dart index 06dae450..c6d53ed3 100644 --- a/app/test/integration_aspect_ratio_test.dart +++ b/app/test/integration_aspect_ratio_test.dart @@ -13,15 +13,21 @@ library; // ignore_for_file: avoid_print — these tests print diagnostics to the test log. import 'dart:io'; +import 'dart:typed_data'; +import 'dart:ui' show Size; import 'package:flutter_test/flutter_test.dart'; import 'package:uuid/uuid.dart'; import 'package:vapourbox/models/crop_resize_parameters.dart'; import 'package:vapourbox/models/encoding_settings.dart'; +import 'package:vapourbox/models/preview_frame_info.dart'; import 'package:vapourbox/models/processing_pipeline.dart'; import 'package:vapourbox/models/qtgmc_parameters.dart'; import 'package:vapourbox/models/video_job.dart'; +import 'package:vapourbox/services/image_dimensions.dart'; +import 'package:vapourbox/viewmodels/main_viewmodel.dart' + show processedDisplayAspect; import 'support/worker_harness.dart'; @@ -35,13 +41,76 @@ const _srcWidth = 720; const _srcHeight = 576; const _srcSar = 16 / 11; +/// The issue #149 source: 720x576 PAL 4:3 (16:15 pixels), a flat mid-grey so +/// the picture's extent inside the bars can be measured. +late String _pal43; + +VideoJob _job( + String name, + CropResizeParameters cropResize, { + required String input, + required String inputSar, +}) => + VideoJob( + id: const Uuid().v4(), + inputPath: input, + outputPath: '$_outDir/$name.mkv', + processingPipeline: ProcessingPipeline( + deinterlace: const QTGMCParameters(enabled: false), + cropResize: cropResize, + ), + encodingSettings: const EncodingSettings( + codec: VideoCodec.h264, + container: ContainerFormat.mkv, + audioMode: AudioMode.none, + ), + inputWidth: _srcWidth, + inputHeight: _srcHeight, + inputFrameRate: 25.0, + inputPixelFormat: 'yuv420p', + inputSar: inputSar, + ); + +/// Preview one frame the way the app does, returning the PNG, what the +/// worker reported for it, and the display aspect the app draws it at. +Future<({Uint8List png, PreviewFrameInfo info, double? aspect})> _preview( + String name, + CropResizeParameters cropResize, { + required String input, + required String inputSar, +}) async { + final job = _job(name, cropResize, input: input, inputSar: inputSar); + final result = + await WorkerHarness.runPreview(job.toJson(), frame: 2, label: name); + expect(result.success, isTrue, reason: '$name: $result'); + final info = PreviewFrameInfo.parse(result.logs); + expect(info, isNotNull, reason: '$name: no PREVIEW_INFO line'); + final aspect = processedDisplayAspect(result.png, info); + print(' $name (preview): ${encodedImageSize(result.png!)} ' + 'SAR=${info!.outputSar} DAR=${info.outputDar} -> drawn at $aspect'); + return (png: result.png!, info: info, aspect: aspect); +} + +/// Columns of the middle row that are picture rather than black bars. +Future _pictureWidth(Uint8List rgb24, int width, int height) async { + final row = (height ~/ 2) * width * 3; + var count = 0; + for (var x = 0; x < width; x++) { + final i = row + x * 3; + if (rgb24[i] + rgb24[i + 1] + rgb24[i + 2] > 3 * 64) count++; + } + return count; +} + Future<({int width, int height, String sar, String dar})> _encode( String name, - CropResizeParameters cropResize, -) async { + CropResizeParameters cropResize, { + String? input, + String inputSar = '16:11', +}) async { final job = VideoJob( id: const Uuid().v4(), - inputPath: _anamorphic, + inputPath: input ?? _anamorphic, outputPath: '$_outDir/$name.mkv', processingPipeline: ProcessingPipeline( deinterlace: const QTGMCParameters(enabled: false), @@ -55,7 +124,7 @@ Future<({int width, int height, String sar, String dar})> _encode( inputWidth: _srcWidth, inputHeight: _srcHeight, inputFrameRate: 25.0, - inputSar: '16:11', + inputSar: inputSar, startFrame: 0, endFrame: 4, ); @@ -114,6 +183,22 @@ void main() { selector: 'v:0', entries: ['sample_aspect_ratio']); expect(src?['sample_aspect_ratio'], '16:11', reason: 'fixture must be anamorphic to exercise this at all'); + + _pal43 = '$_outDir/src_pal_4x3_16x15.mkv'; + final pal = await Process.run( + WorkerHarness.ffmpegPath, + [ + '-hide_banner', '-loglevel', 'error', '-y', + '-f', 'lavfi', + '-i', 'color=c=0x808080:size=${_srcWidth}x$_srcHeight:rate=25:duration=0.4', + '-c:v', 'ffv1', '-pix_fmt', 'yuv420p', + // 16:15 pixels: PAL 4:3. 720 * 16/15 / 576 = 4:3. + '-vf', 'setsar=16/15', + _pal43, + ], + environment: WorkerHarness.ffmpegEnv, + ); + expect(pal.exitCode, 0, reason: 'fixture: ${pal.stderr}'); }); group('aspect ratio (full encode)', () { @@ -217,4 +302,72 @@ void main() { expect(out.height, 1024); }, timeout: const Timeout(Duration(minutes: 5))); }); + + // Issue #149: a 720x576 PAL 4:3 capture resized to 1440x1080 and given + // borders out to 1920x1080 previewed squashed, because the processed frame + // was drawn at the source's 4:3. The preview now reports the aspect the + // encode declares, and is fitted with the same SAR. + group('preview aspect (issue #149)', () { + CropResizeParameters reporter(PixelAspectMode mode) => CropResizeParameters( + enabled: true, + resizeEnabled: true, + targetWidth: 1440, + targetHeight: 1080, + maintainAspect: true, + pixelAspect: mode, + padEnabled: true, + padWidth: 1920, + padHeight: 1080, + ); + + test('square pixels: drawn 16:9, picture fitted like the encode', () async { + final params = reporter(PixelAspectMode.square); + final preview = await _preview('preview_149_square', params, + input: _pal43, inputSar: '16:15'); + expect(encodedImageSize(preview.png), const Size(1920, 1080)); + expect(preview.info.outputSar, isNull); + expect(preview.info.outputDar, isNull); + expect(preview.aspect, closeTo(16 / 9, 1e-6), + reason: 'the processed frame must not be drawn at the source\'s 4:3'); + + // The picture inside the bars is the width the encode makes it: 4:3 + // at 1080 tall is 1440. Without the source SAR in the preview job it + // was fitted as 1350. + final out = await _encode('aspect_149_square', params, + input: _pal43, inputSar: '16:15'); + expect((out.width, out.height), (1920, 1080)); + final encoded = await _pictureWidth( + await WorkerHarness.frameRgb24( + '$_outDir/aspect_149_square.mkv', 0, label: 'enc149'), + 1920, 1080); + final previewed = await _pictureWidth( + await WorkerHarness.imageToRgb24(preview.png, label: 'pv149'), + 1920, 1080); + print(' picture width: encode $encoded, preview $previewed'); + expect(encoded, closeTo(1440, 4)); + expect(previewed, closeTo(encoded, 4)); + }, timeout: const Timeout(Duration(minutes: 5))); + + test('preserve: drawn at the SAR the encode stamps', () async { + final params = reporter(PixelAspectMode.preserve); + final preview = await _preview('preview_149_preserve', params, + input: _pal43, inputSar: '16:15'); + expect(preview.info.outputSar, closeTo(16 / 15, 1e-9)); + final out = await _encode('aspect_149_preserve', params, + input: _pal43, inputSar: '16:15'); + expect(preview.aspect, closeTo(_displayAspect(out), 0.01), + reason: 'the preview must be drawn at the encode\'s display aspect'); + }, timeout: const Timeout(Duration(minutes: 5))); + + test('a forced display aspect is reported as a DAR', () async { + final preview = await _preview( + 'preview_149_dar', + const CropResizeParameters(enabled: true, displayAspect: '16:9'), + input: _pal43, + inputSar: '16:15', + ); + expect(preview.info.outputDar, closeTo(16 / 9, 1e-9)); + expect(preview.aspect, closeTo(16 / 9, 1e-9)); + }, timeout: const Timeout(Duration(minutes: 5))); + }); } diff --git a/app/test/preview_aspect_test.dart b/app/test/preview_aspect_test.dart new file mode 100644 index 00000000..56555fc9 --- /dev/null +++ b/app/test/preview_aspect_test.dart @@ -0,0 +1,70 @@ +// Issue #149: the processed preview is drawn at the shape the encode will +// have, not the source's. +// +// Run with: flutter test test/preview_aspect_test.dart + +import 'dart:typed_data'; + +import 'package:flutter_test/flutter_test.dart'; +import 'package:vapourbox/models/preview_frame_info.dart'; +import 'package:vapourbox/services/field_order_detector.dart'; +import 'package:vapourbox/viewmodels/main_viewmodel.dart'; + +/// Just enough of a PNG for its header to be read: signature, IHDR length and +/// type, width, height. +Uint8List _pngHeader(int width, int height) { + final b = ByteData(24); + const sig = [0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A]; + for (var i = 0; i < 8; i++) { + b.setUint8(i, sig[i]); + } + b.setUint32(8, 13); + b.setUint32(12, 0x49484452); // "IHDR" + b.setUint32(16, width); + b.setUint32(20, height); + return b.buffer.asUint8List(); +} + +void main() { + group('processedDisplayAspect', () { + final canvas = _pngHeader(1920, 1080); + + test('square pixels: the frame\'s own shape, not the source\'s', () { + // The reporter's setup with Square pixel aspect: nothing declared. + final info = PreviewFrameInfo.parse('PREVIEW_INFO:frame=1')!; + expect(processedDisplayAspect(canvas, info), closeTo(16 / 9, 1e-9)); + }); + + test('a declared SAR stretches the stored size', () { + final info = PreviewFrameInfo.parse('PREVIEW_INFO:frame=1 sar=16/15')!; + expect(processedDisplayAspect(canvas, info), + closeTo(1920 * 16 / 15 / 1080, 1e-9)); + }); + + test('a declared DAR is used as is', () { + final info = PreviewFrameInfo.parse('PREVIEW_INFO:frame=1 dar=4/3')!; + expect(processedDisplayAspect(canvas, info), closeTo(4 / 3, 1e-9)); + }); + + test('nothing to go on draws the frame at its own shape', () { + final info = PreviewFrameInfo.parse('PREVIEW_INFO:frame=1')!; + expect(processedDisplayAspect(null, info), isNull); + expect(processedDisplayAspect(canvas, null), isNull); + expect(processedDisplayAspect(Uint8List.fromList([1, 2, 3]), info), + isNull); + }); + }); + + group('FieldOrderDetector.normalizeSar', () { + test('keeps a real SAR', () { + expect(FieldOrderDetector.normalizeSar('16:15'), '16:15'); + expect(FieldOrderDetector.normalizeSar(' 64:45 '), '64:45'); + }); + + test('square, unknown and missing read as none', () { + for (final raw in [null, '', 'N/A', '1:1', '0:1']) { + expect(FieldOrderDetector.normalizeSar(raw), isNull, reason: '$raw'); + } + }); + }); +} diff --git a/app/test/preview_frame_info_test.dart b/app/test/preview_frame_info_test.dart index 7337042d..de4bf233 100644 --- a/app/test/preview_frame_info_test.dart +++ b/app/test/preview_frame_info_test.dart @@ -1,3 +1,5 @@ +import 'dart:ui' show Size; + import 'package:flutter_test/flutter_test.dart'; import 'package:vapourbox/models/preview_frame_info.dart'; @@ -31,5 +33,52 @@ void main() { 'PREVIEW_INFO:frame=3 sub_frame=9 sub_frames=2 generated=0')!; expect(info.subFrame, 1); }); + + // Issue #149: the processed frame is drawn at the aspect the encode will + // declare, not the source's. + test('reads the output SAR the encode will declare', () { + final info = PreviewFrameInfo.parse( + 'PREVIEW_INFO:frame=3 sub_frame=0 sub_frames=1 generated=0 sar=16/15')!; + expect(info.outputSar, closeTo(16 / 15, 1e-9)); + expect(info.outputDar, isNull); + // 1920x1080 padded, Preserve kept the PAL 4:3 SAR: wider than 16:9. + expect(info.displayAspectFor(const Size(1920, 1080)), + closeTo(1920 / 1080 * 16 / 15, 1e-9)); + }); + + test('a forced DAR wins over the frame size', () { + final info = + PreviewFrameInfo.parse('PREVIEW_INFO:frame=3 dar=16/9')!; + expect(info.outputDar, closeTo(16 / 9, 1e-9)); + expect(info.displayAspectFor(const Size(720, 576)), closeTo(16 / 9, 1e-9)); + }); + + test('no aspect token means square pixels: the frame\'s own shape', () { + final info = PreviewFrameInfo.parse( + 'PREVIEW_INFO:frame=3 sub_frame=0 sub_frames=1 generated=0')!; + expect(info.outputSar, isNull); + expect(info.outputDar, isNull); + expect(info.displayAspectFor(const Size(1920, 1080)), + closeTo(16 / 9, 1e-9)); + }); + + test('a malformed aspect is ignored rather than drawn zero-wide', () { + final info = + PreviewFrameInfo.parse('PREVIEW_INFO:frame=3 sar=0/1 dar=wide')!; + expect(info.outputSar, isNull); + expect(info.outputDar, isNull); + expect(info.frame, 3); + }); + }); + + group('PreviewFrameInfo.parseRatio', () { + test('accepts the forms the worker and users write', () { + expect(PreviewFrameInfo.parseRatio('16/9'), closeTo(16 / 9, 1e-9)); + expect(PreviewFrameInfo.parseRatio('16:9'), closeTo(16 / 9, 1e-9)); + expect(PreviewFrameInfo.parseRatio('1.5'), 1.5); + for (final junk in [null, '', '16/0', '-4/3', 'wide', '1/2/3']) { + expect(PreviewFrameInfo.parseRatio(junk), isNull, reason: '$junk'); + } + }); }); } diff --git a/worker/src/models/crop_resize_parameters.rs b/worker/src/models/crop_resize_parameters.rs index 78706c95..e9920227 100644 --- a/worker/src/models/crop_resize_parameters.rs +++ b/worker/src/models/crop_resize_parameters.rs @@ -521,6 +521,21 @@ pub enum AspectDeclaration { None, } +impl AspectDeclaration { + /// The `key=value` token the preview reports on its `PREVIEW_INFO:` line, + /// or `None` for square pixels: `sar=16/15` or `dar=16/9`. The app draws + /// the processed frame at this shape; without it the frame was drawn at + /// the *source's* display aspect, squashing anything a pass reshaped + /// (issue #149). '/' as the separator, matching what ffmpeg is given. + pub fn preview_token(&self) -> Option { + match self { + AspectDeclaration::Sar(sar) => Some(format!("sar={}", sar.trim().replace(':', "/"))), + AspectDeclaration::Dar(dar) => Some(format!("dar={}", dar.trim().replace(':', "/"))), + AspectDeclaration::None => None, + } + } +} + #[allow(dead_code)] impl CropResizeParameters { /// Whether this pass upscales with a neural model, which needs the VSNCNN @@ -958,6 +973,19 @@ mod tests { assert_eq!(params.aspect_declaration(Some("10:11")), AspectDeclaration::None); } + #[test] + fn test_preview_token() { + assert_eq!( + AspectDeclaration::Sar("16:15".into()).preview_token().as_deref(), + Some("sar=16/15") + ); + assert_eq!( + AspectDeclaration::Dar("16/9".into()).preview_token().as_deref(), + Some("dar=16/9") + ); + assert_eq!(AspectDeclaration::None.preview_token(), None); + } + #[test] fn test_parse_hex_color() { assert_eq!(parse_hex_color("#FF8000"), Some([255, 128, 0])); diff --git a/worker/src/models/processing_pipeline.rs b/worker/src/models/processing_pipeline.rs index a4cecfb4..955a14c3 100644 --- a/worker/src/models/processing_pipeline.rs +++ b/worker/src/models/processing_pipeline.rs @@ -9,6 +9,7 @@ use super::{ ChromaDenoiseParameters, ChromaFixParameters, ColorCorrectionParameters, CropResizeParameters, DebandParameters, DeblockParameters, DehaloParameters, DeinterlaceMethod, DeScratchParameters, SpotLessParameters, SharpenParameters, NoiseReductionParameters, QTGMCParameters, FrameRateParameters, FrameRateMethod, NoiseReductionMethod, DeflickerParameters, EdgeRepairParameters, GhostRemovalParameters, DescaleParameters, FaceRestoreParameters, ColorizeParameters, AudioRestorationParameters,}; +use super::AspectDeclaration; /// Minimum temporal context (frames on each side of the target) a windowed /// preview decodes, regardless of which filters are enabled. Motion-compensated @@ -860,6 +861,20 @@ impl ProcessingPipeline { declared.max(MIN_PREVIEW_RADIUS) } + /// How the output's aspect is declared, given the source's SAR as ffprobe + /// reports it ("16:15", or `None` for square pixels). + /// + /// The single decision used by both the encode (which stamps it with + /// `setsar`/`setdar`, because the Y4M pipe strips aspect) and the preview + /// (which reports it so the app can draw the processed frame at the shape + /// the encode will have — issue #149). A quarter turn exchanges pixel width + /// and height, so the source SAR is adjusted for rotation first, or a + /// rotated anamorphic source is stretched by the square of its own aspect. + pub fn output_aspect(&self, input_sar: Option<&str>) -> AspectDeclaration { + let rotated_sar = self.geometry.adjusted_sar(input_sar); + self.crop_resize.aspect_declaration(rotated_sar.as_deref()) + } + /// Check if a specific pass is enabled. pub fn is_pass_enabled(&self, pass: PassType) -> bool { match pass { @@ -894,6 +909,49 @@ impl ProcessingPipeline { mod tests { use super::*; + #[test] + fn test_output_aspect_carries_the_source_sar_through() { + let p = ProcessingPipeline::default(); + assert_eq!(p.output_aspect(Some("16:15")), AspectDeclaration::Sar("16:15".into())); + assert_eq!(p.output_aspect(None), AspectDeclaration::None); + } + + #[test] + fn test_output_aspect_adjusts_the_sar_for_a_quarter_turn() { + let p = ProcessingPipeline { + geometry: GeometryParameters { + enabled: true, + rotation: crate::models::Rotation::Cw90, + ..Default::default() + }, + ..Default::default() + }; + assert_eq!(p.output_aspect(Some("16:15")), AspectDeclaration::Sar("15:16".into())); + } + + #[test] + fn test_output_aspect_follows_the_crop_resize_decision() { + let square = ProcessingPipeline { + crop_resize: CropResizeParameters { + enabled: true, + pixel_aspect: crate::models::PixelAspectMode::Square, + ..Default::default() + }, + ..Default::default() + }; + assert_eq!(square.output_aspect(Some("16:15")), AspectDeclaration::None); + + let forced = ProcessingPipeline { + crop_resize: CropResizeParameters { + enabled: true, + display_aspect: Some("16:9".into()), + ..Default::default() + }, + ..Default::default() + }; + assert_eq!(forced.output_aspect(Some("16:15")), AspectDeclaration::Dar("16/9".into())); + } + fn retimed(target: (u32, u32), src: (u32, u32), method: FrameRateMethod, deinterlace: bool) -> ProcessingPipeline { ProcessingPipeline { deinterlace: QTGMCParameters { enabled: deinterlace, ..Default::default() }, diff --git a/worker/src/pipeline_executor.rs b/worker/src/pipeline_executor.rs index 42cd146c..1ae86d4d 100644 --- a/worker/src/pipeline_executor.rs +++ b/worker/src/pipeline_executor.rs @@ -163,6 +163,28 @@ fn preview_window(target: i32, radius: i32) -> (i32, i32, i32) { } use crate::audio_restoration::{self, RestoredTrack}; +/// The `PREVIEW_INFO:` line a preview reports on stderr after rendering: the +/// frame it rendered, which of that frame's outputs, and — when not square — +/// the display aspect the encode will declare for it. The PNG carries no +/// aspect, so without that last token the app had to guess, and it guessed the +/// *source's* shape: a 720x576 4:3 source upscaled and padded to 1920x1080 was +/// drawn squashed into 4:3 (issue #149). +fn preview_info_line(job: &VideoJob, target: i32, out: &crate::models::PreviewOutput) -> String { + let mut line = format!( + "PREVIEW_INFO:frame={} sub_frame={} sub_frames={} generated={}", + target, out.sub_frame, out.sub_frames, out.generated as u8 + ); + if let Some(token) = job + .effective_pipeline() + .output_aspect(job.input_sar.as_deref()) + .preview_token() + { + line.push(' '); + line.push_str(&token); + } + line +} + use crate::comment_metadata::{self, CommentFile}; use crate::gpu_plugin::GpuPlugin; use crate::hdr_metadata::HdrMetadata; @@ -972,10 +994,8 @@ impl PipelineExecutor { // a resize ran, which used to drop it and leave an anamorphic source // squashed (#50). let pipeline = job.effective_pipeline(); - // A quarter turn exchanges pixel width and height, so the SAR that - // describes the source no longer describes the rotated frame. Adjust it - // before declaring it, or a rotated anamorphic source is stretched by - // the square of its own aspect. + // `output_aspect` adjusts the SAR for a quarter turn; the preview + // reports the same decision, so the two cannot disagree (#149). // // Filters accumulate into one chain. ffmpeg takes the LAST -vf and // silently drops any earlier one, so appending a second would throw @@ -983,8 +1003,7 @@ impl PipelineExecutor { // would have broken issue #50's third leg. let mut vf: Vec = Vec::new(); - let rotated_sar = pipeline.geometry.adjusted_sar(input_sar); - match pipeline.crop_resize.aspect_declaration(rotated_sar.as_deref()) { + match pipeline.output_aspect(input_sar) { AspectDeclaration::Sar(sar) => { // '/' as the ratio separator — ':' is ffmpeg's filter option separator. vf.push(format!("setsar={}", sar.replace(':', "/"))); @@ -1250,8 +1269,9 @@ impl PipelineExecutor { /// then emits the exact output frame that `frame_index` maps to (accounting /// for any frame-rate change such as QTGMC double-rate). The requested frame /// is echoed back on stderr as `PREVIEW_INFO:frame=N sub_frame=K - /// sub_frames=C generated=0|1` so the caller can - /// label the result truthfully. + /// sub_frames=C generated=0|1 [sar=N/D | dar=N/D]` so the caller can + /// label the result truthfully and draw it at the encode's display aspect + /// (see [`preview_info_line`]). pub fn generate_preview(&self, job: &VideoJob, frame_index: i32, sub_frame: u32) -> Result<()> { use std::io::Write; @@ -1509,10 +1529,7 @@ impl PipelineExecutor { // Echo the frame we actually rendered so the caller can label it (the // PNG itself is the only thing on stdout, so this goes to stderr). - eprintln!( - "PREVIEW_INFO:frame={} sub_frame={} sub_frames={} generated={}", - target, out.sub_frame, out.sub_frames, out.generated as u8 - ); + eprintln!("{}", preview_info_line(job, target, &out)); // Write PNG to stdout std::io::stdout().write_all(&output.stdout)?; @@ -1952,6 +1969,51 @@ mod tests { args } + fn preview_out() -> crate::models::PreviewOutput { + crate::models::PreviewOutput { + local_index: 3, + rate_pad: 0, + sub_frame: 0, + sub_frames: 2, + generated: false, + } + } + + /// The preview reports the aspect the encode will declare, so the app can + /// draw the processed frame at its own shape (issue #149). + #[test] + fn preview_info_reports_the_output_aspect() { + let mut job = create_test_job("/tmp/out.mkv"); + job.input_sar = Some("16:15".to_string()); + assert_eq!( + preview_info_line(&job, 7, &preview_out()), + "PREVIEW_INFO:frame=7 sub_frame=0 sub_frames=2 generated=0 sar=16/15" + ); + + // Squaring the pixels resamples the grid: nothing to declare. + let mut pipeline = job.effective_pipeline(); + pipeline.crop_resize.enabled = true; + pipeline.crop_resize.pixel_aspect = crate::models::PixelAspectMode::Square; + job.processing_pipeline = Some(pipeline.clone()); + assert_eq!( + preview_info_line(&job, 7, &preview_out()), + "PREVIEW_INFO:frame=7 sub_frame=0 sub_frames=2 generated=0" + ); + + // A forced display aspect is reported as a DAR. + pipeline.crop_resize.pixel_aspect = crate::models::PixelAspectMode::Preserve; + pipeline.crop_resize.display_aspect = Some("16:9".to_string()); + job.processing_pipeline = Some(pipeline); + assert!(preview_info_line(&job, 7, &preview_out()).ends_with(" dar=16/9")); + + // Square-pixel source, nothing changed: the line is what it always was. + let job = create_test_job("/tmp/out.mkv"); + assert_eq!( + preview_info_line(&job, 7, &preview_out()), + "PREVIEW_INFO:frame=7 sub_frame=0 sub_frames=2 generated=0" + ); + } + fn create_test_job(output_path: &str) -> VideoJob { VideoJob { id: Uuid::new_v4(), diff --git a/worker/tests/filter_integration_test.rs b/worker/tests/filter_integration_test.rs index 91bc743c..e963d8c6 100644 --- a/worker/tests/filter_integration_test.rs +++ b/worker/tests/filter_integration_test.rs @@ -7818,3 +7818,67 @@ fn test_209_gpu_plugin_models_are_addons() { assert!(cases[0].0.effective_pipeline().required_addons().iter().any(|n| n.ids == [RIFE_ADDON])); let _ = std::fs::remove_dir_all(&root); } + +/// Issue #149: the preview must be drawn at the shape the encode declares, and +/// fitted the same way. Two halves: +/// - the preview script's square-pixel fitting reads the source SAR exactly as +/// the encode's does (the app used to send no `inputSar` with a preview job, +/// so `{{SOURCE_SAR}}` fell back to 1 and a 720x576 4:3 source was fitted to +/// 1350x1080 in the preview but 1440x1080 in the encode); +/// - the declaration the preview reports (`output_aspect`) is the one the +/// encode stamps, for the reporter's setup (square + resize + pad) and for +/// the default Preserve. +#[test] +fn test_210_preview_aspect_matches_the_encode() { + create_output_dir(); + let mut job = create_base_job("test_210_preview_aspect"); + job.qtgmc_parameters.enabled = false; + job.input_sar = Some("16:15".to_string()); + let crop_resize = CropResizeParameters { + enabled: true, + resize_enabled: true, + pixel_aspect: PixelAspectMode::Square, + target_width: Some(1440), + target_height: Some(1080), + ..Default::default() + }; + job.processing_pipeline = Some(ProcessingPipeline { + deinterlace: QTGMCParameters { enabled: false, ..Default::default() }, + crop_resize: crop_resize.clone(), + ..ProcessingPipeline::default() + }); + + let generator = ScriptGenerator::new().unwrap(); + let encode = std::fs::read_to_string(generator.generate(&job).unwrap()).unwrap(); + let params = PreviewParams { + rate_pad: 0, + width: 720, height: 576, pix_fmt: "yuv420p".into(), num_frames: 5, + fps_num: 25, fps_den: 1, output_index: 2, + }; + let preview = std::fs::read_to_string(generator.generate_preview(&job, ¶ms).unwrap()).unwrap(); + + let aspect_line = |script: &str| { + script + .lines() + .find(|l| l.starts_with("_display_aspect = (clip.width *")) + .map(str::to_string) + }; + let encode_line = aspect_line(&encode).expect("encode fits by display aspect"); + assert!(encode_line.contains("1.0667"), "source SAR 16/15 expected: {encode_line}"); + assert_eq!( + aspect_line(&preview).as_deref(), + Some(encode_line.as_str()), + "the preview must fit by the same display aspect as the encode" + ); + + // Square pixels: nothing to declare, so the preview reports nothing. + let pipeline = job.effective_pipeline(); + assert_eq!(pipeline.output_aspect(job.input_sar.as_deref()), AspectDeclaration::None); + + // Preserve keeps the source SAR on the padded frame; the preview reports it. + let mut preserve = pipeline.clone(); + preserve.crop_resize.pixel_aspect = PixelAspectMode::Preserve; + let declared = preserve.output_aspect(job.input_sar.as_deref()); + assert_eq!(declared, AspectDeclaration::Sar("16:15".to_string())); + assert_eq!(declared.preview_token().as_deref(), Some("sar=16/15")); +} From 6cc1d5b6059dce5a7bdb584b47a6e0463e7b55d1 Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Tue, 10 Nov 2026 17:21:05 +1100 Subject: [PATCH 2/2] test(preview): cover #149's wiring, unreshaped anamorphic and rotated previews - Move the comparison's per-side aspect choice into MainViewModel (sourceDisplayAspect, comparisonAfterAspect) and the preview-arrival storage into _acceptProcessedPreview, so both are testable without a worker. - preview_aspect_test: an anamorphic source only denoised keeps 16:9 on both sides; the processed aspect survives a scrubber move (fails if computed from previewFrameInfo); falls back to the source's before a processed frame; cleared with a failed preview. - integration_aspect_ratio_test (heavy): denoise-only 16:11 source and a rotated 16:15 source, preview aspect against the encode's. --- CLAUDE.md | 14 ++-- app/lib/viewmodels/main_viewmodel.dart | 48 +++++++++++-- app/lib/views/preview_panel.dart | 27 ++----- app/test/integration_aspect_ratio_test.dart | 71 +++++++++++++++--- app/test/preview_aspect_test.dart | 80 +++++++++++++++++++++ 5 files changed, 199 insertions(+), 41 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4023f345..13109379 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -856,10 +856,16 @@ padded to 1920x1080, say) came out squashed. Three invariants now hold: one encoded (1350 vs 1440 wide in the #149 setup). `PreviewGenerator` reads it through the same `FieldOrderDetector.normalizeSar` as the encode job. -Covered by `test_185` (Rust), `preview_frame_info_test`, `preview_aspect_test`, -`before_after_comparison_test`, and the heavy `preview aspect (issue #149)` -group in `integration_aspect_ratio_test.dart`, which compares the preview's -picture extent with the encode's. +The panel reads both sides' aspects from the view model +(`sourceDisplayAspect`, `comparisonAfterAspect`), not from its own arithmetic, +so the wiring is testable without a worker (`debugAcceptProcessedPreview`). + +Covered by `test_185` (Rust), `preview_frame_info_test`, `preview_aspect_test` +(including the view-model wiring: an unreshaped anamorphic source keeps its +shape, the aspect survives a scrubber move), `before_after_comparison_test`, and +the heavy `preview aspect (issue #149)` group in `integration_aspect_ratio_test.dart`, +which compares the preview with the encode for square, preserve, forced DAR, +denoise-only anamorphic and rotated anamorphic jobs. ### Borders (issue #86) diff --git a/app/lib/viewmodels/main_viewmodel.dart b/app/lib/viewmodels/main_viewmodel.dart index abdd732b..3db220b2 100644 --- a/app/lib/viewmodels/main_viewmodel.dart +++ b/app/lib/viewmodels/main_viewmodel.dart @@ -258,7 +258,8 @@ class MainViewModel extends ChangeNotifier { /// Put an analysed video in the queue and select it, for widget tests. @visibleForTesting - void debugAddQueueItemForTest(String path, {required int width, required int height}) { + void debugAddQueueItemForTest(String path, + {required int width, required int height, String? sar}) { final item = QueueItem( inputPath: path, outputPath: '$path.out.mkv', @@ -271,6 +272,7 @@ class MainViewModel extends ChangeNotifier { codec: 'h264', pixelFormat: 'yuv420p', hasAudio: false, + sar: sar, ), ); _queue.add(item); @@ -689,6 +691,23 @@ class MainViewModel extends ChangeNotifier { /// The display aspect of [processedPreview] — the shape the encode will /// have — or null when it is drawn at its own pixel shape. double? get processedPreviewAspect => selectedItem?.processedPreviewAspect; + + /// The source's display aspect, `width / height * SAR`, for the "before" + /// frame; null for square pixels or an unreadable SAR. + double? get sourceDisplayAspect { + final info = videoInfo; + if (info == null || info.height <= 0) return null; + final sar = PreviewFrameInfo.parseRatio(info.sar); + if (sar == null) return null; + return info.width / info.height * sar; + } + + /// The aspect the "after" side of the comparison is drawn at: the processed + /// frame's own (the encode's shape, which a resize, pad, crop, rotation or + /// squaring makes different from the source's — issue #149), or, until a + /// processed frame exists, the source's, since the source frame stands in. + double? get comparisonAfterAspect => + processedPreview != null ? processedPreviewAspect : sourceDisplayAspect; double get scrubberPosition => selectedItem?.scrubberPosition ?? 0.0; bool get isGeneratingPreview => _isGeneratingPreview; double get videoDuration => _previewGenerator.duration; @@ -1434,10 +1453,8 @@ class MainViewModel extends ChangeNotifier { ); if (generation == _previewGeneration && !cancelToken.isCancelled) { - item.processedPreview = preview; - _previewFrameInfo = preview == null ? null : _previewGenerator.lastFrameInfo; - item.processedPreviewAspect = - processedDisplayAspect(preview, _previewFrameInfo); + _acceptProcessedPreview( + item, preview, preview == null ? null : _previewGenerator.lastFrameInfo); // The worker clamps to the frames there are, e.g. after the frame // rate pass is switched off; step on from what it showed. if (_previewFrameInfo != null) _previewSubFrame = _previewFrameInfo!.subFrame; @@ -1454,6 +1471,27 @@ class MainViewModel extends ChangeNotifier { } } + /// Store a rendered preview on [item] with the display aspect it is drawn + /// at. The aspect is kept with the image because [previewFrameInfo] goes + /// null as soon as the scrubber moves, while this image stays on screen + /// until the next one replaces it (issue #149). + void _acceptProcessedPreview( + QueueItem item, Uint8List? preview, PreviewFrameInfo? info) { + item.processedPreview = preview; + _previewFrameInfo = info; + item.processedPreviewAspect = processedDisplayAspect(preview, info); + } + + /// Deliver a preview as the worker would have, for tests of what the + /// comparison is drawn at. + @visibleForTesting + void debugAcceptProcessedPreview(Uint8List? preview, PreviewFrameInfo? info) { + final item = selectedItem; + if (item == null) return; + _acceptProcessedPreview(item, preview, info); + notifyListeners(); + } + /// Cancels any ongoing preview generation. void _cancelPreviewGeneration() { _previewDebounceTimer?.cancel(); diff --git a/app/lib/views/preview_panel.dart b/app/lib/views/preview_panel.dart index e39aaeca..77d15f39 100644 --- a/app/lib/views/preview_panel.dart +++ b/app/lib/views/preview_panel.dart @@ -7,7 +7,6 @@ import 'package:provider/provider.dart'; import '../models/app_shortcut.dart'; import '../models/preview_frame_info.dart'; -import '../services/field_order_detector.dart' show VideoInfo; import '../services/timeline_math.dart'; import '../viewmodels/main_viewmodel.dart'; import '../widgets/before_after_comparison.dart'; @@ -47,20 +46,7 @@ class _PreviewPanelState extends State { ); } - /// The display aspect of the source frame, `width / height * SAR`, or null - /// for square pixels or an unreadable SAR. - static double? _sourceDisplayAspect(VideoInfo? videoInfo) { - if (videoInfo == null || videoInfo.height <= 0) return null; - final sar = PreviewFrameInfo.parseRatio(videoInfo.sar); - if (sar == null) return null; - return videoInfo.width / videoInfo.height * sar; - } - Widget _buildPreviewComparison(BuildContext context, MainViewModel viewModel) { - // The source's display aspect, for SAR correction of the "before" - // frame: DAR = (width / height) * SAR. Null for square pixels. - final sourceAspect = _sourceDisplayAspect(viewModel.videoInfo); - // The scope reads the processed preview, falling back to the source frame // before any pass has rendered — that is what the user is looking at. final scopeImage = viewModel.processedPreview ?? viewModel.currentFrame; @@ -75,15 +61,10 @@ class _PreviewPanelState extends State { afterImage: scopeImage, isBeforeLoading: viewModel.isAnalyzing, isAfterLoading: viewModel.isGeneratingPreview, - beforeAspectRatio: sourceAspect, - // The processed frame is drawn at the shape the encode will - // have, which a resize, pad, crop, rotation or squaring makes - // different from the source's (issue #149). Until a processed - // frame exists the source frame stands in for it, at its own - // aspect. - afterAspectRatio: viewModel.processedPreview != null - ? viewModel.processedPreviewAspect - : sourceAspect, + // Each side at its own shape: the processed frame at the + // encode's, not the source's (issue #149). + beforeAspectRatio: viewModel.sourceDisplayAspect, + afterAspectRatio: viewModel.comparisonAfterAspect, zoomController: viewModel.previewZoom, ), ), diff --git a/app/test/integration_aspect_ratio_test.dart b/app/test/integration_aspect_ratio_test.dart index c6d53ed3..4e6f1212 100644 --- a/app/test/integration_aspect_ratio_test.dart +++ b/app/test/integration_aspect_ratio_test.dart @@ -21,6 +21,8 @@ import 'package:uuid/uuid.dart'; import 'package:vapourbox/models/crop_resize_parameters.dart'; import 'package:vapourbox/models/encoding_settings.dart'; +import 'package:vapourbox/models/geometry_parameters.dart'; +import 'package:vapourbox/models/noise_reduction_parameters.dart'; import 'package:vapourbox/models/preview_frame_info.dart'; import 'package:vapourbox/models/processing_pipeline.dart'; import 'package:vapourbox/models/qtgmc_parameters.dart'; @@ -50,15 +52,17 @@ VideoJob _job( CropResizeParameters cropResize, { required String input, required String inputSar, + ProcessingPipeline? pipeline, }) => VideoJob( id: const Uuid().v4(), inputPath: input, outputPath: '$_outDir/$name.mkv', - processingPipeline: ProcessingPipeline( - deinterlace: const QTGMCParameters(enabled: false), - cropResize: cropResize, - ), + processingPipeline: pipeline ?? + ProcessingPipeline( + deinterlace: const QTGMCParameters(enabled: false), + cropResize: cropResize, + ), encodingSettings: const EncodingSettings( codec: VideoCodec.h264, container: ContainerFormat.mkv, @@ -78,8 +82,10 @@ Future<({Uint8List png, PreviewFrameInfo info, double? aspect})> _preview( CropResizeParameters cropResize, { required String input, required String inputSar, + ProcessingPipeline? pipeline, }) async { - final job = _job(name, cropResize, input: input, inputSar: inputSar); + final job = _job(name, cropResize, + input: input, inputSar: inputSar, pipeline: pipeline); final result = await WorkerHarness.runPreview(job.toJson(), frame: 2, label: name); expect(result.success, isTrue, reason: '$name: $result'); @@ -107,15 +113,17 @@ Future<({int width, int height, String sar, String dar})> _encode( CropResizeParameters cropResize, { String? input, String inputSar = '16:11', + ProcessingPipeline? pipeline, }) async { final job = VideoJob( id: const Uuid().v4(), inputPath: input ?? _anamorphic, outputPath: '$_outDir/$name.mkv', - processingPipeline: ProcessingPipeline( - deinterlace: const QTGMCParameters(enabled: false), - cropResize: cropResize, - ), + processingPipeline: pipeline ?? + ProcessingPipeline( + deinterlace: const QTGMCParameters(enabled: false), + cropResize: cropResize, + ), encodingSettings: const EncodingSettings( codec: VideoCodec.h264, container: ContainerFormat.mkv, @@ -369,5 +377,50 @@ void main() { expect(preview.info.outputDar, closeTo(16 / 9, 1e-9)); expect(preview.aspect, closeTo(16 / 9, 1e-9)); }, timeout: const Timeout(Duration(minutes: 5))); + + // The everyday case, and the one the fix could break: an anamorphic + // source that no pass reshapes. Before #149 both sides were drawn at the + // source's aspect without asking; now the processed side relies on the + // worker's report, so it must come out the source's shape. + test('an anamorphic source only denoised keeps its shape', () async { + final pipeline = ProcessingPipeline( + deinterlace: const QTGMCParameters(enabled: false), + noiseReduction: + NoiseReductionParameters.fromPreset(NoiseReductionPreset.light), + ); + const none = CropResizeParameters(); + final preview = await _preview('preview_149_denoise', none, + input: _anamorphic, inputSar: '16:11', pipeline: pipeline); + expect(encodedImageSize(preview.png), Size(_srcWidth.toDouble(), _srcHeight.toDouble())); + expect(preview.info.outputSar, closeTo(16 / 11, 1e-9)); + final out = await _encode('aspect_149_denoise', none, + input: _anamorphic, inputSar: '16:11', pipeline: pipeline); + const source = _srcWidth * _srcSar / _srcHeight; + expect(_displayAspect(out), closeTo(source, 0.01)); + expect(preview.aspect, closeTo(source, 0.01), + reason: 'with nothing reshaping it the processed frame must be ' + 'drawn at the source\'s display aspect, as it was before #149'); + }, timeout: const Timeout(Duration(minutes: 5))); + + // A quarter turn swaps the stored width and height and inverts the SAR: + // 720x576 at 16:15 (4:3) becomes 576x720 at 15:16 (3:4). + test('a rotated anamorphic source is drawn at the encode\'s shape', + () async { + final pipeline = ProcessingPipeline( + deinterlace: const QTGMCParameters(enabled: false), + geometry: + const GeometryParameters(enabled: true, rotation: Rotation.cw90), + ); + const none = CropResizeParameters(); + final preview = await _preview('preview_149_rotate', none, + input: _pal43, inputSar: '16:15', pipeline: pipeline); + expect(encodedImageSize(preview.png), Size(_srcHeight.toDouble(), _srcWidth.toDouble())); + expect(preview.info.outputSar, closeTo(15 / 16, 1e-9)); + final out = await _encode('aspect_149_rotate', none, + input: _pal43, inputSar: '16:15', pipeline: pipeline); + expect((out.width, out.height), (_srcHeight, _srcWidth)); + expect(_displayAspect(out), closeTo(3 / 4, 0.01)); + expect(preview.aspect, closeTo(_displayAspect(out), 0.01)); + }, timeout: const Timeout(Duration(minutes: 5))); }); } diff --git a/app/test/preview_aspect_test.dart b/app/test/preview_aspect_test.dart index 56555fc9..33108a8c 100644 --- a/app/test/preview_aspect_test.dart +++ b/app/test/preview_aspect_test.dart @@ -26,6 +26,8 @@ Uint8List _pngHeader(int width, int height) { } void main() { + group('comparison aspects through the view model', viewModelAspects); + group('processedDisplayAspect', () { final canvas = _pngHeader(1920, 1080); @@ -68,3 +70,81 @@ void main() { }); }); } + +/// What the comparison panel draws each side at, through the view model it +/// reads — so dropping the stored aspect, or computing it from the frame +/// info (null once the scrubber moves), fails here. +void viewModelAspects() { + TestWidgetsFlutterBinding.ensureInitialized(); + + late MainViewModel vm; + setUp(() => vm = MainViewModel()); + tearDown(() => vm.dispose()); + + // NTSC DVD: 720x480 with 32:27 pixels is 16:9 on screen. + const dvd = 720 / 480 * 32 / 27; + + test('an anamorphic source with no reshaping pass: both sides 16:9', () { + vm.debugAddQueueItemForTest('/tmp/vb_dvd.mkv', + width: 720, height: 480, sar: '32:27'); + expect(vm.sourceDisplayAspect, closeTo(dvd, 1e-9)); + + // A denoise-only pipeline: the worker re-stamps the source's SAR. + vm.debugAcceptProcessedPreview(_pngHeader(720, 480), + PreviewFrameInfo.parse('PREVIEW_INFO:frame=0 sar=32/27')); + expect(vm.comparisonAfterAspect, closeTo(dvd, 1e-9), + reason: 'the processed frame must keep the source\'s shape when ' + 'nothing reshapes it'); + }); + + test('before a processed frame exists the after side is the source\'s', + () { + vm.debugAddQueueItemForTest('/tmp/vb_dvd.mkv', + width: 720, height: 480, sar: '32:27'); + expect(vm.processedPreview, isNull); + expect(vm.comparisonAfterAspect, closeTo(dvd, 1e-9)); + }); + + test('square-pixel source: no correction on either side', () { + vm.debugAddQueueItemForTest('/tmp/vb_sq.mkv', width: 1920, height: 1080); + expect(vm.sourceDisplayAspect, isNull); + vm.debugAcceptProcessedPreview(_pngHeader(1920, 1080), + PreviewFrameInfo.parse('PREVIEW_INFO:frame=0')); + expect(vm.comparisonAfterAspect, closeTo(16 / 9, 1e-9)); + }); + + test('a reshaped frame is drawn at its own shape, not the source\'s', () { + vm.debugAddQueueItemForTest('/tmp/vb_pal.mkv', + width: 720, height: 576, sar: '16:15'); + vm.debugAcceptProcessedPreview(_pngHeader(1920, 1080), + PreviewFrameInfo.parse('PREVIEW_INFO:frame=0')); + expect(vm.sourceDisplayAspect, closeTo(4 / 3, 1e-9)); + expect(vm.comparisonAfterAspect, closeTo(16 / 9, 1e-9)); + }); + + test('the processed aspect survives a scrubber move', () async { + vm.debugAddQueueItemForTest('/tmp/vb_pal.mkv', + width: 720, height: 576, sar: '16:15'); + // Rendered for frame 7; the scrubber then sits somewhere else (with no + // video loaded every position maps to frame 0). + vm.debugAcceptProcessedPreview(_pngHeader(1920, 1080), + PreviewFrameInfo.parse('PREVIEW_INFO:frame=7')); + + // The image stays on screen until the next preview replaces it, but the + // frame info no longer matches the scrubber's frame. + await vm.setScrubberPosition(0.5); + expect(vm.previewFrameInfo, isNull); + expect(vm.processedPreview, isNotNull); + expect(vm.comparisonAfterAspect, closeTo(16 / 9, 1e-9)); + }); + + test('a failed preview clears the stored aspect with the image', () { + vm.debugAddQueueItemForTest('/tmp/vb_pal.mkv', + width: 720, height: 576, sar: '16:15'); + vm.debugAcceptProcessedPreview(_pngHeader(1920, 1080), + PreviewFrameInfo.parse('PREVIEW_INFO:frame=0')); + vm.debugAcceptProcessedPreview(null, null); + expect(vm.processedPreviewAspect, isNull); + expect(vm.comparisonAfterAspect, closeTo(4 / 3, 1e-9)); + }); +}