Printable View
In the meantime I thoroughly examined your .bmp file with a hex editor and concluded it's perfectly healthy. In particular the odd width of the image (159) is not problematic: The rows are cleanly padded to a size of an integral multiple of a DWORD. Hence, the problem must originate elsewhere...
For what it's worth, I downloaded the bitmap and it loaded fine (on XP SP2 w/Visual Studio 2008).
Code:HANDLE hBitmap = (HBITMAP)::LoadImage(0 ,
_T("C:\\Documents and Settings\\arjay\\My Documents\\My Pictures\\4by4bitmap.bmp"),
IMAGE_BITMAP,
0,
0,
LR_LOADFROMFILE | LR_CREATEDIBSECTION );
How incredibly bizarre! I copy-pasted your code and put in the pathname as follows:
and the error is #8. There is no way that is true. I can run graphic intense games on this thing. How frustrating! :(Code:m_bitmap = (HBITMAP)::LoadImage(0 ,_T("C:\\4by4bitmap.bmp"), IMAGE_BITMAP, 0, 0, LR_LOADFROMFILE | LR_CREATEDIBSECTION );
DWORD error = GetLastError();
I have poured through the internet looking for examples for loading bitmaps, and have Frankensteined together the following function.
The trouble is, by the time it gets near the bottom of the function, the destination bits inexplicably vanish. . . at least that's what it seems. It appears to happen during the large loop that transfers data from the temporary buffer to the destination bits, but I can't seem to figure out the problem. (It's the loop that uses int j, just after if(bmih.biHeight > 0) )
What's wrong with this code? :,(Code:int CDynamicBitmapCreationDlg::load_bitmap_from_file(char *file, CBitmap* p_newbitmap)
{
if(!p_newbitmap)
{
MessageBox("Error location 0");
return 0;
}
//First we open the file the binary mode
FILE* in = fopen(file,"rb");
//Next we read the BITMAPFILEHEAR. . .
BITMAPFILEHEADER bmfh;
size_t result = fread(&bmfh,sizeof(BITMAPFILEHEADER),1,in);
if(result < 1)
{
MessageBox("Error location 1");
return 0;
}
if(bmfh.bfType != 19778)
{
MessageBox("Error location 2");
return 0;
}
//Then the BITMAPINFOHEADER. . .
BITMAPINFOHEADER bmih;
result = fread(&bmih,sizeof(BITMAPINFOHEADER),1,in);
if(result < 1)
{
MessageBox("Error location 3");
return 0;
}
//Next comes the palette
RGBQUAD* colors = NULL;
//set the number of colours
int number_of_colors = 1 << bmih.biBitCount;
//load the palette if the bitmap has less than 24 bits per pixel
if(bmih.biBitCount < 24)
{
colors = new RGBQUAD[number_of_colors];
result = fread(colors,sizeof(RGBQUAD),number_of_colors,in);
if(result < number_of_colors)
{
MessageBox("Error location 4");
delete [] colors;
return 0;
}
}
//Now we read in the pixel data
DWORD data_size = bmfh.bfSize - bmfh.bfOffBits;
BYTE* p_initialdata = new BYTE[data_size];
if(p_initialdata == NULL)
{
fclose(in);
if(colors)
delete [] colors;
MessageBox("Error location 5");
return 0;
}
result = fread(p_initialdata,sizeof(BYTE),data_size,in);
if(result < data_size)
{
if(colors)
delete [] colors;
delete [] p_initialdata;
MessageBox("Error location 6");
return 0;
}
//The data has been read. First, check if padding is needed
//byteWidth is the width of the actual image in bytes
//padWidth is the width of the image plus the extra padding
LONG width_in_bytes = 0;
LONG width_plus_padding = 0;
width_in_bytes = width_plus_padding = (LONG)((float)bmih.biWidth*(float)bmih.biBitCount/8.0);
//add any extra space to bring each line to a DWORD boundary
while(width_plus_padding % 4 != 0)
{
width_plus_padding++;
}
//Second, we transfer the data from the temporary buffer to the final buffer,
// adjusting for padding and inversion, if necessary
DWORD actual_size;
int offset;
//set diff to the actual image size(no padding)
actual_size = bmih.biHeight * width_in_bytes;
//allocate memory for the image
BYTE* p_finaldata = new BYTE[actual_size];
if(p_finaldata == NULL)
{
fclose(in);
if(colors)
delete [] colors;
delete [] p_initialdata;
MessageBox("Error location 7");
return 0;
}
if(bmih.biHeight > 0)
{
// The bitmap is inverted, so we'll need to reverse the image and remove the padding
int j = data_size - 3;
offset = width_plus_padding - width_in_bytes;
for( int i = 0 ; i < data_size ; i += 3 )
{
if( (i+offset) % width_plus_padding == 0 )
{
i += offset;
}
*(p_finaldata + j + 2) = *(p_initialdata + i + 0);
*(p_finaldata + j + 1) =* (p_initialdata + i + 1);
*(p_finaldata + j + 0) =* (p_initialdata + i + 2);
j -= 3;
}
}
else
{
// The bitmap is not inverted. We only need to remove the padding
LONG height = bmih.biHeight * -1;
offset = 0;
do
{
memcpy((p_finaldata+(offset*width_in_bytes)),(p_initialdata + (offset*width_plus_padding)),width_in_bytes);
offset++;
} while(offset < height);
}
//Here we begin to construct the BITMAP object
BITMAP bmp;
bmp.bmBitsPixel = bmih.biBitCount;
bmp.bmHeight = bmih.biHeight;
bmp.bmWidth = bmih.biWidth;
bmp.bmType = 0;
bmp.bmWidthBytes = width_plus_padding;
bmp.bmPlanes = bmih.biPlanes;
bmp.bmBits = p_finaldata;
if(!(p_newbitmap->CreateBitmapIndirect(&bmp)))
{
if(colors)
delete [] colors;
delete [] p_initialdata;
delete [] p_finaldata;
MessageBox("Error location 8");
return 0;
}
if(colors)
delete [] colors;
delete [] p_initialdata;
return 1;
}
That's something wrong with your system, and the bitmap is okay. I was able to load the bitmap with exact your original LoadImage line, with the path C:\4by4bitmap.bmp, in Windows XP SP3. Everything went just fine.
Generally, Windows API functions DO NOT obviously set last error to 0 (zero) in case of success.
So, the correct usage of ::GetLastError is
In our case:Code:if(some_winapi_function_failed)
{
call_GetLastError_to_see_what_is_going_wrong.
}
See also: How to get the reason for a failure of a SDK function?Code:HBITMAP hBitmap = (HBITMAP)::LoadImage(NULL,
_T("C:\\4by4bitmap.bmp"),
IMAGE_BITMAP, 0, 0,
LR_LOADFROMFILE|LR_CREATEDIBSECTION);
if(NULL == hBitmap)
{
DWORD dwError = ::GetLastError();
// ...
}
That was humbling. Thank you ovidiucucu. I narrowed the problem to not having set the CStatic's style to SS_BITMAP. *sigh* Silly mistake. The GetLastError thing threw me off. Thanks again.
Well, one problem down. Does anyone know why the function I posted does not load bitmaps from file properly?
First, you should clean the code up, as it won't work correctly if there is a memory related issue. If there is a memory related issue, you have memory leaks and file handles will remain opened with the code as-is when run with any version of Visual Studio above version 6.0. You should change the code to use RAII techniques as I'll attempt to explain below.Quote:
Originally Posted by paradoxresolved
First, use this very simple class for FILE* handling:
Then when you create a FileObject object and assign the FILE* you get when you called fopen(), this object will automatically close the file handle when the function exits without you having to write fclose() in multiple places.Code:#include <stdio.h>
//...
class FileObject
{
FILE* m_pFile;
public:
FileObject( FILE *pFile ) : m_pFile( pFile ) {}
~FileObject() { if (m_pFile) fclose(m_pFile); }
};
This simple addition allows this magic to occur.Code:FILE* in = fopen(file,"rb");
FileObject fObject( in );
Next, let's look at the memory management:
Now you don't need that delete [] colors or p_initialdata all over the place in your code. These will automatically get deleted when that function exits for any reason whatsoever.Code:#include <vector>
//...
std::vector<RGBQUAD> colors;
//...
colors.resize(number_of_colors);
result = fread(&colors[0], sizeof(RGBQUAD), number_of_colors,in);
//...
std::vector<BYTE> p_initialdata(data_size);
The reason why you should really change to vector and the simple file class is the original code below, which does not work for any modern (above 6.0) Visual Studio C++:
By default, operator new[] does not return NULL if there is a problem. Instead, an exception is thrown (std::bad_alloc). If an exception was thrown, your code without using vector has memory leaks, since you never get a chance to delete[] colors, and you have an open file handle, in.Code:BYTE* p_initialdata = new BYTE[data_size];
if(p_initialdata == NULL)
Using vector and the simple file class, if an exception is thrown, the vector automatically cleans itself up and the file gets closed. You can use the nothrow version of new[], but changing to vector and the file class makes things a little more cleaner with less chance of memory leaks and open handles.
Now, this code below will require allocation of memory, since you need to allocate for the image data. So a local vector can't be used here in this context (as it will be destroyed on return). If you had a real image class, then you could use vector, as the lifetime of the vector would have the same lifetime of the image object. But again, a check for NULL is incorrect, as it will never happen (an exception will be thrown).
So here are the proposed changes, given all of the above:
Note there is no need to delete colors or initialdata, as they get cleaned up automatically, plus the file handle gets closed without you having to do so explicitly. There is only one place where dynamic allocation is done, and that is p_finaldata, and that is where you can place the focus without worrying about memory leaks or open handles.Code://set diff to the actual image size(no padding)
actual_size = bmih.biHeight * width_in_bytes;
//allocate memory for the image
BYTE* p_finaldata = new BYTE[actual_size];
//...
*(p_finaldata + j + 2) = p_initialdata[i];
*(p_finaldata + j + 1) = p_initialdata[ i + 1];
*(p_finaldata + j + 0) = p_initialdata[ i + 2];
j -= 3;
//...
memcpy((p_finaldata+(offset*width_in_bytes)),(&p_initialdata[0] + (offset*width_plus_padding)),width_in_bytes);
//...
//Here we begin to construct the BITMAP object
BITMAP bmp;
bmp.bmBitsPixel = bmih.biBitCount;
bmp.bmHeight = bmih.biHeight;
bmp.bmWidth = bmih.biWidth;
bmp.bmType = 0;
bmp.bmWidthBytes = width_plus_padding;
bmp.bmPlanes = bmih.biPlanes;
bmp.bmBits = p_finaldata;
if(!(p_newbitmap->CreateBitmapIndirect(&bmp)))
{
delete [] p_finaldata;
MessageBox("Error location 8");
return 0;
}
return 1;
}
Now, why your program doesn't work, I didn't attempt to run it.
Regards,
Paul McKenzie
Just fixed.Quote:
Originally Posted by Eri523
Check it again:
http://msdn.microsoft.com/en-us/libr....85%29.aspx#11
Great! :)Quote:
Originally Posted by ovidiucucu
Now this is a perfect example of a case where a coincidence of several factors obviously caused us to need 20+ posts to sort out a rather simple issue:
- The OP gets error #8.
- The .NET classes throw an OutOfMemoryException in case of bitmap format issues.
- The MSDN user contribution post reports something that definitely looks related (but probably actually not even is).
At least my own thoughts have been seriously led astray by that mix... :sick:
Will check out your new FAQ entry as soon as I find the time.
Thanks Paul. That does make more sense. Here is the updated code. It still doesn't seem to load up anything though. I tried finding the topic of loading bitmaps in the FAQ but didn't see anything. Was I looking in the wrong place?
This is the third time in five years that I've tried working with the guts of bitmaps, and I always fail miserably. :( I'd like to get this working for my own sanity and education. In the very least, I can stop pestering all of you about it. :)
Any help would of course be greatly appreciated. Thanks! :)Code:// CDynamicBitmapCreationDlg.h
class FileObject
{
FILE* m_pFile;
public:
FileObject( FILE *pFile ) : m_pFile( pFile ) {}
~FileObject() { if (m_pFile) fclose(m_pFile); }
};
// CDynamicBitmapCreationDlg.cpp
int CDynamicBitmapCreationDlg::load_bitmap_from_file(char *file, CBitmap* p_newbitmap)
{
if(!p_newbitmap)
{
MessageBox("Error location 0");
return 0;
}
//First we open the file the binary mode
FILE* in = fopen(file,"rb");
FileObject fObject( in );
//Next we read the BITMAPFILEHEAR. . .
BITMAPFILEHEADER bmfh;
size_t result = fread(&bmfh,sizeof(BITMAPFILEHEADER),1,in);
if(result < 1)
{
MessageBox("Error location 1");
return 0;
}
if(bmfh.bfType != 19778)
{
MessageBox("Error location 2");
return 0;
}
//Then the BITMAPINFOHEADER. . .
BITMAPINFOHEADER bmih;
result = fread(&bmih,sizeof(BITMAPINFOHEADER),1,in);
if(result < 1)
{
MessageBox("Error location 3");
return 0;
}
//Next comes the palette
std::vector<RGBQUAD> colors;
//set the number of colours
int number_of_colors = 1 << bmih.biBitCount;
//load the palette if the bitmap has less than 24 bits per pixel
if(bmih.biBitCount < 24)
{
colors.resize(number_of_colors);
result = fread(&colors[0], sizeof(RGBQUAD), number_of_colors,in);
if(result < number_of_colors)
{
MessageBox("Error location 4");
return 0;
}
}
//Now we read in the pixel data
DWORD data_size = bmfh.bfSize - bmfh.bfOffBits;
std::vector<BYTE> p_initialdata(data_size);
result = fread(&p_initialdata[0],sizeof(BYTE),data_size,in);
if(result < data_size)
{
MessageBox("Error location 6");
return 0;
}
//The data has been read. First, check if padding is needed
//byteWidth is the width of the actual image in bytes
//padWidth is the width of the image plus the extra padding
LONG width_in_bytes = 0;
LONG width_plus_padding = 0;
width_in_bytes = width_plus_padding = (LONG)((float)bmih.biWidth*(float)bmih.biBitCount/8.0);
//add any extra space to bring each line to a DWORD boundary
while(width_plus_padding % 4 != 0)
{
width_plus_padding++;
}
//Second, we transfer the data from the temporary buffer to the final buffer,
// adjusting for padding and inversion, if necessary
DWORD actual_size;
int offset;
//set diff to the actual image size(no padding)
actual_size = bmih.biHeight * width_in_bytes;
//allocate memory for the image
BYTE* p_finaldata = new BYTE[actual_size];
if(bmih.biHeight > 0)
{
// The bitmap is inverted, so we'll need to reverse the image and remove the padding
int j = data_size - 3;
offset = width_plus_padding - width_in_bytes;
for( int i = 0 ; i < data_size ; i += 3 )
{
if( (i+offset) % width_plus_padding == 0 )
{
i += offset;
}
*(p_finaldata + j + 2) = p_initialdata[i];
*(p_finaldata + j + 1) = p_initialdata[i + 1];
*(p_finaldata + j) = p_initialdata[i + 2];
j -= 3;
}
}
else
{
// The bitmap is not inverted. We only need to remove the padding
LONG height = bmih.biHeight * -1;
offset = 0;
do
{
memcpy((p_finaldata+(offset*width_in_bytes)),(&p_initialdata[0] + (offset*width_plus_padding)),width_in_bytes);
offset++;
} while(offset < height);
}
//Here we begin to construct the BITMAP object
BITMAP bmp;
bmp.bmBitsPixel = bmih.biBitCount;
bmp.bmHeight = bmih.biHeight;
bmp.bmWidth = bmih.biWidth;
bmp.bmType = 0;
bmp.bmWidthBytes = width_in_bytes;
bmp.bmPlanes = bmih.biPlanes;
bmp.bmBits = p_finaldata;
if(!(p_newbitmap->CreateBitmapIndirect(&bmp)))
{
delete [] p_finaldata;
MessageBox("Error location 8");
return 0;
}
return 1;
}
I think I've narrowed the problem to the very end of the function where BITMAP is being filled in. The final bit data is preserved at that point, but sometime shortly after the function ends, a call to p_bitmap->GetBitmap reveals that the BITMAP bit pointer is NULL. Is there something special about the way you fill out the BITMAP structure or perhaps something special about how one uses CreateBitmapIndirect?
This is confusing.
First, two aside remarks about your code, given the fact you are using MFC:
- You have defined your own class for manipulating files while MFC has CFile class.
- You are using std::vector<BYTE> while MFC has CByteArray class.
Besides, my post #20 is enough clear: the problem from the title of this thread is because of wrong using of GetLastError and not because of LoadImage function itself.
Then, it has not much sense reinventing the wheel and bittwiddlering in bitmap format structures.
One good approach is to extend CBitmap class by adding FromFile method as follows:
Code:struct CBitmapEx : public CBitmap
{
DWORD FromFile(LPCTSTR szFileName, BOOL bDIBSection = TRUE);
};
Another easier way is to use CImage ATL/MFC class instead of CBitmap.Code:DWORD CBitmapEx::FromFile(LPCTSTR szFileName, BOOL bDIBSection)
{
DWORD dwRet = NO_ERROR;
if(NULL != GetSafeHandle())
DeleteObject();
UINT nFlags = LR_LOADFROMFILE;
if(bDIBSection)
nFlags |= LR_CREATEDIBSECTION;
HBITMAP hBitmap = (HBITMAP)LoadImage(NULL, szFileName,
IMAGE_BITMAP, 0, 0, nFlags);
if(NULL == hBitmap)
dwRet = ::GetLastError();
else
Attach(hBitmap);
return dwRet;
}
Ovidiucucu: Thank you for your reply and solving the GetLastError issue. The FileObject and vector parts of the function were Paul's suggestion in order to clean up the previous version. My current goal (sparked by another of Paul's suggestions) has been shifted to learning how to load bitmaps without using MFC's functions, so I'm taking baby steps in that direction.
This inquery is admittedly not related to the heading of this thread, so I've started a new thread to cover it.
Thanks again.