opj_encode with tcp broken on 64bit

[CarloWood]

The SecondLife viewer does this:

parameters.tcp_numlayers = 5;
parameters.tcp_rates[0] = 1920.0f;
parameters.tcp_rates[1] = 480.0f;
parameters.tcp_rates[2] = 120.0f;
parameters.tcp_rates[3] = 30.0f;
parameters.tcp_rates[4] = 10.0f;
parameters.irreversible = 1;
parameters.tcp_mct = 1;

after the call to opj_set_default_encoder_parameters(&parameters);

For the rest it is basically the same as image_to_j2k.c in
trunk.tar.gz

I added the above to image_to_j2k.c and get the same result as in
the SL viewer: images with alpha (RGBA) are screwed on 64bit:
the image becomes (most often) completely half-transparent.

This code DOES work on 32bit.

To test, apply the following patch and convert a .tga with alpha
to j2k (and back) on a 64bit machine:

diff -rc trunk.orig/codec/image_to_j2k.c trunk/codec/image_to_j2k.c
*** trunk.orig/codec/image_to_j2k.c 2008-10-01 18:00:42.000000000
+0200
--- trunk/codec/image_to_j2k.c 2009-04-05 03:50:56.000000000 +0200
***************
*** 1503,1508 ****
--- 1503,1518 ----
/* set encoding parameters to default values */
opj_set_default_encoder_parameters(&parameters);

+ /* Use these TCP settings */
+ parameters.tcp_numlayers = 5;
+ parameters.tcp_rates[0] = 1920.0f;
+ parameters.tcp_rates[1] = 480.0f;
+ parameters.tcp_rates[2] = 120.0f;
+ parameters.tcp_rates[3] = 30.0f;
+ parameters.tcp_rates[4] = 10.0f;
+ parameters.irreversible = 1;
+ parameters.tcp_mct = 1;
+
/* Initialize indexfilename and img_fol */
*indexfilename = 0;
memset(&img_fol,0,sizeof(img_fol_t));
***************
*** 1642,1648 ****
break;
}
/* Decide if MCT should be used */
! parameters.tcp_mct = image->numcomps == 3 ? 1 : 0;

if(parameters.cp_cinema){
cinema_setup_encoder(&parameters,image,&img_fol);
--- 1652,1658 ----
break;
}
/* Decide if MCT should be used */
! //parameters.tcp_mct = image->numcomps == 3 ? 1 : 0;

if(parameters.cp_cinema){
cinema_setup_encoder(&parameters,image,&img_fol);

[Dzonatas]

I've done some tests about this.

No need to modify image_to_j2k/j2k_to_image to make this happen, as it
can be done by the command line options.

Before image: http://mono.dzonux.net/file/logo_before.tga

$ image_to_j2k -o logo.j2k -i logo_before.tga -r 1920,480,120,40,10
$ j2k_to_image -i logo.j2k -o logo_after.tga

After image: http://mono.dzonux.net/file/logo_after.tga

[CarloWood]

Thanks for the verification.

Is this group / list still alive (is someone going to look into this,
and fix this), or is the openjpeg development dead?

Carlo

[Dzonatas]

If it is confirmed a 64bit issue, then the fix may require the openjpeg
2000 library to have C99 as a requirement. C99 defines standard types to
handle such issues without any knowledge of the platform the compiler is
on. The current library is built with C99 in the Makefile, but it has
not been made a requirement for other project files to build openjpeg.

I think we can safely deprecate support for compilers that do not
support at least C99 or C++ standards. If a compiler doesn't support C99
but does support C++, then we can fallback to C++ standards for now to
support 64bit types.

The problem seems to exist, on 64bit machines, when T2 is activated for
rates. A lossy or lossless decode without rates tests fine on 64bit.
(alpha layer is correct)

Yes this group is still alive.

[Bob Friesenhahn]

On Tue, 28 Apr 2009, Dzonatas wrote:

> If it is confirmed a 64bit issue, then the fix may require the openjpeg
> 2000 library to have C99 as a requirement. C99 defines standard types to
> handle such issues without any knowledge of the platform the compiler is
> on. The current library is built with C99 in the Makefile, but it has
> not been made a requirement for other project files to build openjpeg.

Any project which uses Autoconf can easily emulate the C99 int types.
That is what my project, littlecms, and the forthcoming libtiff4 do.
This allows working on older systems.

> I think we can safely deprecate support for compilers that do not
> support at least C99 or C++ standards. If a compiler doesn't support C99
> but does support C++, then we can fallback to C++ standards for now to
> support 64bit types.

A problem is that the popular open source compiler GCC decided not to
provide replacements for the C99 stdint header files if they are
missing so older systems won't be fixed by simply installing/updating
GCC. It took three years (or more) beyond 1999 before updated
operating systems were released with conformant header files. And
then of course there are those who continue to use older Windows
compilers.

Bob
--
Bob Friesenhahn
bfri...@simple.dallas.tx.us, http://www.simplesystems.org/users/bfriesen/
GraphicsMagick Maintainer, http://www.GraphicsMagick.org/

[Dzonatas]

Bob Friesenhahn wrote:
> A problem is that the popular open source compiler GCC decided not to
> provide replacements for the C99 stdint header files if they are
> missing so older systems won't be fixed by simply installing/updating
> GCC.

The older systems may need backports, which would be two branches of
libopenjpeg. I don't think we need to use the intypes.h header, which
would be the one in question under Visual Studio. The other C99 headers
are now ISO standards, but for awhile the C99 standard was not an ISO
standard.

This is what I have so far in opj_headers.h:

...include C90 standard headers

... include openjpeg.h header

/*
==========================================================
C99 Standard: detection and includes
==========================================================
*/

#define OPJ_C99 (__STDC_VERSION__ >= 199901L)

#if OPJ_C99
#include <stddef.h>
#include <stdint.h>
#endif

/*
==========================================================
Non-C99 support and deprecation
==========================================================
*/

#if OPJ_C99 || defined(__cplusplus)
#define INLINE inline

#elif defined(__GNUC__)
#define INLINE __inline__

#elif defined(_MSC_VER)
#define INLINE __inline

#elif defined(__MWERKS__)
#define INLINE inline

#else
#define INLINE /* inline */
#endif

#if !OPJ_C99 && defined(__GNUC__)
#define restrict __restrict__

#elif !OPJ_C99
#define restrict /* restrict */

#endif

#if !OPJ_C99 && defined(__cplusplus)
#include <cstddefs>

#elif !OPJ_C99 && !defined(__cplusplus)
#error Your compiler is neither set to C99 nor C++ standard mode.
32bit int type compatibility assumed.
typedef int ptrdiff_t;

#endif

...include misc defs

..include module headers

[Dzonatas]

The rates are handled in T2 and TCD, so that's where I have looked recently.

I downloaded the 1.3 exe (from www.openjpeg.org) onto may 32bit Windows
system, and I got the same alpha bug. Even if 1.0 is specified as a
single rate, it still happens. Therefore, I doubt this is a 64bit issue.

Going over file diffs since 0.97...

[Bob Friesenhahn]

On Tue, 28 Apr 2009, Dzonatas wrote:

> The older systems may need backports, which would be two branches of
> libopenjpeg. I don't think we need to use the intypes.h header, which
> would be the one in question under Visual Studio. The other C99 headers
> are now ISO standards, but for awhile the C99 standard was not an ISO
> standard.

While the C99 standard was deleloped, there was an <inttypes.h> header
in early drafts. Later on <stdint.h> was added. The <inttypes.h>
header includes everything that <stdint.h> does and is part of the
final standard.

>
> This is what I have so far in opj_headers.h:

> #define OPJ_C99 (__STDC_VERSION__ >= 199901L)

Unfortunately, __STDC_VERSION__ is useless on systems using GCC when
it comes to deciding which headers and standard types may be
available.

> #if OPJ_C99
> #include <stddef.h>
> #include <stdint.h>
> #endif

The above will fail to work on Solaris 9. If you include <inttypes.h>
rather than <stdint.h> then it will work for Solaris 9. But it won't
work for Solaris 8 or earlier.

[Dzonatas]

Bob Friesenhahn wrote:

>
>> #define OPJ_C99 (__STDC_VERSION__ >= 199901L)
>>
>
> Unfortunately, __STDC_VERSION__ is useless on systems using GCC when
> it comes to deciding which headers and standard types may be
> available.
>

GCC has -std=c99, which is what has been set in the Makefile in the
repository. Can you explain more why you think it is useless?

>
>> #if OPJ_C99
>> #include <stddef.h>
>> #include <stdint.h>
>> #endif
>>
>
> The above will fail to work on Solaris 9. If you include <inttypes.h>
> rather than <stdint.h> then it will work for Solaris 9. But it won't
> work for Solaris 8 or earlier.
>

__STDC_VERSION__ isn't set before Solaris 10 (Sun Studio 10). Solaris 9
and 8 will fallback to C++ standard.

The two main declarations needed are:

size_t
ptrdiff_t

The optional ones are:

uint8_t
uint32_t
int32_t

C89/C90 (default for many compilers) has size_t defined by stdlib.h. C99
and C++ define ptrdiff_t. Instead of worrying about if inttypes.h
exists, we can default uint8_t to unsigned char.

The main reason to use uint8_t is to avoid issues in compilers that try
to change the size of char. The libopenjpeg code now uses 'unsigned
char', so if uint8_t can be detected then that is a plus other it goes
back to the default as it is now, unsigned char.

Popular compilers have nuances about int and unsigned since 64bit
(GCC4.x+/MSVC7+). Both "int" and "unsigned" keywords are stuck to 32bit
terms in those compilers. Hence, libopenjpeg compiles find now on 64bit
systems that have stuck nuances as such. However, there are potential
pointer difference compatibility issues. Also, it probably is not a good
idea to assume other compilers will have these nuances. Instead of a
messy header file to handle all cases of these nuances, we can default
uint32_t and int32_t as mentioned above in uint8_t. Maybe offer an
#ifdef to override these defaults when C99 is not found.

[Bob Friesenhahn]

On Wed, 29 Apr 2009, Dzonatas wrote:
>>
>> Unfortunately, __STDC_VERSION__ is useless on systems using GCC when
>> it comes to deciding which headers and standard types may be
>> available.
>
> GCC has -std=c99, which is what has been set in the Makefile in the
> repository. Can you explain more why you think it is useless?

Because part of the headers are provided by the compiler but most of
the headers are provided by the operating system. Since GCC does not
provide <stdint.h> if it is missing then you can't count on GCC's
claim of compliance to know if the header is available. The option
does influence the compiler itself to respect C99 syntax.

>> The above will fail to work on Solaris 9. If you include <inttypes.h>
>> rather than <stdint.h> then it will work for Solaris 9. But it won't
>> work for Solaris 8 or earlier.
>>
> __STDC_VERSION__ isn't set before Solaris 10 (Sun Studio 10). Solaris 9
> and 8 will fallback to C++ standard.

I am not sure how to answer this except to point out that many
different verions of GCC and the Sun compiler will work on Solaris 10.
It is likely that the compiler produces this define. Solaris 10's
assert.h knows about __STDC_VERSION__, but Solaris 9's does not.

> The two main declarations needed are:
>
> size_t
> ptrdiff_t

The size_t is defined by original POSIX so it is available on all Unix
type systems. As far as Solaris goes, I see that ptrdiff_t was added
in Solaris 7.

> The main reason to use uint8_t is to avoid issues in compilers that try
> to change the size of char. The libopenjpeg code now uses 'unsigned
> char', so if uint8_t can be detected then that is a plus other it goes
> back to the default as it is now, unsigned char.

I have yet to encounter a system where 'char' is not 8 bits, although
I have encountered at least one system where char is signed.

> Popular compilers have nuances about int and unsigned since 64bit
> (GCC4.x+/MSVC7+). Both "int" and "unsigned" keywords are stuck to 32bit
> terms in those compilers. Hence, libopenjpeg compiles find now on 64bit
> systems that have stuck nuances as such. However, there are potential
> pointer difference compatibility issues. Also, it probably is not a good
> idea to assume other compilers will have these nuances. Instead of a
> messy header file to handle all cases of these nuances, we can default
> uint32_t and int32_t as mentioned above in uint8_t. Maybe offer an
> #ifdef to override these defaults when C99 is not found.

It is true that normal systems (i.e. other than weird ones like Cray)
will have a 32-bit int. There is value for OpenJPEG to be able to
support really large images so it seems that OpenJPEG should have the
ability to use 64-bit types as well. I have not encountered any
general purpose CPUs which don't support a 64-bit type, although some
of them provide less performance, or don't provide type conversions
to/from an unsigned 64-bit type (an issue with older Visual Studio).

[Dzonatas]

Bob Friesenhahn wrote:
> Because part of the headers are provided by the compiler but most of
> the headers are provided by the operating system. Since GCC does not
> provide <stdint.h> if it is missing then you can't count on GCC's
> claim of compliance to know if the header is available. The option
> does influence the compiler itself to respect C99 syntax.
>

So, you mean where GCC is installed and something other than libc6 is
used. This would fall into the crowd to not use -std=c99 to compile
libopenjpeg, one would then either use G++ or use the default int/char.
These are tests that should be preformed outside the C/C++ preprocessor.
(As you suggested, autoconf and like)

I do realize compilers did support parts of C99 before they conformed to
ISO C99.

> There is value for OpenJPEG to be able to
> support really large images so it seems that OpenJPEG should have the
> ability to use 64-bit types as well. I have not encountered any
> general purpose CPUs which don't support a 64-bit type, although some
> of them provide less performance, or don't provide type conversions
> to/from an unsigned 64-bit type (an issue with older Visual Studio).
>

Right now OpenJPEG seems to only support images with max 2GB dimensions
since the default file formats for j2k, jp2, etc and the transform use
32bit units by the standard. The C99/C++ standards could help to deal
with addressable space beyond 32bit in order to handle images with that
maximum.

ISO C99 helps avoid the need to define messy fringe cases in the
preprocessor, so we need to take a step in that direction.

[Callum Lerwick]

On Wed, Apr 29, 2009 at 1:02 PM, Bob Friesenhahn
<bfri...@simple.dallas.tx.us> wrote:
>
> On Wed, 29 Apr 2009, Dzonatas wrote:
>> The main reason to use uint8_t is to avoid issues in compilers that try
>> to change the size of char. The libopenjpeg code now uses 'unsigned
>> char', so if uint8_t can be detected then that is a plus other it goes
>> back to the default as it is now, unsigned char.
>
> I have yet to encounter a system where 'char' is not 8 bits, although
> I have encountered at least one system where char is signed.

C standards dictate that sizeof(char) == 1. Thus you can count on char
being 8 bits on anything except maybe a PDP-10. However, yes
signedness is not standardized and is typically whatever is optimal
for the architecture.

[Bob Friesenhahn]

On Wed, 29 Apr 2009, Callum Lerwick wrote:
>>
>> I have yet to encounter a system where 'char' is not 8 bits, although
>> I have encountered at least one system where char is signed.
>
> C standards dictate that sizeof(char) == 1. Thus you can count on char
> being 8 bits on anything except maybe a PDP-10. However, yes

Sizeof counts the number of characters, not bits, so sizeof(char) == 1
indicates very little.

[Dzonatas]

So, I spent the last week going over the code about this alpha (opacity)
issue. That's why there is a few more e-mails from me lately.

The good news is, we know that alpha/opacity components do work with
OpenJPEG v2 even if various rates are specified.

The bad news is, I doubt that this implementation that works in v2 can
be backported to v1 (1.3) to just get alpha/opacity to fully work. In
v2, there is a new stream implementation, and the old stream
implementation is where it appears the error of the alpha/opacity issue
exists. I know I say appears and that it also seems odd for the stream
to somehow affect the alpha/opacity issue.

Fortunately, maybe something can be done in the meantime until v2
becomes more active. I do have a suggestion. If the only reason to
switch to v2 is to gain a fully workable opacity component, then the
current version 1.3 can be used to decode while the v2 can be used to
encode. I suggest this because v2 is reported to not work with truncated
streams like v1.3 can do at the time. Most likely, you can guarantee the
stream to encode won't be truncated like the stream to decode.

Well, this will be interesting.

[Callum Lerwick]

On Sat, May 2, 2009 at 11:33 PM, Dzonatas <dzon...@dzonux.net> wrote:
>
> So, I spent the last week going over the code about this alpha (opacity)
> issue. That's why there is a few more e-mails from me lately.
>
> The good news is, we know that alpha/opacity components do work with
> OpenJPEG v2 even if various rates are specified.
>
> The bad news is, I doubt that this implementation that works in v2 can
> be backported to v1 (1.3) to just get alpha/opacity to fully work. In
> v2, there is a new stream implementation, and the old stream
> implementation is where it appears the error of the alpha/opacity issue
> exists. I know I say appears and that it also seems odd for the stream
> to somehow affect the alpha/opacity issue.
>
> Fortunately, maybe something can be done in the meantime until v2
> becomes more active.

I think we should just be the ones to make v2 active.

Anyone tried getting it working with SL yet? We're certainly the ones
to handle that...

> I do have a suggestion. If the only reason to
> switch to v2 is to gain a fully workable opacity component, then the
> current version 1.3 can be used to decode while the v2 can be used to
> encode. I suggest this because v2 is reported to not work with truncated
> streams like v1.3 can do at the time.

This can probably be fixed...

Just getting v2 up to snuff seems like less work than trying to link
v1.3 and v2 into the same app at the same time. :P

[Dzonatas]

Callum Lerwick wrote:
> On Sat, May 2, 2009 at 11:33 PM, Dzonatas <dzon...@dzonux.net> wrote:
>
>> Fortunately, maybe something can be done in the meantime until v2
>> becomes more active.
>>
>
> I think we should just be the ones to make v2 active.
>

I agree. I'll post a patch that merges the latest changes from v1.3
after it was released into v2, so we can get going on this.

[Dzonatas]

Callum Lerwick wrote:
> Anyone tried getting it working with SL yet? We're certainly the ones
> to handle that...
>

Yes. I have v2 doing the encode and decode in the viewer, and it works
perfectly with alpha.

I'll post a patch when I clean-up what I've done. Part patch here to
openjpeg, and the rest over there.