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

Update stb_image to v2.02 #777

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@h0tw1r3
Contributor

h0tw1r3 commented Jan 12, 2015

Cmake module added to detect SSE support, required with the updated version. NEON support is explicitly enabled (if applicable), but not tested.

I have only tested on Linux x86 (32bit) w/SSE2.

Show outdated Hide outdated cmake/Modules/FindSSE.cmake Outdated

@h0tw1r3 h0tw1r3 referenced this pull request Jan 12, 2015

Closed

PNG files larger than 8bit? #63

@@ -55,7 +56,7 @@ namespace
sf::InputStream* stream = static_cast<sf::InputStream*>(user);
return static_cast<int>(stream->read(data, size));
}
void skip(void* user, unsigned int size)
void skip(void* user, int size)

This comment has been minimized.

@eXpl0it3r

eXpl0it3r Jan 12, 2015

Member

What's the reason for removing the unsigned?

@eXpl0it3r

eXpl0it3r Jan 12, 2015

Member

What's the reason for removing the unsigned?

This comment has been minimized.

@h0tw1r3

h0tw1r3 Jan 12, 2015

Contributor

Updated lib allows for skipping backwards.

@h0tw1r3

h0tw1r3 Jan 12, 2015

Contributor

Updated lib allows for skipping backwards.

This comment has been minimized.

@eXpl0it3r

eXpl0it3r Jan 12, 2015

Member

Does the new stb_image make use of this, i.e. does it skip back at one point?

@eXpl0it3r

eXpl0it3r Jan 12, 2015

Member

Does the new stb_image make use of this, i.e. does it skip back at one point?

This comment has been minimized.

@eXpl0it3r

eXpl0it3r Jan 16, 2015

Member

So may I ask again, does stb_image skip back at some point? I mean this function is an internal detail of ImageLoader and won't be used by anyone else.

@eXpl0it3r

eXpl0it3r Jan 16, 2015

Member

So may I ask again, does stb_image skip back at some point? I mean this function is an internal detail of ImageLoader and won't be used by anyone else.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 12, 2015

Member

Cmake module added to detect SSE support, required with the updated version.

In which way is SSE required with the new stb_image version?

NEON support is explicitly enabled (if applicable), but not tested.

Can you quickly explain what "NEON" is?

Member

eXpl0it3r commented Jan 12, 2015

Cmake module added to detect SSE support, required with the updated version.

In which way is SSE required with the new stb_image version?

NEON support is explicitly enabled (if applicable), but not tested.

Can you quickly explain what "NEON" is?

@h0tw1r3

This comment has been minimized.

Show comment
Hide comment
@h0tw1r3

h0tw1r3 Jan 12, 2015

Contributor

In which way is SSE required with the new stb_image version?

It's not required per-say, the in-line documentation explains it best:

// SIMD support
//
// The JPEG decoder will try to automatically use SIMD kernels on x86 when
// supported by the compiler. For ARM Neon support, you must explicitly
// request it.
//
// (The old do-it-yourself SIMD API is no longer supported in the current
// code.)
//
// On x86, SSE2 will automatically be used when available based on a run-time
// test; if not, the generic C versions are used as a fall-back. On ARM targets,
// the typical path is to have separate builds for NEON and non-NEON devices
// (at least this is true for iOS and Android). Therefore, the NEON support is
// toggled by a build flag: define STBI_NEON to get NEON loops.
//
// The output of the JPEG decoder is slightly different from versions where
// SIMD support was introduced (that is, for versions before 1.49). The
// difference is only +-1 in the 8-bit RGB channels, and only on a small
// fraction of pixels. You can force the pre-1.49 behavior by defining
// STBI_JPEG_OLD, but this will disable some of the SIMD decoding path
// and hence cost some performance.
//
// If for some reason you do not want to use any of SIMD code, or if
// you have issues compiling it, you can disable it entirely by
// defining STBI_NO_SIMD.
Contributor

h0tw1r3 commented Jan 12, 2015

In which way is SSE required with the new stb_image version?

It's not required per-say, the in-line documentation explains it best:

// SIMD support
//
// The JPEG decoder will try to automatically use SIMD kernels on x86 when
// supported by the compiler. For ARM Neon support, you must explicitly
// request it.
//
// (The old do-it-yourself SIMD API is no longer supported in the current
// code.)
//
// On x86, SSE2 will automatically be used when available based on a run-time
// test; if not, the generic C versions are used as a fall-back. On ARM targets,
// the typical path is to have separate builds for NEON and non-NEON devices
// (at least this is true for iOS and Android). Therefore, the NEON support is
// toggled by a build flag: define STBI_NEON to get NEON loops.
//
// The output of the JPEG decoder is slightly different from versions where
// SIMD support was introduced (that is, for versions before 1.49). The
// difference is only +-1 in the 8-bit RGB channels, and only on a small
// fraction of pixels. You can force the pre-1.49 behavior by defining
// STBI_JPEG_OLD, but this will disable some of the SIMD decoding path
// and hence cost some performance.
//
// If for some reason you do not want to use any of SIMD code, or if
// you have issues compiling it, you can disable it entirely by
// defining STBI_NO_SIMD.
@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 12, 2015

Member

I don't think it is worth complicating the CMake setup for this:

  • on x86 architectures (99% of users), SSE is detected automatically
  • on ARM Neon architectures (a few people), SSE will not be used... just like before
Member

LaurentGomila commented Jan 12, 2015

I don't think it is worth complicating the CMake setup for this:

  • on x86 architectures (99% of users), SSE is detected automatically
  • on ARM Neon architectures (a few people), SSE will not be used... just like before
@h0tw1r3

This comment has been minimized.

Show comment
Hide comment
@h0tw1r3

h0tw1r3 Jan 12, 2015

Contributor

I don't think it is worth complicating the CMake setup for this:

Are you referring to @MarioLiebisch code comment? Even if options are not added, removing SSE3 may be adventageous as it's unnecessary.

Contributor

h0tw1r3 commented Jan 12, 2015

I don't think it is worth complicating the CMake setup for this:

Are you referring to @MarioLiebisch code comment? Even if options are not added, removing SSE3 may be adventageous as it's unnecessary.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 12, 2015

Member

Are you referring to @MarioLiebisch code comment?

No, I was referring to the whole SSE thing. We should just update stb_image and stop there.

Member

LaurentGomila commented Jan 12, 2015

Are you referring to @MarioLiebisch code comment?

No, I was referring to the whole SSE thing. We should just update stb_image and stop there.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Jan 16, 2015

Member

So do we want this change, or do we just want to update the stb_image an be done with it?

Member

eXpl0it3r commented Jan 16, 2015

So do we want this change, or do we just want to update the stb_image an be done with it?

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 16, 2015

Member

We should just update stb_image.

Member

LaurentGomila commented Jan 16, 2015

We should just update stb_image.

@h0tw1r3

This comment has been minimized.

Show comment
Hide comment
@h0tw1r3

h0tw1r3 Jan 16, 2015

Contributor

Updated stb_image default is to enable SIMD optimized functions on x86 32/64bit arch (SSE2+ required). Without the cmake module building fails on my dev platform (ArchLinux).

If you don't want the SIMD functions (why???), then a define will need to be added which forces them off. In which case there is no way for the developer to enable them at compile time via flags.

Contributor

h0tw1r3 commented Jan 16, 2015

Updated stb_image default is to enable SIMD optimized functions on x86 32/64bit arch (SSE2+ required). Without the cmake module building fails on my dev platform (ArchLinux).

If you don't want the SIMD functions (why???), then a define will need to be added which forces them off. In which case there is no way for the developer to enable them at compile time via flags.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 16, 2015

Member

Without the cmake module building fails on my dev platform

Then I think it's a bug, because the doc clearly states that SIMD is tested and enabled automatically on x86 platforms. And it actually works fine for me without doing anything (Visual C++ 12).

After confirming this on a minimal example (ie. outside SFML), you should probably report the problem to the author.

Member

LaurentGomila commented Jan 16, 2015

Without the cmake module building fails on my dev platform

Then I think it's a bug, because the doc clearly states that SIMD is tested and enabled automatically on x86 platforms. And it actually works fine for me without doing anything (Visual C++ 12).

After confirming this on a minimal example (ie. outside SFML), you should probably report the problem to the author.

@h0tw1r3

This comment has been minimized.

Show comment
Hide comment
@h0tw1r3

h0tw1r3 Jan 16, 2015

