Issue #144 Fix - JBIG2 - Changed integer variables types

[kucjac]

This PR fixes #144.

There were problem with non-sized integers for non amd64 platforms. A lot of structures in the JBIG2 encoding required to have their integer variable sizes set on.
The problem no longer occurs on emulated environment.

This PR were tested in an emulated ARMv7 environment as well as on amd64 platform.

---

This change is 

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (development@febf633). Click here to learn what that means.
The diff coverage is 66.01%.

@@              Coverage Diff               @@
##             development     #148   +/-   ##
==============================================
  Coverage               ?   63.01%           
==============================================
  Files                  ?      187           
  Lines                  ?    33705           
  Branches               ?        0           
==============================================
  Hits                   ?    21239           
  Misses                 ?    11968           
  Partials               ?      498

Impacted Files	Coverage Δ
internal/jbig2/page.go	38.04% <0%> (ø)
internal/jbig2/segments/table_segment.go	0% <0%> (ø)
internal/jbig2/segments/eos.go	0% <0%> (ø)
internal/jbig2/decoder/huffman/encoded_table.go	0% <0%> (ø)
internal/jbig2/decoder/mmr/rundata.go	79.41% <100%> (ø)
internal/jbig2/segments/region.go	85% <100%> (ø)
internal/jbig2/decoder/arithmetic/arithmetic.go	87.83% <100%> (ø)
internal/jbig2/segments/pattern-dictionary.go	69.23% <100%> (ø)
internal/jbig2/decoder/mmr/mmr.go	66.03% <100%> (ø)
internal/jbig2/decoder/huffman/standard_table.go	86.48% <100%> (ø)
... and 10 more

---

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 febf633...1d2aabe. Read the comment docs.

[gunnsth]

Why are those specific integers needed, i.e. like int32 etc, why not just int?
We don't have a convention of using int32s over int in the codebase overall

Any specific reason why it would be needed in jbig2 over other parts of the code? What do other go projects do ?

[adrg]

The changes look correct to me. It should fix issue #144. However, there seem to be a lot of modifications. I'm not sure why all the int -> int32 and int -> uint32 changes. Maybe only a subset of the modifications is needed in order to solve the issue? The problem seems to be the usage of 0xffffffff which is basically the value of math.MaxUint32, with int variables..
From what I can tell, an int is at least 32 bits on all of the platforms/architectures that Go supports.

In my opinion, the changes in types should be done depending on the context. I usually follow these guidelines for integers:

1. If the maximum value of an entity is known, then the smallest type that can hold that maximum value should be used, in order to be memory efficient. For example, in the image/color package, color.RGBA holds components of type uint8 as the values cannot be larger than 255. Similarly RGBA64 has color components of type uint16.
2. If the value of an entity cannot be negative, then an unsigned type should be used.
3. When not sure, use int or a larger type if large values are to be expected (like int64). I tend to prefer int in most cases, especially for indices of loops or lengths of collections. The len builtin function returns an int.

An advantage of using fixed types like int32 is that the behavior is the same on all platforms.
However, when used for lengths of slices and loop indices, a lot of casts have to be made, and the code does not look that pretty:

for curLen := int32(1); curLen <= int32(len(lenCount)); curLen++ {}

There also seem to be places in the code where an int is used, not an int32 and then the values are clamped to the maximum of int32 like this:

r.XLocation = int(temp & math.MaxInt32)

Why not use an int32 if the maximum allowed value is the maximum of int32?
As an alternative, you could use something like:

const MaxUint = ^uint(0) 
const MaxInt = int(MaxUint >> 1) 

The value of MaxInt should be math.MaxInt64 on platforms where int is 64 bits and math.MaxInt32 on platforms where int is 32 bits. Then, MaxInt could be used instead of math.MaxInt32.

[kucjac]

Current PR not only fixes #144 but also prevent unwanted behavior of the huffman table and arithmetic decoder where the int32 variables mustn't overflow on 64bit platforms .
The most recent revision contains already a subset of the modifications that are required for the jbig2 decoder to work properly.

The changes of int -> uint32 sets the unsigned restriction.

In initial revision the int -> int32 change were applied to most of the jbig2 packages. The JBIG2 standard ISO/IEC 14992 defines segments and document variable size precisely. As discussed with gunnsth setting int32 instead of int is against the convention. Variables size constraint in jbig2 segments and document doesn't change the logic. What's more casting into int32 makes the code less readable.

That's why only the decoders parts have an int32 type constraint, while the other were left with int.

Reviewable status: 0 of 24 files reviewed, all discussions resolved

[CLAassistant]

All committers have signed the CLA.

[gunnsth]

@kucjac Can you sign the CLA again? There was a problem with it, just fixed it.
I think wherever the specifications say 32-bit, it makes sense to use a dedicated 32 bit variable.
All the changes seem to be in internal packages so no breaking changes in this.

[kucjac]

@gunnsth Sure, I've already signed it.
Ok, I'm going to change all the specification defined 32-bit integers in the segment definitions from int -> int32.

Reviewable status: 0 of 24 files reviewed, all discussions resolved

[gunnsth]

LGTM