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

Use StbSharp for all Texture2D.FromStream #6008

Merged
merged 2 commits into from Feb 23, 2018

Conversation

KonajuGames
Copy link
Contributor

StbSharp is now used for all implementations of Texture2D.FromStream(GraphicsDevice, Stream), avoiding discrepancies with OS-provided image loading routines.

As a result of this, DirectX platforms no longer support TIF or DDS through FromStream().

Added unit tests for loading 1-, 8-, 24- and 32-bit PNGs.

Fixes #6001

@rds1983
Copy link
Contributor

rds1983 commented Oct 13, 2017

Also, all images will be converted to 32-bit on loading as Stb is called with Imaging.STBI_rgb_alpha. It could be avoided by using Imaging.STBI__default, then images will be loaded with their original channels per pixel. But then a conversion code will be required to convert image to the corresponding SurfaceType.

@tomspilman
Copy link
Member

Also, all images will be converted to 32-bit on loading

That should be correct. If i remember correctly XNA only returned Color images from FromStream regardless of the input format of the image.

@KonajuGames
Copy link
Contributor Author

KonajuGames commented Oct 14, 2017 via email

@rds1983
Copy link
Contributor

rds1983 commented Oct 14, 2017

I see. Thanks for the explanation.

using (var bitmap = LoadBitmap(stream, out decoder))
using (decoder)
// XNA blacks out any pixels with an alpha of zero.
if (channels == 4)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be unnecessary as we always read 4 channels?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait... is this the original number for channels before conversion to RGBA? If so i see this is just here to avoid doing the black out if the data was 24bit and didn't have alpha to begin with.

b[i + 0] = 0;
b[i + 1] = 0;
b[i + 2] = 0;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know if this is necessary with STB? We should determine if it internally does this already to avoid the extra work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XNA was unique in this regard by partially premultiplying alpha (only where A = 0). STB returns the data as in the original PNG. In my test case, this was RGB = 0xFFFFFF and A = 0.

@amerkoleci
Copy link

Agree, why adding extra dependency if we can use WIC on windows and the corresponding platform code?

@KonajuGames
Copy link
Contributor Author

Agree, why adding extra dependency if we can use WIC on windows and the corresponding platform code?

That's what we are trying to avoid. We want one managed codebase that works on all platforms.

@afrodynamics
Copy link
Contributor

Would love to see this merged! Most of our textures are loaded via FromStream directly from PNGs, but it seems some driver vendors do not support this and crash instead. The less we can depend on driver vendors the better, IMO.

@rds1983
Copy link
Contributor

rds1983 commented Jan 12, 2018

Sorry, my PR #6131 created conflict with this one.
Hovewer should be easy to resolve as it affected only MonoGame.Framework.definition(I've removed TinyJpegSharp from it).

@dellis1972
Copy link
Contributor

@KonajuGames can you rebase this?

Steve Williams added 2 commits February 21, 2018 22:34
StbSharp is now used for all implementations of Texture2D.FromStream(GraphicsDevice, Stream), avoiding discrepancies with OS-provided image loading routines.

As a result of this, DirectX platforms no longer support TIF or DDS through FromStream().

Added unit tests for loading 1-, 8-, 24- and 32-bit PNGs.

Fixes MonoGame#6001
@KonajuGames
Copy link
Contributor Author

This PR is now rebased and passing again.
@dellis1972 @tomspilman

@dellis1972
Copy link
Contributor

looks good, @tomspilman

@tomspilman
Copy link
Member

Thanks @KonajuGames !

@tomspilman tomspilman added this to the 3.7 Release milestone Feb 23, 2018
@tomspilman tomspilman merged commit 1b1a768 into MonoGame:develop Feb 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants