[WIP] Preserving isTrans data #764

devedse · 2018-10-30T23:42:22Z

Prerequisites

I have written a descriptive pull-request title
I have verified that there are no overlapping pull-requests open
I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

Currently still WIP but I wanted to contribute something to the project 😄. So I'm working on fixing my own issue:
#763

CLAassistant · 2018-10-30T23:42:28Z

All committers have signed the CLA.

devedse · 2018-10-30T23:48:31Z

I'm not sure if we want to implement it this way or have a look at the already existing Palette code which should be extended to always write alpha if the hasAlpha flag is true. But please let me know :)

Lowercase b

JimBobSquarePants · 2018-10-31T14:34:39Z

Hey @devedse this all looks great! Thanks for making the effort to fix this for us.

I'll pull it down for a final review but from looking on GitHub I can't see anything amiss.

In the interim could you please sign the CLA? Cheers! 👍

devedse · 2018-10-31T14:42:40Z

I think the bitconversion could use some improvements. But its not really my speciality.
Ill look at signing the ca

devedse · 2018-11-01T21:25:50Z

Signed 😄

antonfirsov

@JimBobSquarePants @devedse I have a poor understanding of PNG, so I need some explanation to understand everything, and have some ideas for defining proper test coverage:

When a color is defined as transparent, and we decode it to Rgba32 or similar, do we set the alpha to 255 when we encounter that color? (This would make the most sense to me!)
What is then happening in the encoder? Are we converting all colors set to (*,*,*,255) to the transparent color again?

src/ImageSharp/Formats/Png/PngMetaData.cs

JimBobSquarePants

Looking good with just a couple of changes.

Anton made some valid suggestion regarding using our new gray types and better variable naming. We should also check the bit depth as well as whether the property is not null.

src/ImageSharp/Formats/Png/PngEncoderCore.cs

src/ImageSharp/MetaData/ImageMetaData.cs

JimBobSquarePants · 2018-11-01T22:40:53Z

@antonfirsov To answer your questions regarding the tRNS chunk.

