Unify background handling under a single :background option#213
Open
hlindset wants to merge 1 commit into
Open
Unify background handling under a single :background option#213hlindset wants to merge 1 commit into
:background option#213hlindset wants to merge 1 commit into
Conversation
1fcdda0 to
9068de1
Compare
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
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.
First of all: Very open for input/suggestions/discussions on the choices made in this PR!
In an earlier iteration of the PR, I removed libvips' default fill (the zero-fill) and instead leaned on using
background: :transparent+extend_mode: :background, but that caused some inconsistency with e.g. CMYK on transform ops vs flatten/write, so I reversed course and instead standardized on default/background: nilmeaning libvips' default zero-fill, which resolved the inconsistency. The only wrinkle is that libvips' default is only accessible by not setting:background(or explicitly setting it tonil).Generally
:background_color/:background_transparencyoptions into a single:backgroundoption, resolved viaImage.BackgroundColor.resolve/2everywhere (embed, flatten, join, rotate, affine, translate, shear, warp_perspective, write):average) in a tuple, like so:{color, alpha: a}, whereais0..255 | 0.0..1.0 | :opaque | :transparent(theImage.Pixel.transparencytype)background: nilmeans "unset" everywhere (falls back to the op's default).Defaults
:backgrounddefers to libvips' native all-zeros fill everywhere (embed, join, rotate, affine, translate, shear, flatten, write): transparent on images with an alpha band, black otherwise (white in CMYK). Keepsflatten() |> write()andwrite()consistent.warp_perspectivedefaults to:black(it flattens before warping)extend_modeembed:copy | :repeat | :mirror | :background:black/:whiteremoved: use an explicit:backgroundinstead. They're somewhat confusing anyway, e.g.:blackactually being white in CMYK.:copy/:repeat/:mirror) consumes no color, so combining one with a non-nil:backgroundis contradictory and returns an erroraffine/shear/translate/warp_perspectiveExtend here only affects the antialiased fringe at the content edge. The uncovered canvas is always filled with
:background.extend_mode: :backgroundis the option that makes sense most of the time.:background | :copyextend_mode: :background(default) blends the fringe into the fill, which is right whenever canvas is exposed:copyclamps to content, which can be right e.g. when the content fills the whole canvas, where usingextend_mode: :backgroundwould leave a faint border along the edge:repeat/:mirrormodes:repeatbleeds the opposite edge into the fringe:mirroris pixel-identical to:copyfor the fringerotatedoesn't accept:extend_mode. Both of its internal paths blend the fringe toward:background.Breaking changes
:background_color/:background_transparency→:background(embed, flatten, join)embedon an alpha image now defaults to a transparent border (was opaque black)embed's:black/:whiteextend modes removedaffine-family:extend_modenarrowed to:background | :copyflattenwith no:backgroundnow defers to libvips' native fill instead of:black: identical for sRGB, paper-white instead of ink-black for CMYK (aligningflatten() |> write()withwrite())embedwith content:extend_mode(:copy/:repeat/:mirror) plus a non-nil:backgroundnow errorsP.S.
warp_perspective/straighten_perspectivecurrently flatten. I can have a look at solving it similarly toaffinewith premultiply/unpremultiply to support transparency through the transform in a follow-up PR, resolving the one exception left after this PR