Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pipelineColourspace operator #2704

Closed
wants to merge 1 commit into from

Conversation

Daiz
Copy link
Contributor

@Daiz Daiz commented May 8, 2021

This pull request adds a new operator: pipelineColourspace (and a pairing alias pipelineColorspace). You can use it like such:

sharp(input)
  .pipelineColourspace('rgb16')
  .resize(320, 240)
  .gamma()
  .toColourspace('srgb') // this is the default, but included here for clarity
  .toBuffer();

Currently, sharp runs the pipeline in the colorspace of the input image, which means that if you feed sharp an image with 8 bits per channel (bpc), it will process the image in 8bpc, and similarly with 16bpc input the pipeline will run in 16bpc. This operator provides control over the pipeline colorspace independent of the input image, allowing the user to easily run the pipeline in 16bpc even with 8bpc input. This is relevant eg. if you want to avoid banding issues when using the gamma operator.

While this implementation is not exactly what was proposed in #1317, it fulfils the goals of it with the control it provides - you could do .pipelineColourspace('rgb16') or .pipelineColourspace('scrgb') depending on your exact needs. This is relevant because of the performance impact a given choice can have:

Average runtime of the pipeline colourspace unit test (1000x1000 -> 320x320 resize with gamma) with different options:
srgb   (8bpc): ~57ms
rgb16 (16bpc): ~75ms (~1.3x slowdown)
scrgb (16bpc): ~2436ms (~42.7x slowdown)

Some other things of note with this PR:

  • Using pipelineColourspace currently disables fastShrinkOnLoad. This was pondered on in Enhancement: provide wideGamut option to process in 16-bit linear colourspace #1317 and it made sense to me as this is essentially very much a "quality over speed" operator.
  • I changed the libvips link URL in the .toColourspace JSDoc because it was pointing to the wrong thing due to changes in the libvips master branch since the link was first added. It now points to a static commit, which should keep the link more stable.
  • The gradient-rgb8.png test image was made by me, simulating an image where I first spotted the banding issue I was having with the gamma operator and an 8bpc pipeline.
  • I haven't done this as of yet, but I think it would be a good idea to add a recommendation to the gamma operator documentation about using .pipelineColourspace('rgb16') to avoid banding.
  • Enhancement: provide wideGamut option to process in 16-bit linear colourspace #1317 mentions "using an embedded input profile, if any, converted via the XYZ connection space" but I wasn't fully sure what it meant... I did do some manual testing with the Channel_digital_image_CMYK_color.jpg test file which has an embedded ICC profile, and using .pipelineColourspace('rgb16') did not seem to have any impact on color accuracy when dealing with it.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 99.899% when pulling 0f46348 on Daiz:feat/pipeline-colourspace into 070534d on lovell:master.

@Daiz
Copy link
Contributor Author

Daiz commented May 8, 2021

So the coverage report lists the pipelineColorspace alias as uncovered. Looking at the toColorspace alias, it seems this gets covered by a random usage of the alias in another test in joinChannel.js. This feels a bit janky to at least me though... perhaps the aliases could just be tested in the "invalid input" tests? Or have some sort of dedicated simple tests for them?


