2
\$\begingroup\$

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;
}
\$\endgroup\$
0

2 Answers 2

4
\$\begingroup\$

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;
    } 
\$\endgroup\$
2
  • \$\begingroup\$ Thanks, I have to look at what Array class provides! Though, guess I should check whether it's actually faster or not. \$\endgroup\$
    – Keikoku
    Commented Aug 11, 2012 at 4:01
  • \$\begingroup\$ It really should be faster :) \$\endgroup\$
    – WouterH
    Commented Aug 11, 2012 at 4:04
3
\$\begingroup\$

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;
        }
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Yes, I defined a separate header class because it seemed like something nice to do, but I wasn't sure how to justify it. The positiveMask is just used because the format uses the high-order bit to identify the packet type and the rest of the bits to store a number. I didn't think about setting it as a const though and that would avoid having to compute it again and again when it's always the same value. \$\endgroup\$
    – MxLDevs
    Commented Aug 15, 2012 at 19:29
  • \$\begingroup\$ I see ... this should execute a bit faster but it is harder to understand. I like using something like: stackoverflow.com/questions/2431732/… Since there is only one bit that drives this, you do not need to have two separate if statements. You could do if(cond) { <doSmthIfYes> return;} <doSmthIfNo> instead of if (...) {...} else {...} but it is just a matter of preference; either one will execute fast enough. \$\endgroup\$
    – Leonid
    Commented Aug 15, 2012 at 19:50

Your Answer

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

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