When decoding either grayscale, 24bit, or 48bit color types, if the chunk is encountered we decode to Rgba32 and Rgba64 pixel formats (we don't have a GrayAlpha pixel format) and set the alpha component to fully opaque for any pixel that does not match the value contained within the chunk.

When encoding we use the Rgb24, Rgb48, Gray8, or Gray16 pixel format layouts and assign a color that should be regarded as transparent to the chunk.

Does that make sense?

codecov · 2018-11-02T16:36:54Z

Codecov Report

Merging #764 into master will increase coverage by 0.05%.
The diff coverage is 95.37%.

@@            Coverage Diff             @@
##           master     #764      +/-   ##
==========================================
+ Coverage   88.51%   88.56%   +0.05%     
==========================================
  Files        1005     1005              
  Lines       42769    42846      +77     
  Branches     3149     3159      +10     
==========================================
+ Hits        37855    37948      +93     
+ Misses       4222     4203      -19     
- Partials      692      695       +3

Impacted Files	Coverage Δ
...eSharp.Tests/Formats/Png/PngDecoderTests.Chunks.cs	`100% <ø> (ø)`	⬆️
tests/ImageSharp.Tests/TestImages.cs	`100% <ø> (ø)`	⬆️
src/ImageSharp/Formats/Png/PngMetaData.cs	`100% <100%> (ø)`	⬆️
.../ImageSharp.Tests/Formats/Png/PngChunkTypeTests.cs	`100% <100%> (ø)`	⬆️
src/ImageSharp/Formats/Png/PngDecoderCore.cs	`82.53% <100%> (ø)`	⬆️
src/ImageSharp/Formats/Png/PngScanlineProcessor.cs	`63.28% <75%> (+3.84%)`	⬆️
src/ImageSharp/Formats/Png/PngEncoderCore.cs	`93.51% <90%> (-0.33%)`	⬇️
...ts/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs	`99.47% <97.36%> (-0.53%)`	⬇️
...ions/Generated/Rgba32.PixelOperations.Generated.cs	`100% <0%> (+9.57%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9d570f...6849617. Read the comment docs.

devedse · 2018-11-02T16:51:02Z

I've implemented almost all findings, please redo code review :)

antonfirsov · 2018-11-02T17:02:29Z

and set the alpha component to fully opaque for any pixel that does not match the value contained within the chunk

But what happens when we encounter pixels matching the value? Do we decode fully transparent pixels in those cases?
Do we alter transparent pixels to chunk-defined pixels when encoding?

antonfirsov

We should cover all 4 trancparency cases with tests for both decoder and encoder. Not yet sure whether we can base them on current tests or we should invent something new. (I'm still unsure whether I understand decoding/re-encoding of transparent pixels correctly.)

JimBobSquarePants · 2018-11-02T20:17:18Z

@antonfirsov

But what happens when we encounter pixels matching the value? Do we decode fully transparent pixels in those cases?

We set the alpha component to 0 leaving the other component data intact.

Do we alter transparent pixels to chunk-defined pixels when encoding?

No the tRNS chunk is simply a marker. Pixels are encoded as normal. Delete the tRNS chunk and you have an opaque image.

@devedse

We have some test images already that might cover your needs but there's an official suite here.

http://www.schaik.com/pngsuite/

devedse · 2018-11-04T10:31:53Z

@devedse

We have some test images already that might cover your needs but there's an official suite here.

http://www.schaik.com/pngsuite/

What do you mean by this? For writing test cases?

antonfirsov · 2018-11-04T10:38:34Z

@devedse input images for decoder tests.

JimBobSquarePants · 2018-11-22T17:22:19Z

@devedse I've updated the code to check for the correct bit depth and also reduce allocations.

Once we add the tests images from the collection I linked this is good to go.

JimBobSquarePants · 2018-11-23T13:44:11Z

Tests added and correct behaviour confirmed. I think this is good to go now.

Code and tests are now up to spec.

[WIP] Preserving isTrans data

Work in progress preserving isTrans data

b798d8d

devedse and others added 3 commits October 31, 2018 01:14

This should write transparency

e65fc54

Fixed bug, code now actually fixed my unit test

fae06ca

Update PngEncoderCore.cs

02a8d0d

Lowercase b

antonfirsov reviewed Nov 1, 2018

View reviewed changes

src/ImageSharp/Formats/Png/PngMetaData.cs Outdated Show resolved Hide resolved

src/ImageSharp/Formats/Png/PngMetaData.cs Outdated Show resolved Hide resolved

src/ImageSharp/Formats/Png/PngMetaData.cs Outdated Show resolved Hide resolved

JimBobSquarePants requested changes Nov 1, 2018

View reviewed changes

src/ImageSharp/Formats/Png/PngEncoderCore.cs Outdated Show resolved Hide resolved

src/ImageSharp/MetaData/ImageMetaData.cs Outdated Show resolved Hide resolved

devedse added 2 commits November 2, 2018 17:36

Removed unused using

a7aef83

Merge branch 'master' of https://github.com/devedse/ImageSharp

1438b96

Resolved findings in PR

6364b50

antonfirsov previously requested changes Nov 2, 2018

View reviewed changes

JimBobSquarePants added 4 commits November 12, 2018 11:42

Merge branch 'master' into master

9fdd90d

Merge branch 'master' into master

dafbe94

Reduce allocatoins, check bit depth and rename.

f79f189

Use existing field

ead17fd

JimBobSquarePants approved these changes Nov 22, 2018

View reviewed changes

Fix trns preservation and add tests

6849617

JimBobSquarePants merged commit 0483efa into SixLabors:master Nov 23, 2018

JimBobSquarePants mentioned this pull request Nov 23, 2018

Rgb image with transparency color will have transparency removed when being saved #763

Closed

4 tasks

antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019

Merge pull request SixLabors#764 from devedse/master

920237a

[WIP] Preserving isTrans data

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Preserving isTrans data #764

[WIP] Preserving isTrans data #764

devedse commented Oct 30, 2018

CLAassistant commented Oct 30, 2018 •

edited

Loading

devedse commented Oct 30, 2018

JimBobSquarePants commented Oct 31, 2018

devedse commented Oct 31, 2018 •

edited

Loading

devedse commented Nov 1, 2018

antonfirsov left a comment

JimBobSquarePants left a comment

JimBobSquarePants commented Nov 1, 2018 •

edited

Loading

codecov bot commented Nov 2, 2018 •

edited

Loading

devedse commented Nov 2, 2018

antonfirsov commented Nov 2, 2018 •

edited

Loading

antonfirsov left a comment

JimBobSquarePants commented Nov 2, 2018

devedse commented Nov 4, 2018

antonfirsov commented Nov 4, 2018

JimBobSquarePants commented Nov 22, 2018

JimBobSquarePants commented Nov 23, 2018

[WIP] Preserving isTrans data #764

[WIP] Preserving isTrans data #764

Conversation

devedse commented Oct 30, 2018

Prerequisites

Description

CLAassistant commented Oct 30, 2018 • edited Loading

devedse commented Oct 30, 2018

JimBobSquarePants commented Oct 31, 2018

devedse commented Oct 31, 2018 • edited Loading

devedse commented Nov 1, 2018

antonfirsov left a comment

Choose a reason for hiding this comment

JimBobSquarePants left a comment

Choose a reason for hiding this comment

JimBobSquarePants commented Nov 1, 2018 • edited Loading

codecov bot commented Nov 2, 2018 • edited Loading

Codecov Report

devedse commented Nov 2, 2018

antonfirsov commented Nov 2, 2018 • edited Loading

antonfirsov left a comment

Choose a reason for hiding this comment

JimBobSquarePants commented Nov 2, 2018

devedse commented Nov 4, 2018

antonfirsov commented Nov 4, 2018

JimBobSquarePants commented Nov 22, 2018

JimBobSquarePants commented Nov 23, 2018

CLAassistant commented Oct 30, 2018 •

edited

Loading

devedse commented Oct 31, 2018 •

edited

Loading

JimBobSquarePants commented Nov 1, 2018 •

edited

Loading

codecov bot commented Nov 2, 2018 •

edited

Loading

antonfirsov commented Nov 2, 2018 •

edited

Loading