On another note, while all the tests pass with this branch since you have to explicitly opt-in to a potentially higher bit depth pipeline via the operator, I also decided out of curiosity to try what happens if I set rgb16 as the default pipelineColourspace and ran all the tests. The results: 847 passing, 48 failing. Things that seem to fail:

  • Boolean operations between two images:
    • all fail (too large similarity distances across the board)
  • Image channel extraction:
    • non-existent channel (assert(err instanceof Error) fails)
  • Image channel insertion:
    • Grayscale to RGB/RGBA/CMYK, raw buffers to RGB (info.channels has +2 channels compared to expected)
  • Linear adjustment:
    • linear levels w/ alpha, offset level w/ alpha (too large similarity distance)
  • Image metadata:
    • Apply CMYK output ICC profile (diff of 1 on max 0)
    • custom output ICC profile (diff of ~10 on max 9)
  • Modulate:
    • hue-rotate (returns {r: 40, g: 107, b: 57}, test expects r: 41)
    • darken (returns g: 16, test expects g: 17)
    • saturate (r: 199, b: 42, test expects r: 198, b: 43)
    • all channels: (b: 215, expects b: 214)
    • hue-rotate by degree (all except 360deg return similarity distances of ~64-66 rather than 1)
  • Negate:
    • false (similarity distance of ~2 rather than 0)
  • Gaussian noise:
    • overlaid on transparent png (reports 6 channels instead of 4)
  • Resize:
    • fastShrinkOnLoad: false ensures image is not shifted (diff of 23 on max distance of 5)
    • fastShrinkOnLoad: true might result in shifted image (diff of 29 on max distance of 5)
  • Rotation:
    • by 30 degrees on solid background (diff of 16 on max 5)
  • SVG Input:
    • SVG to PNG at 72DPI (diff of 9 on max 5)
  • Threshold:
    • color threshold (diff of 36 on max 5)
  • Trim borders:
    • skip shrink-on-load (assert.strictEqual(true, inRange(info.trimOffsetLeft, -873, -870)); fails)
    • single colour PNG where alpha channel provides the image (info.height of 138 instead of 137)
    • should rotate before trim (info.width of 22 instead of 20)
  • Utilities:
    • Counters have zero value at rest (12 instead of 0)

Looking at the overall results, I think the general outcome can be categorized into a few different pools:

  • Features that simply don't seem to work right with a 16bpc pipeline
    • There's plenty of tests with huge difference values, clearly indicating things going majorly wrong (I didn't inspect the visual results as of yet)
    • Whatever is going on with info.channels jumping to 5/6 instead of 3/4 in several places
    • Not sure what's going on with border trimming size differences?
    • There's potential for separate PRs in here IMO, as all of these would be affected even right now if you fed 16bpc input into sharp and tried to use those features - at the very least, it would probably be good for documentation to mention if a feature doesn't currently work right in 16bpc
  • Not necessarily "true" errors at all
    • eg. modulate functions besides hue rotation by degree
  • Things that could/should probably be looked into in this PR
    • Possibly the output ICC related errors? Since there's no ICC handling in the initial pipeline colourspace conversion as of now.
    • That resize fastShrinkOnLoad: false error could be a potential bug for this PR too? (The fastShrinkLoad: true test failing is to be expected, I think, since using pipelineColourspace is supposed to turn fastShrinkOnLoad off.)
    • Not sure what's going on with the trim borders shrink-on-load skip error either...
  • Also, counters not being 0 just seems weird. No idea what's up with that.

Someone more familiar with the codebase can probably weigh in on these, though I'd like to reiterate that all the failing tests I just talked about only pop up if you set pipelineColourspace('rgb16') as the default, which is not something that this PR does. I guess you could say that this was more of a preliminary investigation into how 16bpc compatible sharp internals currently are (although this might not still paint a fully accurate picture if some operations are "prematurely" converting to 8bpc as part of their implementation, I guess).

@lovell
Copy link
Owner

lovell commented May 11, 2021

Thank you very much for this PR, I will make some time for a proper review of the code and your detailed notes (thank you) as soon as I can.

@lovell
Copy link
Owner

lovell commented Jun 2, 2021

Quick update: I haven't forgotten this PR, but sadly have been rather busy during the last month to dedicate time for the review that this improvement deserves. ❤️

@lovell
Copy link
Owner

lovell commented Jul 9, 2021

Thanks again, I've finally made/found some time to review PRs and this all looks great.

I plan to merge this into the circle branch for inclusion in the v0.29.0 release as an experimental feature then we can start to fix up some of the tests that fail when using a 16-bit pipeline. (I'm hoping most of these will be relatively straightforward - the appearance of two extra channels seems like the most important to address.)

@lovell lovell added this to the v0.29.0 milestone Jul 9, 2021
lovell added a commit that referenced this pull request Jul 10, 2021
@lovell
Copy link
Owner

lovell commented Jul 10, 2021

Landed via commit 8ad02e6 - thank you very much for this, it will be part of v0.29.0.

@lovell lovell closed this Jul 10, 2021
@Daiz
Copy link
Contributor Author

Daiz commented Jul 10, 2021

Lovely! Looking forward to retiring my previous double pipeline hack in favor of this when 0.29 is out 🎉

lovell added a commit that referenced this pull request Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants