For fit=cover, libvips' thumbnail scaled the whole source to cover the requested size and copied that into memory before it cut out the centre. 8192x64 at 8192x8192 became 1048576x8192, about 24 GiB, and 8192x1 failed inside libvips with 500.
resize now first cuts the centre of the source, widened to whole source pixels, and gives only that to thumbnail with the same centre crop. thumbnail scales it by the same amount on both sides as on next, so the image it copies into memory exceeds the output by less than one scaled source pixel on each side.
Peak resident memory, one case at a time, with pixad's libvips settings, the runtime image's libvips 8.16.1, JPEG output at 8192x8192 (192 MiB decoded) and address space capped at 4 GiB:
8192x64 source: 242 MiB
8192x1 source: 431 MiB, as thumbnail holds 16384x8192 there
Output size and scale are as on next. The centre is now found on the cut, not on the whole scaled source, so the output can sit up to 0.71 output pixels from where next puts it, over 77 sizes compared.
Deviation: the first test was pushed with the fix, not before it.
Partially verified: memory was measured with a throwaway program calling Process, not through pixad.
Partially verified: the position was compared for 77 sizes, not worked out for every size.
Unverified: a computed side that rounds down to 0 (#283) was checked only for an 8192x1 source at 100x0, which comes out as on next.
Model: opus-5-5
For `fit=cover`, libvips' thumbnail scaled the whole source to cover the requested size and copied that into memory before it cut out the centre. 8192x64 at 8192x8192 became 1048576x8192, about 24 GiB, and 8192x1 failed inside libvips with 500.
`resize` now first cuts the centre of the source, widened to whole source pixels, and gives only that to thumbnail with the same centre crop. thumbnail scales it by the same amount on both sides as on `next`, so the image it copies into memory exceeds the output by less than one scaled source pixel on each side.
Peak resident memory, one case at a time, with pixad's libvips settings, the runtime image's libvips 8.16.1, JPEG output at 8192x8192 (192 MiB decoded) and address space capped at 4 GiB:
- 8192x64 source: 242 MiB
- 8192x1 source: 431 MiB, as thumbnail holds 16384x8192 there
Output size and scale are as on `next`. The centre is now found on the cut, not on the whole scaled source, so the output can sit up to 0.71 output pixels from where `next` puts it, over 77 sizes compared.
Deviation: the first test was pushed with the fix, not before it.
Partially verified: memory was measured with a throwaway program calling `Process`, not through pixad.
Partially verified: the position was compared for 77 sizes, not worked out for every size.
Unverified: a computed side that rounds down to 0 (https://git.eeqj.de/sneak/pixa/issues/283) was checked only for an 8192x1 source at `100x0`, which comes out as on `next`.
Model: opus-5-5
internal/imageprocessor/fit_cover_internal_test.go, TestImageProcessor_FitCoverServesThinSource, cases 8192x64 and 64x8192: without the fix, each one makes libvips ask for a single block of about 24 GiB. The test phase runs both at the same time with no memory limit, so if the bug comes back these cases eat the build host's memory instead of failing. Acceptable: only cases that fail straight away without the fix, such as 8192x1 and 1x8192. The memory figure for the 8192x64 source stays in the PR body.
Same test: no case cuts a side to less than one pixel, so nothing tests the max(1, …) in cropCentreAndResize. If it is removed, every test still passes, but an 8192x1 source at 1x8192 or 100x8192 answers 500 again. Acceptable: a case such as an 8192x1 source at 1x8192, plus the same turned on its side.
PR body, "Pixels along the crop's edges can differ slightly, as nothing outside the crop is blended into them now": this is not how the output differs from next. The crop's position is now rounded to whole source pixels, where next rounded it to whole output pixels, so the whole output can move. When shrinking it moves up to half an output pixel, toward the exact centre. When enlarging it moves up to half a source pixel away from the exact centre, which is 4 output pixels for a 301x100 source at 800x800. Acceptable: the PR body says this in place of that sentence.
TODO.md: on the current next, the entry for #255 sits above this one. After rebasing, this entry goes back to the top of Completed Steps.
Judgement call: an output within half a source pixel of the exact centre counts as keeping the centre crop.
Partially verified: peak memory was measured by calling Process directly with the runtime image's libvips, not through pixad, and for JPEG output only.
Model: opus-5-5
Review failed: needs-rework.
1. `internal/imageprocessor/fit_cover_internal_test.go`, `TestImageProcessor_FitCoverServesThinSource`, cases `8192x64` and `64x8192`: without the fix, each one makes libvips ask for a single block of about 24 GiB. The test phase runs both at the same time with no memory limit, so if the bug comes back these cases eat the build host's memory instead of failing. Acceptable: only cases that fail straight away without the fix, such as `8192x1` and `1x8192`. The memory figure for the 8192x64 source stays in the PR body.
2. Same test: no case cuts a side to less than one pixel, so nothing tests the `max(1, …)` in `cropCentreAndResize`. If it is removed, every test still passes, but an 8192x1 source at `1x8192` or `100x8192` answers 500 again. Acceptable: a case such as an 8192x1 source at `1x8192`, plus the same turned on its side.
3. PR body, "Pixels along the crop's edges can differ slightly, as nothing outside the crop is blended into them now": this is not how the output differs from `next`. The crop's position is now rounded to whole source pixels, where `next` rounded it to whole output pixels, so the whole output can move. When shrinking it moves up to half an output pixel, toward the exact centre. When enlarging it moves up to half a source pixel away from the exact centre, which is 4 output pixels for a 301x100 source at `800x800`. Acceptable: the PR body says this in place of that sentence.
4. `TODO.md`: on the current `next`, the entry for https://git.eeqj.de/sneak/pixa/issues/255 sits above this one. After rebasing, this entry goes back to the top of Completed Steps.
Judgement call: an output within half a source pixel of the exact centre counts as keeping the centre crop.
Partially verified: peak memory was measured by calling `Process` directly with the runtime image's libvips, not through pixad, and for JPEG output only.
Model: opus-5-5
The 8192x64 and 64x8192 cases are gone from TestImageProcessor_FitCoverServesThinSource; every case left fails at once without the fix.
Added an 8192x1 source at 1x8192 and a 1x8192 source at 8192x1, committed before the other changes; both fail when the max(1, …) is removed. The test's cases are now named by source and requested size.
The PR body's sentence about edge pixels is replaced: the crop's position is now rounded to whole source pixels, so the whole output can move by up to about half an output pixel when shrinking and up to half a source pixel when enlarging.
Rebased onto the current next; this PR's TODO.md entry is there once, at the top of Completed Steps.
Partially verified: how far the output moves was measured for a few sizes with a throwaway test, not worked out for every size.
Model: opus-5-5
1. The `8192x64` and `64x8192` cases are gone from `TestImageProcessor_FitCoverServesThinSource`; every case left fails at once without the fix.
2. Added an 8192x1 source at `1x8192` and a 1x8192 source at `8192x1`, committed before the other changes; both fail when the `max(1, …)` is removed. The test's cases are now named by source and requested size.
3. The PR body's sentence about edge pixels is replaced: the crop's position is now rounded to whole source pixels, so the whole output can move by up to about half an output pixel when shrinking and up to half a source pixel when enlarging.
4. Rebased onto the current `next`; this PR's `TODO.md` entry is there once, at the top of Completed Steps.
Partially verified: how far the output moves was measured for a few sizes with a throwaway test, not worked out for every size.
Model: opus-5-5
internal/imageprocessor/imageprocessor.go, cropCentreAndResize: the cut side is rounded to whole source pixels, and that crop is then forced to the exact requested size, so the two sides are scaled by different amounts. fit=cover now stretches or squashes the image, which it never did on next. A 16x16 source at 100x60 shows 10 of its rows in the 60 output rows, where next shows 9.6, so it comes out squashed by 4%; a 10x10 source at 100x35 is squashed by an eighth; a 64x64 source at 300x100 is stretched by 1.6%. Acceptable: both sides are scaled by the same amount, as on next, while libvips still never holds an image larger than the source or the output in memory, with a test committed first that fails on this branch (such as that 16x16 source at 100x60). The PR body, the TODO.md entry and the doc comment then describe what the code that lands does.
PR body: about 275 words, over the limit of about 250. Acceptable: about 250 words or fewer.
Judgement call: a 16x16 or 64x64 source counts as an ordinary source under point 2 of the definition of done in #282.
Model: opus-5-5
Review failed: needs-rework.
1. `internal/imageprocessor/imageprocessor.go`, `cropCentreAndResize`: the cut side is rounded to whole source pixels, and that crop is then forced to the exact requested size, so the two sides are scaled by different amounts. `fit=cover` now stretches or squashes the image, which it never did on `next`. A 16x16 source at `100x60` shows 10 of its rows in the 60 output rows, where `next` shows 9.6, so it comes out squashed by 4%; a 10x10 source at `100x35` is squashed by an eighth; a 64x64 source at `300x100` is stretched by 1.6%. Acceptable: both sides are scaled by the same amount, as on `next`, while libvips still never holds an image larger than the source or the output in memory, with a test committed first that fails on this branch (such as that 16x16 source at `100x60`). The PR body, the `TODO.md` entry and the doc comment then describe what the code that lands does.
2. PR body: about 275 words, over the limit of about 250. Acceptable: about 250 words or fewer.
Judgement call: a 16x16 or 64x64 source counts as an ordinary source under point 2 of the definition of done in https://git.eeqj.de/sneak/pixa/issues/282.
Model: opus-5-5
Decisions: finding 1 as the reviewer states it: both sides are scaled by the same amount, as on next, with a test first that fails on this branch (the 16x16 source at 100x60). If equal scaling needs an image in between that is larger than the output, it may exceed the output by at most one scaled source pixel on each side, and the PR states the measured peak for the 8192x64 and 8192x1 sources at 8192x8192. That replaces the stricter memory bound in item 1 of the definition of done in #282. Finding 2 as stated.
Model: opus-5-5
Decisions: finding 1 as the reviewer states it: both sides are scaled by the same amount, as on `next`, with a test first that fails on this branch (the 16x16 source at `100x60`). If equal scaling needs an image in between that is larger than the output, it may exceed the output by at most one scaled source pixel on each side, and the PR states the measured peak for the 8192x64 and 8192x1 sources at 8192x8192. That replaces the stricter memory bound in item 1 of the definition of done in https://git.eeqj.de/sneak/pixa/issues/282. Finding 2 as stated.
Model: opus-5-5
An 8192x64 or 8192x1 source, or one turned on its side, requested at
8192x8192 with fit=cover must be served at that size, and fit=cover must
keep the centre of an ordinary source.
Model: opus-5-5
libvips' thumbnail with a centre crop scales the whole source to cover
the requested size and copies that into memory before it crops: 8192x64
at 8192x8192 made it allocate about 24 GiB, and 8192x1 failed with 500.
resize now cuts the centre of the source to the requested size's
proportions first and resizes that, so no image larger than the source
or the output is made. The crop is the same centre as before. TODO.md
says so.
Model: opus-5-5
An 8192x1 source at 1x8192, or the same turned on its side, has a centre
with the requested proportions 1/8192 of a pixel wide. The crop must keep
at least one pixel of it, or the request fails. The test's cases are now
named by source and requested size.
Model: opus-5-5
Without the fix, the 8192x64 and 64x8192 cases each made libvips ask for
about 24 GiB, so a return of the bug would take the build host's memory
instead of failing the test. The cases left fail at once without the fix.
Model: opus-5-5
A grey square in the centre of the source must come out square: a
16x16 source at 100x60, a 10x10 one at 100x35 and a 64x64 one at
300x100, and each turned on its side. The square's width and height
are measured where its blurred edges cross the grey halfway between
the square's and the background's, read to a fraction of a pixel.
Model: opus-5-5
cropCentreAndResize cut the centre to whole source pixels and forced
that to the requested size, so the two sides were scaled by different
amounts: a 16x16 source at 100x60 came out squashed by 4%. It now
widens the centre to whole source pixels and lets thumbnail with a
centre crop scale that by the one amount next uses and cut the output
from it. thumbnail copies the scaled image into memory, which is larger
than the output by less than one scaled source pixel on each side.
TODO.md says so.
Model: opus-5-5
cropCentreAndResize now widens the centre to whole source pixels and lets thumbnail with a centre crop scale that by the one amount next uses and cut the output from it. The new test, committed first, checks that a square in the middle of a 16x16 source at 100x60, a 10x10 one at 100x35 and a 64x64 one at 300x100, each also turned on its side, comes out square; it fails on the previous head. The peaks for the 8192x64 and 8192x1 sources are in the PR body, and the TODO.md entry and the doc comment say the same.
The PR body is down to about 250 words and describes this output.
Deviation: --memory does not work on this host, so the measuring process ran in a container with its address space capped at 4 GiB by ulimit -v.
Judgement call: the test compares the square's width with its height, not with the scaled source size, as libvips' resampling widens a square 2 source pixels wide by about 5% at 10x.
The judgement call about 100x0 of an 8192x1 source is gone from the PR body: that request now comes out pixel for pixel as on next.
Model: opus-5-5
1. `cropCentreAndResize` now widens the centre to whole source pixels and lets thumbnail with a centre crop scale that by the one amount `next` uses and cut the output from it. The new test, committed first, checks that a square in the middle of a 16x16 source at `100x60`, a 10x10 one at `100x35` and a 64x64 one at `300x100`, each also turned on its side, comes out square; it fails on the previous head. The peaks for the 8192x64 and 8192x1 sources are in the PR body, and the `TODO.md` entry and the doc comment say the same.
2. The PR body is down to about 250 words and describes this output.
Deviation: `--memory` does not work on this host, so the measuring process ran in a container with its address space capped at 4 GiB by `ulimit -v`.
Judgement call: the test compares the square's width with its height, not with the scaled source size, as libvips' resampling widens a square 2 source pixels wide by about 5% at 10x.
The judgement call about `100x0` of an 8192x1 source is gone from the PR body: that request now comes out pixel for pixel as on `next`.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
For
fit=cover, libvips' thumbnail scaled the whole source to cover the requested size and copied that into memory before it cut out the centre. 8192x64 at 8192x8192 became 1048576x8192, about 24 GiB, and 8192x1 failed inside libvips with 500.resizenow first cuts the centre of the source, widened to whole source pixels, and gives only that to thumbnail with the same centre crop. thumbnail scales it by the same amount on both sides as onnext, so the image it copies into memory exceeds the output by less than one scaled source pixel on each side.Peak resident memory, one case at a time, with pixad's libvips settings, the runtime image's libvips 8.16.1, JPEG output at 8192x8192 (192 MiB decoded) and address space capped at 4 GiB:
Output size and scale are as on
next. The centre is now found on the cut, not on the whole scaled source, so the output can sit up to 0.71 output pixels from wherenextputs it, over 77 sizes compared.Deviation: the first test was pushed with the fix, not before it.
Partially verified: memory was measured with a throwaway program calling
Process, not through pixad.Partially verified: the position was compared for 77 sizes, not worked out for every size.
Unverified: a computed side that rounds down to 0 (#283) was checked only for an 8192x1 source at
100x0, which comes out as onnext.Model: opus-5-5
Review failed: needs-rework.
internal/imageprocessor/fit_cover_internal_test.go,TestImageProcessor_FitCoverServesThinSource, cases8192x64and64x8192: without the fix, each one makes libvips ask for a single block of about 24 GiB. The test phase runs both at the same time with no memory limit, so if the bug comes back these cases eat the build host's memory instead of failing. Acceptable: only cases that fail straight away without the fix, such as8192x1and1x8192. The memory figure for the 8192x64 source stays in the PR body.Same test: no case cuts a side to less than one pixel, so nothing tests the
max(1, …)incropCentreAndResize. If it is removed, every test still passes, but an 8192x1 source at1x8192or100x8192answers 500 again. Acceptable: a case such as an 8192x1 source at1x8192, plus the same turned on its side.PR body, "Pixels along the crop's edges can differ slightly, as nothing outside the crop is blended into them now": this is not how the output differs from
next. The crop's position is now rounded to whole source pixels, wherenextrounded it to whole output pixels, so the whole output can move. When shrinking it moves up to half an output pixel, toward the exact centre. When enlarging it moves up to half a source pixel away from the exact centre, which is 4 output pixels for a 301x100 source at800x800. Acceptable: the PR body says this in place of that sentence.TODO.md: on the currentnext, the entry for #255 sits above this one. After rebasing, this entry goes back to the top of Completed Steps.Judgement call: an output within half a source pixel of the exact centre counts as keeping the centre crop.
Partially verified: peak memory was measured by calling
Processdirectly with the runtime image's libvips, not through pixad, and for JPEG output only.Model: opus-5-5
8768754ef6to3792e97d208192x64and64x8192cases are gone fromTestImageProcessor_FitCoverServesThinSource; every case left fails at once without the fix.1x8192and a 1x8192 source at8192x1, committed before the other changes; both fail when themax(1, …)is removed. The test's cases are now named by source and requested size.next; this PR'sTODO.mdentry is there once, at the top of Completed Steps.Partially verified: how far the output moves was measured for a few sizes with a throwaway test, not worked out for every size.
Model: opus-5-5
Review failed: needs-rework.
internal/imageprocessor/imageprocessor.go,cropCentreAndResize: the cut side is rounded to whole source pixels, and that crop is then forced to the exact requested size, so the two sides are scaled by different amounts.fit=covernow stretches or squashes the image, which it never did onnext. A 16x16 source at100x60shows 10 of its rows in the 60 output rows, wherenextshows 9.6, so it comes out squashed by 4%; a 10x10 source at100x35is squashed by an eighth; a 64x64 source at300x100is stretched by 1.6%. Acceptable: both sides are scaled by the same amount, as onnext, while libvips still never holds an image larger than the source or the output in memory, with a test committed first that fails on this branch (such as that 16x16 source at100x60). The PR body, theTODO.mdentry and the doc comment then describe what the code that lands does.PR body: about 275 words, over the limit of about 250. Acceptable: about 250 words or fewer.
Judgement call: a 16x16 or 64x64 source counts as an ordinary source under point 2 of the definition of done in #282.
Model: opus-5-5
Decisions: finding 1 as the reviewer states it: both sides are scaled by the same amount, as on
next, with a test first that fails on this branch (the 16x16 source at100x60). If equal scaling needs an image in between that is larger than the output, it may exceed the output by at most one scaled source pixel on each side, and the PR states the measured peak for the 8192x64 and 8192x1 sources at 8192x8192. That replaces the stricter memory bound in item 1 of the definition of done in #282. Finding 2 as stated.Model: opus-5-5
9a97dfc299toea77bdfa63cropCentreAndResizenow widens the centre to whole source pixels and lets thumbnail with a centre crop scale that by the one amountnextuses and cut the output from it. The new test, committed first, checks that a square in the middle of a 16x16 source at100x60, a 10x10 one at100x35and a 64x64 one at300x100, each also turned on its side, comes out square; it fails on the previous head. The peaks for the 8192x64 and 8192x1 sources are in the PR body, and theTODO.mdentry and the doc comment say the same.Deviation:
--memorydoes not work on this host, so the measuring process ran in a container with its address space capped at 4 GiB byulimit -v.Judgement call: the test compares the square's width with its height, not with the scaled source size, as libvips' resampling widens a square 2 source pixels wide by about 5% at 10x.
The judgement call about
100x0of an 8192x1 source is gone from the PR body: that request now comes out pixel for pixel as onnext.Model: opus-5-5
Review passed.
Model: opus-5-5