Refactoring a TGA image parser

Question

I have followed the TGA specs described here (the info at the end of the page is what this method is based on) and have written a method that takes a TGA file's pixel data section and creates a bitmap out of it.

It works, but the code looks sloppy. Particularly, when I'm actually reading the bytes. You'll notice that I do some sort of odd foreach loop in order to repeatly add the same bytes to my array.

Here's the method that reads the pixels. It returns an array of bytes representing the pixels.

Is there a way for me to re-write the inner loop in the 128 case to NOT use a foreach in order to set the same sequence of bytes to an array? I imagine I will find myself putting bytes into an array often and it would be nice to know other ways to do it.

private byte[] getImageType10()
{
    // header stores total bits per pixel
    int bytesPerPixel = header.bpp / 8; 
    int total = header.width * header.height * bytesPerPixel;

    // number of bytes we've read so far
    int count = 0;                      
    int repeat;
    byte packetHdr;
    int packetType;

    // temp storage
    byte[] bytes;                       
    byte[] pixData = new byte[total];

    while (count < total)
    {
        packetHdr = inFile.ReadByte();
        packetType = packetHdr & (1 << 7);

        // RLE packet
        if (packetType == 128)
        {
            // packet stores number of times following pixel is repeated
            repeat = (packetHdr & ~(1 << 7)) + 1;
            bytes = inFile.ReadBytes(bytesPerPixel);
            for (int j = 0; j < repeat; j++)
            {
                foreach (byte b in bytes)
                {
                    pixData[count] = b;
                    count += 1;
                }
            }
        }

        // raw packet
        else if (packetType == 0)
        {
            // packet stores number of pixels that follow
            repeat = ((packetHdr & ~(1 << 7)) + 1) * bytesPerPixel;
            for (int j = 0; j < repeat; j++)
            {
                pixData[count] = inFile.ReadByte();
                count += 1;
            }
        }
    }
    return pixData;
}

Answer 1

The 128 case can be optimized as:

    if (packetType == 128) 
    { 
        // packet stores number of times following pixel is repeated 
        repeat = (packetHdr & ~(1 << 7)) + 1; 
        bytes = inFile.ReadBytes(bytesPerPixel);
        for (int j = 0; j < repeat; j++) 
        {
            bytes.CopyTo(pixData, count);
            count += bytes.Length;
        }
    } 

The 0 case can be optimized as:

    if (packetType == 0) 
    { 
        // packet stores number of pixels that follow 
        repeat = ((packetHdr & ~(1 << 7)) + 1) * bytesPerPixel;
        bytes = inFile.ReadBytes(repeat);
        bytes.CopyTo(pixData, count);
        count += bytes.Length;
    } 

Answer 2

My answer is heavily based on that by @WouterH, so if you upvote me, then you must upvote Wouter twice. What I did is provided a "full" code listing, cleaned up some variable names and comments (many comments were removed after the proper variable names were used), added a class for the image header and added the placeholder for the error checking. I am not too happy with the magic constant positiveMask because I do not fully understand what it does. You could replace code contracts with exceptions in order to use fewer assemblies. You can speed the code up a bit by pre-computing some variables in the ImageHeader class once. Other improvements are possible, but we have not seen your entire code, so it is hard to tell. I would also split up this one file into two different ones - one per class.

namespace TGAImageParser
{
    using System;
    using System.Diagnostics.Contracts;
    using System.IO;

    // Using a class like this helps to avoid http://c2.com/cgi/wiki?DataEnvy
    internal class TGAImageHeader
    {
        public const int BitsInByte = 8; // const is ok over readonly here since this will not change in the future, right?

        public readonly int BytesPerPixel;
        public readonly int Width;
        public readonly int Height;

        public TGAImageHeader(int bytesPerPixel, int width, int height)
        {
            // Check the inputs thoroughly here.
            Contract.Requires(width > 0, "Image must be at least one pixel wide."); // Or is it zero pixels wide?
            // ... and so on.

            this.BytesPerPixel = bytesPerPixel;
            this.Width = width;
            this.Height = height;
        }

        public int BitsPerPixel
        {
            get
            {
                return this.BytesPerPixel * BitsInByte;
            }
        }

        // If this gets called often, then pre-compute it once.
        public int TotalBytes
        {
            get
            {
                return this.BytesPerPixel * this.TotalPixels;
            }
        }

        // If this gets called often, then pre-compute it once.
        public int TotalPixels
        {
            get
            {
                return this.Width * this.Height;
            }
        }
    }

    public class TGAImageParser
    {
        public const int RLEPacketType = 128;
        public const int RawPacketType = 0;

        private const byte positiveMask = 1 << 7; // TODO: Come up with a better name! What does it do?

        public static byte[] GetImageType10Bytes(BinaryReader inFile, TGAImageHeader imageHeader)
        {
            int numBytesReadSoFar = 0;
            int numTimesPixelIsRepeated;
            byte packetHeaderByte;
            int packetType;

            byte[] tempStorageBuffer;
            byte[] pixData = new byte[imageHeader.TotalBytes];

            while (numBytesReadSoFar < imageHeader.TotalBytes)
            {
                packetHeaderByte = inFile.ReadByte();
                packetType = packetHeaderByte & positiveMask;

                if (packetType == RLEPacketType)
                {
                    numTimesPixelIsRepeated = (packetHeaderByte & ~positiveMask) + 1;
                    tempStorageBuffer = inFile.ReadBytes(imageHeader.BytesPerPixel);
                    for (int i = 0; i < numTimesPixelIsRepeated; i++)
                    {
                        tempStorageBuffer.CopyTo(pixData, numBytesReadSoFar);
                        numBytesReadSoFar += tempStorageBuffer.Length;
                    }

                    continue;
                }

                if (packetType == RawPacketType)
                {
                    numTimesPixelIsRepeated = ((packetHeaderByte & ~positiveMask) + 1) * imageHeader.BytesPerPixel;
                    tempStorageBuffer = inFile.ReadBytes(numTimesPixelIsRepeated);
                    tempStorageBuffer.CopyTo(pixData, numBytesReadSoFar);
                    numBytesReadSoFar += tempStorageBuffer.Length;

                    continue;
                }

                // TODO: report an error here?
            }

            return pixData;
        }
    }
}