Addressing format related feedback from RANS PR that applies to Range Codec as well

Author: yash-puligundla

src/main/java/htsjdk/samtools/cram/compression/range/RangeCoder.java

@@ -11,7 +11,7 @@ public class RangeCoder {
     private boolean carry;
     private int cache;
 
-    public RangeCoder() {
+    protected RangeCoder() {
         // Spec: RangeEncodeStart
         this.low = 0;
         this.range = 0xFFFFFFFFL; // 4 bytes of all 1's
@@ -21,15 +21,15 @@ public RangeCoder() {
         this.cache = 0;
     }
 
-    public void rangeDecodeStart(ByteBuffer inBuffer){
+    protected void rangeDecodeStart(ByteBuffer inBuffer){
         for (int i = 0; i < 5; i++){
 
             // Get next 5 bytes. Ensure it is +ve
             code = (code << 8) + (inBuffer.get() & 0xFF);
         }
     }
 
-    public void rangeDecode(ByteBuffer inBuffer, int sym_low, int sym_freq, int tot_freq){
+    protected void rangeDecode(ByteBuffer inBuffer, int sym_low, int sym_freq, int tot_freq){
         code -= sym_low * range;
         range *= sym_freq;
 
@@ -39,12 +39,38 @@ public void rangeDecode(ByteBuffer inBuffer, int sym_low, int sym_freq, int tot_
         }
     }
 
-    public int rangeGetFrequency(final int tot_freq){
+    protected int rangeGetFrequency(final int tot_freq){
         range =  (long) Math.floor(range / tot_freq);
         return (int) Math.floor(code / range);
     }
 
-    public void rangeShiftLow(ByteBuffer outBuffer) {
+    protected void rangeEncode(final ByteBuffer outBuffer, final int sym_low, final int sym_freq, final int tot_freq){
+        long old_low = low;
+        range = (long) Math.floor(range/tot_freq);
+        low += sym_low * range;
+        low &= 0xFFFFFFFFL; // keep bottom 4 bytes, shift the top byte out of low
+        range *= sym_freq;
+
+        if (low < old_low) {
+            carry = true;
+        }
+
+        // Renormalise if range gets too small
+        while (range < (1<<24)) {
+            range <<= 8;
+            rangeShiftLow(outBuffer);
+        }
+
+    }
+
+    protected void rangeEncodeEnd(final ByteBuffer outBuffer){
+        //TODO: Where is the magic number 5 coming from?
+        for(int i = 0; i < 5; i++){
+            rangeShiftLow(outBuffer);
+        }
+    }
+
+    private void rangeShiftLow(ByteBuffer outBuffer) {
         // rangeShiftLow tracks the total number of extra bytes to emit and
         // carry indicates whether they are a string of 0xFF or 0x00 values
 
@@ -74,30 +100,4 @@ public void rangeShiftLow(ByteBuffer outBuffer) {
         low = low<<8 & (0xFFFFFFFFL); // force low to be +ve
     }
 
-    public void rangeEncode(final ByteBuffer outBuffer, final int sym_low, final int sym_freq, final int tot_freq){
-        long old_low = low;
-        range = (long) Math.floor(range/tot_freq);
-        low += sym_low * range;
-        low &= 0xFFFFFFFFL; // keep bottom 4 bytes, shift the top byte out of low
-        range *= sym_freq;
-
-        if (low < old_low) {
-            carry = true;
-        }
-
-        // Renormalise if range gets too small
-        while (range < (1<<24)) {
-            range <<= 8;
-            rangeShiftLow(outBuffer);
-        }
-
-    }
-
-    public void rangeEncodeEnd(final ByteBuffer outBuffer){
-        //TODO: Where is the magic number 5 coming from?
-        for(int i = 0; i < 5; i++){
-            rangeShiftLow(outBuffer);
-        }
-    }
-
 }

src/main/java/htsjdk/samtools/cram/compression/range/RangeDecode.java

@@ -5,6 +5,7 @@
 import htsjdk.samtools.cram.compression.rans.Utils;
 
 import java.nio.ByteBuffer;
+import java.nio.ByteOrder;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -13,17 +14,15 @@ public class RangeDecode {
     private static final ByteBuffer EMPTY_BUFFER = ByteBuffer.allocate(0);
 
     public ByteBuffer uncompress(final ByteBuffer inBuffer) {
+        inBuffer.order(ByteOrder.LITTLE_ENDIAN);
         return uncompress(inBuffer, 0);
     }
 
-    public ByteBuffer uncompress(final ByteBuffer inBuffer, int outSize) {
+    private ByteBuffer uncompress(final ByteBuffer inBuffer, int outSize) {
         if (inBuffer.remaining() == 0) {
             return EMPTY_BUFFER;
         }
 
-        // TODO: little endian?
-//        inBuffer.order(ByteOrder.LITTLE_ENDIAN);
-
         // the first byte of compressed stream gives the formatFlags
         final int formatFlags = inBuffer.get() & 0xFF;
         final RangeParams rangeParams = new RangeParams(formatFlags);

src/main/java/htsjdk/samtools/cram/compression/range/RangeEncode.java

@@ -16,15 +16,14 @@ public ByteBuffer compress(final ByteBuffer inBuffer, final RangeParams rangePar
             return EMPTY_BUFFER;
         }
 
-        ByteBuffer outBuffer = allocateOutputBuffer(inBuffer.remaining());
+        final ByteBuffer outBuffer = allocateOutputBuffer(inBuffer.remaining());
         outBuffer.order(ByteOrder.BIG_ENDIAN);
         final int formatFlags = rangeParams.getFormatFlags();
         outBuffer.put((byte) (formatFlags));
 
         if (!rangeParams.isNosz()) {
             // original size is not recorded
-            int insize = inBuffer.remaining();
-            Utils.writeUint7(insize,outBuffer);
+            Utils.writeUint7(inBuffer.remaining(),outBuffer);
         }
 
         ByteBuffer inputBuffer = inBuffer;
@@ -68,7 +67,7 @@ public ByteBuffer compress(final ByteBuffer inBuffer, final RangeParams rangePar
             outBuffer.rewind(); // set position to 0
             return outBuffer;
         } else if (rangeParams.isExternalCompression()){
-            byte[] rawBytes = new byte[inputBuffer.remaining()];
+            final byte[] rawBytes = new byte[inputBuffer.remaining()];
             inputBuffer.get( rawBytes,inBuffer.position(), inputBuffer.remaining());
             final BZIP2ExternalCompressor compressor = new BZIP2ExternalCompressor();
             final byte [] extCompressedBytes = compressor.compress(rawBytes);
@@ -79,16 +78,16 @@ public ByteBuffer compress(final ByteBuffer inBuffer, final RangeParams rangePar
         } else if (rangeParams.isRLE()){
             switch (rangeParams.getOrder()) {
                 case ZERO:
-                    return compressRLEOrder0(inputBuffer, rangeParams, outBuffer);
+                    return compressRLEOrder0(inputBuffer, outBuffer);
                 case ONE:
-                    return compressRLEOrder1(inputBuffer, rangeParams, outBuffer);
+                    return compressRLEOrder1(inputBuffer, outBuffer);
             }
         } else {
             switch (rangeParams.getOrder()) {
                 case ZERO:
-                    return compressOrder0(inputBuffer, rangeParams, outBuffer);
+                    return compressOrder0(inputBuffer, outBuffer);
                 case ONE:
-                    return compressOrder1(inputBuffer, rangeParams, outBuffer);
+                    return compressOrder1(inputBuffer, outBuffer);
             }
 
         }
@@ -97,7 +96,6 @@ public ByteBuffer compress(final ByteBuffer inBuffer, final RangeParams rangePar
 
     private ByteBuffer compressOrder0 (
             final ByteBuffer inBuffer,
-            final RangeParams rangeParams,
             final ByteBuffer outBuffer) {
 
         int maxSymbol = 0;
@@ -110,9 +108,9 @@ private ByteBuffer compressOrder0 (
         maxSymbol++; // TODO: Is this correct? Not what spec states!!
 
         // TODO: initialize byteModel -> set and reset symbols?
-        ByteModel byteModel = new ByteModel(maxSymbol);
+        final ByteModel byteModel = new ByteModel(maxSymbol);
         outBuffer.put((byte) maxSymbol);
-        RangeCoder rangeCoder = new RangeCoder();
+        final RangeCoder rangeCoder = new RangeCoder();
         for (int i = 0; i < inSize; i++){
             byteModel.modelEncode(outBuffer,rangeCoder,inBuffer.get(i)&0xFF);
         }
@@ -124,7 +122,6 @@ private ByteBuffer compressOrder0 (
 
     private ByteBuffer compressOrder1 (
             final ByteBuffer inBuffer,
-            final RangeParams rangeParams,
             final ByteBuffer outBuffer) {
         int maxSymbol = 0;
         final int inSize = inBuffer.remaining();
@@ -145,7 +142,7 @@ private ByteBuffer compressOrder1 (
         outBuffer.put((byte) maxSymbol);
 
         // TODO: should we pass outBuffer to rangecoder?
-        RangeCoder rangeCoder = new RangeCoder();
+        final RangeCoder rangeCoder = new RangeCoder();
 
         int last = 0;
         for (int i = 0; i < inSize; i++ ){
@@ -162,25 +159,24 @@ private ByteBuffer compressOrder1 (
 
     private ByteBuffer compressRLEOrder0 (
             final ByteBuffer inBuffer,
-            final RangeParams rangeParams,
             final ByteBuffer outBuffer) {
         int maxSymbols = 0;
-        int inSize = inBuffer.remaining();
+        final int inSize = inBuffer.remaining();
         for (int i = 0; i < inSize; i++) {
             if (maxSymbols < (inBuffer.get(i) & 0xFF)) {
                 maxSymbols = inBuffer.get(i) & 0xFF;
             }
         }
         maxSymbols++;  // FIXME not what spec states!
 
-        ByteModel modelLit = new ByteModel(maxSymbols);
+        final ByteModel modelLit = new ByteModel(maxSymbols);
         final List<ByteModel> byteModelRunsList = new ArrayList(258);
 
         for (int i=0; i <= 257; i++){
             byteModelRunsList.add(i,new ByteModel(4));
         }
         outBuffer.put((byte)maxSymbols);
-        RangeCoder rangeCoder = new RangeCoder();
+        final RangeCoder rangeCoder = new RangeCoder();
 
 
         int i = 0;
@@ -192,7 +188,6 @@ private ByteBuffer compressRLEOrder0 (
             }
             run--; // Check this!!
             int rctx = inBuffer.get(i) & 0xFF;
-            int last = inBuffer.get(i) & 0xFF;
             i += run+1;
             int part = run >=3 ? 3 : run;
             byteModelRunsList.get(rctx).modelEncode(outBuffer, rangeCoder, part);
@@ -213,10 +208,9 @@ private ByteBuffer compressRLEOrder0 (
 
     private ByteBuffer compressRLEOrder1 (
             final ByteBuffer inBuffer,
-            final RangeParams rangeParams,
             final ByteBuffer outBuffer) {
         int maxSymbols = 0;
-        int inSize = inBuffer.remaining();
+        final int inSize = inBuffer.remaining();
         for (int i = 0; i < inSize; i++) {
             if (maxSymbols < (inBuffer.get(i) & 0xFF)) {
                 maxSymbols = inBuffer.get(i) & 0xFF;
@@ -233,7 +227,7 @@ private ByteBuffer compressRLEOrder1 (
             byteModelRunsList.add(i,new ByteModel(4));
         }
         outBuffer.put((byte)maxSymbols);
-        RangeCoder rangeCoder = new RangeCoder();
+        final RangeCoder rangeCoder = new RangeCoder();
 
 
         int i = 0;
@@ -291,8 +285,7 @@ private ByteBuffer encodePack(
         } else if (numSymbols <= 2) {
 
             // 1 bit per value
-            int dataSize = (int) Math.ceil((double) inSize/8);
-            data = ByteBuffer.allocate(dataSize);
+            data = ByteBuffer.allocate((int) Math.ceil((double) inSize/8));
             int j = -1;
             for (int i = 0; i < inSize; i ++) {
                 if (i % 8 == 0) {
@@ -303,8 +296,7 @@ private ByteBuffer encodePack(
         } else if (numSymbols <= 4) {
 
             // 2 bits per value
-            int dataSize = (int) Math.ceil((double) inSize/4);
-            data = ByteBuffer.allocate(dataSize);
+            data = ByteBuffer.allocate((int) Math.ceil((double) inSize/4));
             int j = -1;
             for (int i = 0; i < inSize; i ++) {
                 if (i % 4 == 0) {
@@ -315,8 +307,7 @@ private ByteBuffer encodePack(
         } else {
 
             // 4 bits per value
-            int dataSize = (int) Math.ceil((double)inSize/2);
-            data = ByteBuffer.allocate(dataSize);
+            data = ByteBuffer.allocate((int) Math.ceil((double)inSize/2));
             int j = -1;
             for (int i = 0; i < inSize; i ++) {
                 if (i % 2 == 0) {