Contributor

It is not a bug with stb_image. If you read the stb_image documentation, it is by design. They expect the developer to set compiler options or defines as appropriate for the environment.

GCC does not enable architecture specific options unless specifically told to (or pre-configured to by the packager/distribution).

Archlinux:

[h0tw1r3@archlinux_x86 ~]$ gcc -Q --help=target | grep sse2
  -msse2                                [disabled]
  -msse2avx                             [disabled]

Centos 7:

h0tw1r3@pxe:~$ gcc -Q --help=target | grep sse2
  -msse2                                [disabled]
  -msse2avx                             [disabled]
Contributor

h0tw1r3 commented Jan 16, 2015

It is not a bug with stb_image. If you read the stb_image documentation, it is by design. They expect the developer to set compiler options or defines as appropriate for the environment.

GCC does not enable architecture specific options unless specifically told to (or pre-configured to by the packager/distribution).

Archlinux:

[h0tw1r3@archlinux_x86 ~]$ gcc -Q --help=target | grep sse2
  -msse2                                [disabled]
  -msse2avx                             [disabled]

Centos 7:

h0tw1r3@pxe:~$ gcc -Q --help=target | grep sse2
  -msse2                                [disabled]
  -msse2avx                             [disabled]
@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 16, 2015

Member

If you read the stb_image documentation, it is by design. They expect the developer to set compiler options or defines as appropriate for the environment.

Can you show the corresponding part of the doc? All I can read in this doc is that SIMD is detected automatically, so it should never fail to compile.

Member

LaurentGomila commented Jan 16, 2015

If you read the stb_image documentation, it is by design. They expect the developer to set compiler options or defines as appropriate for the environment.

Can you show the corresponding part of the doc? All I can read in this doc is that SIMD is detected automatically, so it should never fail to compile.

@h0tw1r3

This comment has been minimized.

Show comment
Hide comment
@h0tw1r3

h0tw1r3 Jan 16, 2015

Contributor

You're correct, I misread. Defines apply to forcefully disabling.

I'll be interested to see how stb_image fixes SSE2 autodetection in a way that works as you expect. I have yet to see it (SSE) done without assumption or explicit detection with gcc/mingw.

Contributor

h0tw1r3 commented Jan 16, 2015

You're correct, I misread. Defines apply to forcefully disabling.

I'll be interested to see how stb_image fixes SSE2 autodetection in a way that works as you expect. I have yet to see it (SSE) done without assumption or explicit detection with gcc/mingw.

@jcowgill

This comment has been minimized.

Show comment
Hide comment
@jcowgill

jcowgill Jan 16, 2015

Contributor

A few things:

On x86_64, -msse2 is enabled by default even if you don't pass it in (since all x86_64 processors are guaranteed to have it). gcc -Q --help=target is lying to you.

# Debian GCC 4.9.2 on x86-64
$ echo | gcc -dM -E - | grep -i sse
#define __SSE2_MATH__ 1
#define __SSE_MATH__ 1
#define __SSE2__ 1
#define __SSE__ 1

