Thread: What is wrong with this function!!!

Hello, im working on a graphics library for dos and my getpixel function crashes at certain points on the screen. It doesnt crash in standard vga mode (320x200x256) but it does in svga vesa modes, My best bet is it has something to do with bank switching.

Code:

//reads the pixel at xy, value is which one
//in in 256 color mode value doesnt matter but
//in 16m color mode value determines what value to grab
//1 is red 2 is green 3 is blue
int getpixel(unsigned int x, int y, int value)
{
   long offset=0L;
   int val;

   if(x>SCREENWIDTH) // if its off the screen
   return 0;
   if(y>SCREENHEIGHT) // if its off the screen
   return 0;
   if(y<0)
   return 0;


   if(GFXMODE==0) // if its standard vga mode (no bank switching)
   {
        x=((y*SCREENWIDTH)+x);
        asm{
                mov bx, 0A000H
                mov es, bx
                mov bx, x
                mov ax, [es:bx]
	mov val, ax
        }
        return val;
   }
   
   if(BPP==1) // if its a 256 color mode (1 byte per pixel)
   {
        offset=(long)y*SCREENWIDTH+(long)x; //get the offset
        OldBank=bank; //self explanatory 
        bank=offset>>16; // sets the bank
        x=offset; // x is now the offset for the current bank

        if(OldBank!=bank) // if it needs to bank switch
        {
        asm{
                // set read bank
                mov ax, 04F05H
                mov bx, 0
                mov dx, bank
                int 10H

                // set write bank
                mov ax, 04F05H
                mov bx, 1
                mov dx, bank
                int 10H

        }
        }
        // now read the pixel
        asm{ 
                mov bx, 0A000H 
                mov es, bx
                mov bx, x
                mov ax, [es:bx]
                mov val, ax
        }
   	return val;
   }

   // this is pretty much the same as 1 BPP except it reads 3 bytes instead of 1, its 16.7m color mode
   if(BPP==3)
   {
        offset=(long)y*SCREENWIDTH+(long)x;
        offset*=3;
        OldBank=bank;
        bank=offset>>16;
        x=offset;

        if(OldBank!=bank)
        {
        asm{
                // set read bank
                mov ax, 04F05H
                mov bx, 0
                mov dx, bank
                int 10H

                // set write bank
                mov ax, 04F05H
                mov bx, 1
                mov dx, bank
                int 10H

        }
        }

        asm{
                mov bx, 0A000H
                mov es, bx
                mov bx, x
        }

        if(value==3) // if they wanna read blue
        {
        	asm{
        	        mov ax, [es:bx]
        	        mov val, ax
        	}
        	return val;
        }
        if(value==2) // or green
        {
        	asm{
        	        inc bx
        	        mov ax, [es:bx]
        	        mov val, ax
        	}
        	return val;
        }
        if(value==1) // or red
        {
        	asm{
        	        inc bx
        	        inc bx
        	        mov ax, [es:bx]
        	        mov val, ax
        	}
        	return val;
        }

   }

	return 0;
}

thanks for any help

Are you people that stupid, not one out of like a few hundred users can answer this. This is why cprogramming.com sucks, it used to be cool.

Im not sure exactly but my putpixel also draws the wrong color at the same places, its probably either the end or beginning of a bank because it doesnt happen in vga mode.

Its gonna be pretty sad if someone at flashdaddee answers this first considering how few people are over there.

> Are you people that stupid, not one out of like a few hundred users can answer this
Perhaps if you'd waited more than 6 hours for a reply, you would attract more sympathy.

And since this is a C board, not an ASM hacker board with C wrappers, the vast majority of newbies wont be able to help.

Most people only visit once a day, and you should therefore expect to have to wait up to 24 hours for perhaps the right person to answer the question (assuming they exist). The world doesn't begin at EST and end at PST you know.

> offset*=3;
Given that multiples of 3 are not even divisors of 65536, the occasional pixel will straddle two banks in the BPP==3 mode.

I'm taking you at your word that you know banks are 65536 in length, and not say 64000, because both are in some sense 64K.

Range checking x and y wouldn't go amiss - while you're debugging.

Nor would some test code which deliberately loops over all possible x and y values. Then you would be able to at least answer the question - which specific X and Y values cause the problem. Knowing that x=212 and y=478 say would at least help....

The x and y values it crashes at are different for each video mode so it would be poinless to give you those. Im gonna go run up to the puter lab and see if I can find out where it crashes.

Failing to preserve dx across int 10h call
One serious problem that you have is that you are not pushing the value of dx prior to executing int 10h. If int 10h changes the value of dx then when you set currentbank to dx, it is not what you expect it to be. This in itself is incurring that on every pixel your program is bankswitching, even when it does not need to. Make sure you preserve dx across the int 10h call.

Don't check for screen boundaries
Also do not check for the screen boundaries. These checks will slow the entire system down since it will check this for every pixel. Check for this in another functions such as a clipping function. If you are only doing 2D graphics then you probably have no choice but to do those checks both on GetPixel. PutPixel need not check boundaries. Assume that the programmer knows that he or she should not try to get the color of a pixel that does not exist. Similar to C's error checking. It is the programmer's responsibility to be aware of what he or she is doing. This will save a lot of cycles since you do not have to babysit for the programmer.

Problem with 24-bit modes
Salem is right in that in 24 bit mode their will be a time in which half a pixel lies in the next bank. This only happens in 24 bit modes. Very annoying.

This is for 16-bit modes only, 24-bit was way too slow in DOS and not worth the hassle of the bank problem.

This can be altered to GetPixel with a few minor changes.

Code:

void PlotPixel(long poffset,WORD color)
{
   //16 bit color offset
   register int aoffset=poffset & 0xFFFF;
   register int curbank=poffset>>16;
   asm {
     START:
       mov ax,[curbank]
       cmp ax,[bank]
       jz PLOTPIXEL
     SWITCHBANKS:
       mov ax,4f05h
       mov bx,0
       mov dx,curbank
       push dx             //Preserve dx across int 10h
       int 10h
       pop dx  	//Get previous dx off the stack
     UPDATEBANK:
       mov [bank],dx
     PLOTPIXEL:
       mov ax,0a000h
       mov es,ax
       mov di,WORD [aoffset]
       mov ax,color
       stosw
   }
}

It would be even faster to do a STOSD instead of STOSW but Borland does not allow that instruction. You absolutely must push dx prior to int 10h.

Code two separate functions for VESA and non-VESA
I would code two different functions for VESA and non-VESA modes. Use function overrides so that the compiler chooses which one it will use - that way you do not have to waste cycles checking which one to use.

void PlotPixel(long offset,BYTE color);
void PlotPixel(long offset,WORD color);

WORD GetPixel(long offset);
BYTE GetPixel(long offset);

You are checking to see if you are in VESA mode on every pixel. Once you set the mode, you know you are - so to check on each pixel is a waste of precious cycles.

Compute offset elsewhere or do not use x and y coords
Also, do not, if you can help it, compute the pixel offset within the PlotPixel/GetPixel function. You could #define the conversion function or better yet you could set up your graphics to use continuing offsets into the buffer instead of x and y coords. This will take some careful planning and effort on your part, but it will be well worth it.

You can alter my above PlotPixel function to do the reverse op or GetPixel still using stosw - though I've not converted it yet. Also your checks for the RGB stuff will slow the system down. Simply have your function retrieve the word. Then create a function that will take a WORD and return RGB in a struct. You want your pixel retrieval function to just retrieve the pixel - extract the RGB's later.

Here is a slower example of a getpixel function w/o using stosw and converting from word to RGB.
This assumes 565 RGB.

Code:

RGB GetPixel(DWORD offset)
{
  register int aoffset=offset & 0xFFFF;
  register int curbank=offset>>16;
  WORD color=0;
  asm {
   START:
     mov ax,[curbank]
     cmp ax,[bank]
     jz GETPIXEL
   SWITCHBANKS:
     mov ax,4f05h
     mov bx,0
     mov dx,curbank
     push dx
     int 10h
     pop dx	//Get previous dx
  UPDATEBANK:
     mov [bank],dx
  GETPIXEL:
    mov ax,0a000h
    mov es,ax
    mov bx,WORD [aoffset]
    mov ax,es:[bx]
    mov [color],ax
  }
  //Should be elsewhere
  RGB temp;
  temp.grn=(color &0x7e0)>>6;
  temp.red=(color &0xF800)>>11;
  temp.blu=(color & 0x1f);
  return temp;
}

This does work on my machine and it does not crash.
Hope this helps. Not sure why yours is crashing. Just gave some tips on how to speed it up.

For PlotPixel you can get some speed gains by doing this.

Code:

//These are global or in a struct - your choice
BYTE REDS[32];
BYTE BLUS[32];
BYTE GRNS[32];

for (int i=0;i<32;i++)
{
   REDS[i]=i<<11;
   GRNS[i]=i<<6;
   BLUS[i]=i;
}

#define RGBTOWORD (r,g,b) (WORD)((REDS[r])+(GRNS[g])+(BLUS[b]))

WORD color=RGBTOWORD(red,grn,blu);

Your assembly looks pretty tight, though. Let me know when you are ready to chuck this bank switching stuff and move to protected mode.

Questions? PM me and I'll be glad to help. Is this for your new library? Looks good.

GetPixel need not check for boundaries. Sorry, typo.

Thanks for all the good feedback, Im actually just learning assembly and Im not sure of all of the instructions, Ill look up stosw when I get home today. And ya this is for my new library.