3
\$\begingroup\$

This code is working and was tested for over 10000 images. I have many headerless images, i.e images which are binary files with known dimensions. Some of the images have a visible area which is different from the size of the binary files which have padding.

For example, the image is 100x100, however the file size is 100x164, since padding was added to each line. So the user inputs tells me that and I remove that part and return a 100x100 image. The image has different sizes and bits per pixels.

Please comment about my style of code in C#, and performance issues I might have.

public static byte[] FromHeaderless(string path, int width, int height, int bitsPerPixel, bool isBigEndian, bool isPerformShift = true, int extraColsLeft = 0, int extraColsRight = 0, int extraLinesTop = 0, int extraLinesBottom = 0)
{
    byte[] fileData = null;

    if (!File.Exists(path))
    {
        throw new FileNotFoundException(path);
    }

    fileData = File.ReadAllBytes(path);

    return DecodeHeaderless(fileData, width, height, bitsPerPixel, isBigEndian,isPerformShift, extraColsLeft, 
                            extraColsRight, extraLinesTop, extraLinesBottom);
}

public static byte[] DecodeHeaderless(byte[] fileData, int width, int height, int bitsPerPixel,
    bool isBigEndian, bool isPerformShift = true, int extraColsLeft = 0, int extraColsRight = 0, 
    int extraLinesTop = 0, int extraLinesBottom = 0)
{
    var bytesPerPixel = (bitsPerPixel + 7) / 8;

    int headerLength = fileData.Length - ((width + extraColsLeft + extraColsRight) * (height + extraLinesBottom + extraLinesTop) * bytesPerPixel);

    if (bytesPerPixel == 1)
    {
        byte[] newImage = new byte[width * height * bytesPerPixel];

        if (extraColsLeft == 0 && extraColsRight == 0 && extraLinesBottom == 0 && extraLinesTop == 0)
        {
            Buffer.BlockCopy(fileData, headerLength, newImage, 0, fileData.Length - headerLength);
        }
        else
        {
            int currentBufferIndex = 0;
            int currentDataIndex = headerLength + extraColsLeft + extraLinesTop * (width + extraColsLeft + extraColsRight);
            Parallel.For(0, height, i =>
            {
                Buffer.BlockCopy(fileData, currentDataIndex, newImage, currentBufferIndex, width);
                currentDataIndex = currentDataIndex + width + extraColsRight + extraColsLeft;
                currentBufferIndex = currentBufferIndex + width;
            }
            );
        }
        return newImage;
    }

    if (bytesPerPixel == 2)
    {
        var dataSize = (width + extraColsRight + extraColsLeft) * (height + extraLinesBottom + extraLinesTop) * bytesPerPixel;
        byte[] newImage = new byte[width * height * bytesPerPixel];
        var sdata = new short[dataSize / 2];

        if (isBigEndian == false)
        {
            for (int i = headerLength, shortIndex = 0; i < dataSize; i += 2, shortIndex++)
            {
                CopyBytesToShort(fileData[i], fileData[i + 1], out sdata[shortIndex], bitsPerPixel, isPerformShift);
            }
        }
        else
        {
            for (int i = headerLength, shortIndex = 0; i < dataSize; i += 2, shortIndex++)
            {
                CopyBytesToShort(fileData[i + 1], fileData[i], out sdata[shortIndex], bitsPerPixel, isPerformShift);
            }
        }

        CopyRelevantImageData(sdata, newImage, bytesPerPixel, width, height, extraLinesTop, extraLinesBottom, extraColsLeft, extraColsRight);
        return newImage;
    }
    return null;
}


private static void CopyBytesToShort(byte byte1, byte byte2, out short retShort, int bitsPerPixel, bool isPerformShift)
{
    short lsb, msb;
    lsb = byte1;
    msb = byte2;
    if (isPerformShift)
    {
        lsb <<= 16 - bitsPerPixel;
        msb <<= (16 - (bitsPerPixel - 8));
    }
    else
    {
        msb <<= 8;
    }
    retShort = (short)(msb | lsb);
}

private static void CopyRelevantImageData(short[] sourceData, byte[] destData, int bytesPerPixel, int width, int height, int extraLinesTop, int extraLinesBottom, int extraColsLeft, int extraColsRight)
{

    if (extraColsLeft == 0 && extraColsRight == 0 && extraLinesBottom == 0 && extraLinesTop == 0)
    {
        Buffer.BlockCopy(sourceData, 0, destData, 0, sourceData.Length * bytesPerPixel);
    }
    else
    {
        int currentBufferIndex = 0;
        int currentDataIndex = extraLinesTop * (width + extraColsLeft + extraColsRight) + extraColsLeft;

        for (int i = 0; i < height; i++)
        {
            Buffer.BlockCopy(sourceData, currentDataIndex * bytesPerPixel, destData, currentBufferIndex, width * bytesPerPixel);
            currentDataIndex += (width + extraColsRight + extraColsLeft);
            currentBufferIndex += (width * bytesPerPixel);
        }
    }
}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

FromHeaderless has ten parameters, ditto DecodeHeaderless. That's waaaay too many. Once you go beyond four or so, you need to consider creating a specific class where each of those parameters is a property. That way you only need to pass this class, and adding/removing parameters is much easier. Also, you don't risk to mix up parameters of the same type e.g. width and height, or extraColsLeft, extraColsRight, extraLinesTop and extraLinesBottom.


I would move all of the logic inside if (bytesPerPixel == 1) and if (bytesPerPixel == 2) to a separate method each. Each of these cases follows a completely different logic and to me it just makes sense to "de-clutter" DecodeHeaderless.


I would move the if (isBigEndian == false) logic to CopyBytesToShort, to avoid repeating the same thing with only a parameter change as you're doing here:

    if (isBigEndian == false)
    {
        for (int i = headerLength, shortIndex = 0; i < dataSize; i += 2, shortIndex++)
        {
            CopyBytesToShort(fileData[i], fileData[i + 1], out sdata[shortIndex], bitsPerPixel, isPerformShift);
        }
    }
    else
    {
        for (int i = headerLength, shortIndex = 0; i < dataSize; i += 2, shortIndex++)
        {
            CopyBytesToShort(fileData[i + 1], fileData[i], out sdata[shortIndex], bitsPerPixel, isPerformShift);
        }
    }
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.