The gcc manual on -msse2 (https://gcc.gnu.org/onlinedocs/gcc-4.9.2/gcc/i386-and-x86-64-Options.html) says:

-msse2
[...]
These options enable GCC to use these extended instructions in generated code, even without -mfpmath=sse. Applications that perform run-time CPU detection must compile separate files for each supported architecture, using the appropriate flags. In particular, the file containing the CPU detection code should be compiled without these options.

So I don't see how any runtime detection could reliably work on i386, since you have to turn on -msse2 for the runtime detection code to compile, but then gcc could start using sse instructions by itself anywhere it wants (even if the detection fails).

Contributor

jcowgill commented Jan 16, 2015

A few things:

On x86_64, -msse2 is enabled by default even if you don't pass it in (since all x86_64 processors are guaranteed to have it). gcc -Q --help=target is lying to you.

# Debian GCC 4.9.2 on x86-64
$ echo | gcc -dM -E - | grep -i sse
#define __SSE2_MATH__ 1
#define __SSE_MATH__ 1
#define __SSE2__ 1
#define __SSE__ 1

The gcc manual on -msse2 (https://gcc.gnu.org/onlinedocs/gcc-4.9.2/gcc/i386-and-x86-64-Options.html) says:

-msse2
[...]
These options enable GCC to use these extended instructions in generated code, even without -mfpmath=sse. Applications that perform run-time CPU detection must compile separate files for each supported architecture, using the appropriate flags. In particular, the file containing the CPU detection code should be compiled without these options.

So I don't see how any runtime detection could reliably work on i386, since you have to turn on -msse2 for the runtime detection code to compile, but then gcc could start using sse instructions by itself anywhere it wants (even if the detection fails).

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 6, 2015

Member

Can we change this PR to updating stb_image only and move the SSE discussion either to a dedicated issue or better a forum thread?

Member

eXpl0it3r commented Feb 6, 2015

Can we change this PR to updating stb_image only and move the SSE discussion either to a dedicated issue or better a forum thread?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 20, 2015

Member

@h0tw1r3 Do you want to change the PR to just update stb_image, which has been relocated in master?

Member

eXpl0it3r commented Feb 20, 2015

@h0tw1r3 Do you want to change the PR to just update stb_image, which has been relocated in master?

@h0tw1r3

This comment has been minimized.

Show comment
Hide comment
@h0tw1r3

h0tw1r3 Feb 21, 2015

Contributor

Sure, will update the PR today.

Contributor

h0tw1r3 commented Feb 21, 2015

Sure, will update the PR today.

@h0tw1r3

This comment has been minimized.

Show comment
Hide comment
@h0tw1r3

h0tw1r3 Feb 24, 2015

Contributor

@eXpl0it3r SSE stuff removed and rebased against master.

Contributor

h0tw1r3 commented Feb 24, 2015

@eXpl0it3r SSE stuff removed and rebased against master.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 24, 2015

Member

There's still a modification in ImageLoader.cpp which you haven't commented on it's actual use.

Member

eXpl0it3r commented Feb 24, 2015

There's still a modification in ImageLoader.cpp which you haven't commented on it's actual use.

@eXpl0it3r eXpl0it3r added this to the 2.3 milestone Feb 24, 2015

@eXpl0it3r eXpl0it3r self-assigned this Feb 24, 2015

@eXpl0it3r eXpl0it3r added s:accepted and removed s:undecided labels Feb 24, 2015

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Feb 24, 2015

Member

There's still a modification in ImageLoader.cpp which you haven't commented on it's actual use.

It's a callback, if its signature has changed we have to change it as well so that it still matches.

Member

LaurentGomila commented Feb 24, 2015

There's still a modification in ImageLoader.cpp which you haven't commented on it's actual use.

It's a callback, if its signature has changed we have to change it as well so that it still matches.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 24, 2015

Member

Ah I see. Well then lets test this.

Member

eXpl0it3r commented Feb 24, 2015

Ah I see. Well then lets test this.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Feb 25, 2015

Member

Just one last thing: Can you remove line 4 in .gitattributes that I forgot to remove in the commit that moved it out of the src directory? 😁

Member

binary1248 commented Feb 25, 2015

Just one last thing: Can you remove line 4 in .gitattributes that I forgot to remove in the commit that moved it out of the src directory? 😁

@h0tw1r3 h0tw1r3 changed the title from Update stb_image to v2.00b to Update stb_image to v2.02 Feb 25, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 27, 2015

Member

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

Member

eXpl0it3r commented Feb 27, 2015

This PR has been added to my merge list, meaning it will be merged soon, unless someone raises any concerns.

@zsbzsb

This comment has been minimized.

Show comment
Hide comment
@zsbzsb

zsbzsb Feb 28, 2015

Member

I'm thinking that the last commit should probably be squashed onto the first with the first's commit message of "Update stb_image to v2.02 and stb_image_write to v0.97". I don't see the need for having a second commit when the change in the second should be made in the first when the source was moved.

Member

zsbzsb commented Feb 28, 2015

I'm thinking that the last commit should probably be squashed onto the first with the first's commit message of "Update stb_image to v2.02 and stb_image_write to v0.97". I don't see the need for having a second commit when the change in the second should be made in the first when the source was moved.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Feb 28, 2015

Member

Squashed, rebased and merged in f47f89a

Member

eXpl0it3r commented Feb 28, 2015

Squashed, rebased and merged in f47f89a

@eXpl0it3r eXpl0it3r closed this Feb 28, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment