refactor(media): image-egress review follow-up — value objects, probe result, dedup#1348
Merged
Merged
Conversation
…probe result, dedup
Follow-up to the image-egress PR:
- Wrap the normalizer's bare ints in tidy value objects so px and bytes can't be
confused at a call site: Pixels, ByteSize, JpegQuality. ByteSize.ToString folds
in the human B/KB/MB formatting (replacing the FormatBudget helper).
- ImageNormalizerProbe returns a real ImagingProbeResult { IsWorking, Error }
instead of null-as-success.
- Dedup the "write DataContent -> add reference or append [image omitted] note"
logic that was copy-pasted in ChannelPipeline and ChatMessageConverter into one
SessionMediaStore.WriteMediaInto helper; AppendOmittedImageNote is now private.
- Comments: spell out what hits the "not a decodable image" drop; leave a
breadcrumb where an iterative size-reduction loop would go if over-budget drops
get common; note in Tests.Utilities.csproj that Win/macOS native assets come
transitively from base SkiaSharp (only Linux needs the explicit add).
Media tests 65, Actors 2211; solution builds clean; slopwatch + headers pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1345, addressing the review comments.
What's in here
Value objects (your
ImageNormalization.cs:54note). The options/result records used bareintfor both pixels and bytes, which is exactly the kind of thing a future reader — human or LLM — will mix up. Wrapped them in tidy little structs:Pixels,ByteSize,JpegQuality. No implicit conversions (you go through.Value), so you can't pass a byte count where a pixel cap is expected.ByteSize.ToString()also absorbed the human B/KB/MB formatting, so the oldFormatBudgethelper is gone and the drop messages just interpolate the value object.Real result type for the probe (
ImageNormalizerProbe.cs:23).TryProbe()returnedstring?with null meaning success, which is the null-as-sentinel smell you flagged. It now returnsImagingProbeResult { IsWorking, Error }, and the doctor check reads.IsWorking/.Error.Dedup (
ChatMessageConverter.cs:163— "why is this duplicated?"). Good catch. The "write theDataContent, add a reference or append an[image omitted]note" block was copy-pasted inChannelPipelineandChatMessageConverter. Pulled it into a singleSessionMediaStore.WriteMediaInto(...)so there's one owner of that fold;AppendOmittedImageNoteis now private since nothing outside calls it directly.Comments.
SkiaImageNormalizer.cs:46("what image types are not decodeable?") — spelled it out: corrupt/truncated files, non-image bytes wearing an image MIME, or formats Skia doesn't decode (HEIC/AVIF without a platform codec, SVG). JPEG/PNG/WebP/GIF/BMP all decode fine.SkiaImageNormalizer.cs:90— left the breadcrumb you asked for: we drop an over-budget-after-resize image rather than shrink it further, and if those drops get common the place to reintroduce a bounded size-reduction loop is right there.Tests.Utilities.csproj:14("what about Windows and OS X?") — they come transitively from the baseSkiaSharppackage (it depends onNativeAssets.Win32/NativeAssets.macOS); desktop Linux is the only one not pulled in by default, hence the explicit add. Noted it in the csproj. CI already proved it — Test-windows-latest and Test-macos-26 both passed on fix(media): resize images once at ingest instead of re-inlining them every turn (#1296) #1345.Tests
Netclaw.Media.Tests(65) — added aByteSize.ToStringformatting test (guards the sub-MiB "0MB" case), updated the options/probe call sites.Netclaw.Actors.Tests(2211) — the seam and converter tests cover the deduped path.No behavior change — this is the tidy-up pass.