Unsolved How stupid is QImage implemented??
-
i could improve the code using quint, qfile,.. and by handling the unsuccessful tries an other way. This decreases the time from ~142,5 to ~102,4.
Now i used scanline (thanks for the tipp) and now i'm somewhere around 89. But the images are no longer loaded correctly.Maybe someone of you find the problem:
The Colors are wrong and it looks a bit wrong positioned, too.
Here the old code:
for (unsigned int y = 0; y < ui32Height; y++) { for (unsigned int x = 0; x < ui32Width; x++) { int valr = vui8Pixels.at(y * ui32Width * ui32BpP / 8 + x * ui32BpP / 8 + 2); int valg = vui8Pixels.at(y * ui32Width * ui32BpP / 8 + x * ui32BpP / 8 + 1); int valb = vui8Pixels.at(y * ui32Width * ui32BpP / 8 + x * ui32BpP / 8); int vala = 255; if (ui32BpP == 32) vala = vui8Pixels.at(y * ui32Width * ui32BpP / 8 + x * ui32BpP / 8 + 3); QColor value(valr, valg, valb, vala); img.setPixel(x, ui32Width - 1 - y, value.rgba()); } }
and here the new one
for (unsigned int y = 0; y < ui32Height; y++) { QRgb* imgLine = (QRgb*)img.scanLine(y); for (unsigned int x = 0; x < ui32Width; x++) { int valr = vui8Pixels.at(y * ui32Width * ui32BpP / 8 + x * ui32BpP / 8 + 2); int valg = vui8Pixels.at(y * ui32Width * ui32BpP / 8 + x * ui32BpP / 8 + 1); int valb = vui8Pixels.at(y * ui32Width * ui32BpP / 8 + x * ui32BpP / 8); int vala = 255; if (ui32BpP == 32) vala = vui8Pixels.at(y * ui32Width * ui32BpP / 8 + x * ui32BpP / 8 + 3); QColor value(valr, valg, valb, vala); imgLine[x] = value.rgba(); } }
==EDIT==
i fixed it. For some reason the picture was mirrored and R and B was swapped. now it just takes ~78MS vs Qt 428MS o.O
https://git.rwth-aachen.de/carstenf/OpenGL/blob/master/QtMeshViewer/Header/tga.hif you have further ideas about how to improve, let me know.
-
@QT-static-prgm said in How stupid is QImage implemented??:
QRgb* imgLine = (QRgb*)img.scanLine(y);
This is still a reinterpret cast, so you may run into problems on OSX vs Linux/Windows (see Chris's 3rd or 4th point on byte ordering).
QColor value(valr, valg, valb, vala);
Don't convert into color (which is much more complex), rather use the qRgba() function.
-
If you want to go full speed ahead I would do a couple things:
- further specialize the algorithm for 24 and 32 bit
- explicitly handle the endianness so you don't have to go through it on a byte by byte level
- avoid additional allocations and buffers. Do everything in-place.
Here's a sample for the uncompressed 32bit little endian variant:
if (ui32BpP == 32) { int lineWidth = ui32Width * ui32BpP / 8; QImage img(ui32Width, ui32Height, QImage::Format_RGBA8888); for (int i = ui32Height - 1; i >= 0; --i) file.read(reinterpret_cast<char*>(img.scanLine(i)), lineWidth); #ifdef Q_LITTLE_ENDIAN return qMove(img).rgbSwapped(); //qMove makes it use in-place swap instead of a copy #elif Q_BIG_ENDIAN swapTheBytesInOtherWay(img); return img; #endif }
For 24 bit image you can use
QImage::Format_RGB888
internal format so you can do it as fast.
I got another 4x speedup compared to your version. You could probably optimize the rgb swap further with some clever SSE intrinsics. -
Gonna try this tomorrow. But I think swapping r and b won't be the problem. But tga holds the data bgra and qrgb is defined argb so it's not done by swapping just r and b. A needs to be moved from back to front
-
@QT-static-prgm said in How stupid is QImage implemented??:
But tga holds the data bgra and qrgb is defined argb so it's not done by swapping just r and b. A needs to be moved from back to front
It doesn't matter how
QRgb
stores the data. I'm not using it.
tga stores the bytes in little endian ARGB so the actual order of bytes in the file is [0]BB [1]GG [2]RR [3]AA.
I'm using internal formatFormat_RGBA8888
and on little endian platform this means the actual bytes in memory are stored as [0]RR [1]GG [2]BB [3]AA. As you can see to convert one to the other swapping R and B is enough.
If you want to go through QRgb then yes, you need extra work to put the bytes in right order, but it's unnecessary here and makes it confusing. -
wow, thank you. i'm always confused by those format things.
This is the final code. it can decide whether it is 32 or 24 and chooses the correct internal format.
i don't really wanna touch the compressed part, since it is not relevant for me, but maybe i'll speed it up in the future.if (ui32PicType == 2 && (ui32BpP == 24 || ui32BpP == 32)) { img = QImage(ui32Width, ui32Height, ui32BpP == 32 ? QImage::Format_RGBA8888 : QImage::Format_RGB888); int lineWidth = ui32Width * ui32BpP / 8; for (int i = ui32Height - 1; i >= 0; --i) file.read(reinterpret_cast<char*>(img.scanLine(i)), lineWidth); }
This code makes ~3-4ms!!!!
-
@Chris-Kawa Any ideas how to read directly to QString??
Before i had something like this:
struct{ char name[5]; ... } ... m_file.read(F2V(tmp_header->name[0]), sizeof(tmp_header->name) - 1);
so i have my struct with an array of the size 5 and read in a string of the size 4 (last need to be the \0 character)
Now i'd like to change it to this:
struct{ QString name; ... } ... tmp_header->name.reserve(4); m_file.read(F2V(tmp_header->name), sizeof(tmp_header->name) );
not sure if it needs to be 4 or 5, but non of them do their work. Already tried QBytearray, but this didn't worked either.
-
QString uses variable length Unicode representation with 16bit codepoints. Since you've got an 8bit string you can't read directly into a QString. You'll need to convert.
The easiest way to do it is:tmp_header->name = QString::fromUtf8(m_file.read(4)); //if your string is Utf-8 or ASCII //or tmp_header->name = QString::fromLatin1(m_file.read(4)); //if your string is Latin-1 or ASCII
This variant of
QFile::read
will allocate new QByteArray every time so if you want to avoid that you can create one yourself and reuse it:QByteArray arr(4, 0); m_file.read(arr.data(), 4); tmp_header->name = QString::fromUtf8(arr);
Btw. Many of Qt data classes like QByteArray or QString are non-trivial and use implicit sharing so just directly casting them to the data type pointer is an error (I assume your F2V does something like that?). You'll get a pointer that points to some internal implementation detail not to the data. Qt's storage classes have access methods (usually called
data()
andconstData()
) that give you pointers to the underlying storage. -
Define F2V(value) reinterpret_cast<char*>(&value) is just an "shortcut" that makes the code more readable in my opinion.
-
@Chris-Kawa i tried those variants:
struct{ QByteArray name = QByteArray(4,0); ... } ... m_file.read(tmp_header->name.data(), 4);
~90ms
struct{ QString name; ... } ... tmp_header->name = QString::fromUtf8(m_file.read(4));
~100ms
struct{ QString name; ... } ... char tmpName[5] = { 0 }; m_file.read(F2V(tmpName[0]), sizeof(tmpName) -1); tmp_header->name = QString(tmpName);
~20ms
So both are not fast enough ;)