Imported PNG file not displayed.
Hi,
In the past few days, importing a PNG file seems to have been broken. The svg:image data seems to be correct but the file is not displayed. Does anybody else see this? Tested with latest SVN on Fedora 9 and 11.
Tav
On 16/9/09 15:44, Tavmjong Bah wrote:
In the past few days, importing a PNG file seems to have been broken. The svg:image data seems to be correct but the file is not displayed. Does anybody else see this? Tested with latest SVN on Fedora 9 and 11.
Bug #429610 in Inkscape: “Imported or opened image isn't displayed”: https://bugs.edge.launchpad.net/inkscape/+bug/429610
hth, ~suv
On 09/16/2009 03:44 PM, Tavmjong Bah wrote:
In the past few days, importing a PNG file seems to have been broken. The svg:image data seems to be correct but the file is not displayed. Does anybody else see this? Tested with latest SVN on Fedora 9 and 11.
Hi Tav, is this on a 32 bit OS?
Diederik
On Thu, 2009-09-17 at 14:17 +0200, Diederik van Lierop wrote:
On 09/16/2009 03:44 PM, Tavmjong Bah wrote:
In the past few days, importing a PNG file seems to have been broken. The svg:image data seems to be correct but the file is not displayed. Does anybody else see this? Tested with latest SVN on Fedora 9 and 11.
Hi Tav, is this on a 32 bit OS?
Yes.
Diederik
On 9/17/09, Diederik van Lierop <mail@...1689...> wrote:
Thanks Tav, this once again confirms my suspicion that something is going wrong with using 64 bits long long integers on a 32 bits OS, or more likely: with the way I implemented it in libnr. This would also explain why I can't reproduce this on my 64 bit Fedora machine.
I didn't see any problems with importing on Win XP, which I believe is 32-bit, but now I notice that when you begin to zoom out on a bitmap, below 4% zoom it starts to shift left and/or randomly disappear. Sorry for the oversight, I have backed out that patch now.
Still, thanks to Diederik for investigating this annoying bug, now that he has identified the source of the probelm I'm sure a correct fix will emerge :)
Index: /home/diedenrezi/eclipse/InkscapeSVN/src/libnr/nr-compose-transform.cpp =================================================================== --- /home/diedenrezi/eclipse/InkscapeSVN/src/libnr/nr-compose-transform.cpp (revision 22193) +++ /home/diedenrezi/eclipse/InkscapeSVN/src/libnr/nr-compose-transform.cpp (working copy) @@ -25,10 +25,10 @@ int nr_have_mmx (void); void nr_mmx_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_0 (unsigned char *px, int w, int h, int rs, const unsigned char *spx, int sw, int sh, int srs, - const long *FFd2s, unsigned int alpha); + const long long *FFd2s, unsigned int alpha); void nr_mmx_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_n (unsigned char *px, int w, int h, int rs, const unsigned char *spx, int sw, int sh, int srs, - const long *FFd2s, const long *FF_S, unsigned int alpha, int dbits); + const long long *FFd2s, const long *FF_S, unsigned int alpha, int dbits); #define NR_PIXOPS_MMX (1 && nr_have_mmx ()) #ifdef __cplusplus } @@ -40,6 +40,7 @@
/* Fixed point precision */ #define FBITS 12 +#define FBITS_HP 18 // In some places we need a higher precision
void nr_R8G8B8A8_N_EMPTY_R8G8B8A8_N_TRANSFORM (unsigned char *px, int w, int h, int rs, const unsigned char *spx, int sw, int sh, int srs, @@ -168,10 +169,10 @@ static void nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_0 (unsigned char *px, int w, int h, int rs, const unsigned char *spx, int sw, int sh, int srs, - const long *FFd2s, unsigned int alpha) + const long long *FFd2s, unsigned int alpha) { - unsigned char *d0; - int FFsx0, FFsy0; + unsigned char *d0; + long long FFsx0, FFsy0; int x, y;
d0 = px; @@ -180,15 +181,15 @@
for (y = 0; y < h; y++) { unsigned char *d; - long FFsx, FFsy; + long long FFsx, FFsy; d = d0; FFsx = FFsx0; FFsy = FFsy0; for (x = 0; x < w; x++) { long sx, sy; - sx = FFsx >> FBITS; + sx = long(FFsx >> FBITS_HP); if ((sx >= 0) && (sx < sw)) { - sy = FFsy >> FBITS; + sy = long(FFsy >> FBITS_HP); if ((sy >= 0) && (sy < sh)) { const unsigned char *s; unsigned int a; @@ -224,11 +225,11 @@ static void nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_n (unsigned char *px, int w, int h, int rs, const unsigned char *spx, int sw, int sh, int srs, - const long *FFd2s, const long *FF_S, unsigned int alpha, int dbits) + const long long *FFd2s, const long *FF_S, unsigned int alpha, int dbits) { int size; unsigned char *d0; - int FFsx0, FFsy0; + long long FFsx0, FFsy0; int x, y;
size = (1 << dbits); @@ -242,7 +243,7 @@
for (y = 0; y < h; y++) { unsigned char *d; - long FFsx, FFsy; + long long FFsx, FFsy; d = d0; FFsx = FFsx0; FFsy = FFsy0; @@ -252,9 +253,9 @@ r = g = b = a = 0; for (i = 0; i < size; i++) { long sx, sy; - sx = (FFsx + FF_S[2 * i]) >> FBITS; + sx = (FFsx >> FBITS_HP) + (FF_S[2 * i] >> FBITS); if ((sx >= 0) && (sx < sw)) { - sy = (FFsy + FF_S[2 * i + 1]) >> FBITS; + sy = (FFsy >> FBITS_HP) + (FF_S[2 * i + 1] >> FBITS); if ((sy >= 0) && (sy < sh)) { const unsigned char *s; unsigned int ca; @@ -302,6 +303,7 @@ { int dbits; long FFd2s[6]; + long long FFd2s_HP[6]; // with higher precision int i;
if (alpha == 0) return; @@ -310,17 +312,18 @@
for (i = 0; i < 6; i++) { FFd2s[i] = (long) (d2s[i] * (1 << FBITS) + 0.5); + FFd2s_HP[i] = (long long) (d2s[i] * (1 << FBITS_HP) + 0.5);; }
if (dbits == 0) { #ifdef WITH_MMX if (NR_PIXOPS_MMX) { /* WARNING: MMX composer REQUIRES w > 0 and h > 0 */ - nr_mmx_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_0 (px, w, h, rs, spx, sw, sh, srs, FFd2s, alpha); + nr_mmx_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_0 (px, w, h, rs, spx, sw, sh, srs, FFd2s_HP, alpha); return; } #endif - nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_0 (px, w, h, rs, spx, sw, sh, srs, FFd2s, alpha); + nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_0 (px, w, h, rs, spx, sw, sh, srs, FFd2s_HP, alpha); } else { int xsize, ysize; long FFs_x_x_S, FFs_x_y_S, FFs_y_x_S, FFs_y_y_S; @@ -347,11 +350,11 @@ #ifdef WITH_MMX if (NR_PIXOPS_MMX) { /* WARNING: MMX composer REQUIRES w > 0 and h > 0 */ - nr_mmx_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_n (px, w, h, rs, spx, sw, sh, srs, FFd2s, FF_S, alpha, dbits); + nr_mmx_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_n (px, w, h, rs, spx, sw, sh, srs, FFd2s_HP, FF_S, alpha, dbits); return; } #endif - nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_n (px, w, h, rs, spx, sw, sh, srs, FFd2s, FF_S, alpha, dbits); + nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_n (px, w, h, rs, spx, sw, sh, srs, FFd2s_HP, FF_S, alpha, dbits); } }
On 9/20/09, Diederik van Lierop <mail@...1689...> wrote:
This one I was able to reproduce too, on a windows XP machine. Attached patch is an improved version of my previous patch and fixes this problem, let's hope that it also fixes the issues with images not being shown.
Can anyone please confirm that this patch is free of the no-display problem the original one had, on the same system?
On Sun, 2009-09-20 at 10:29 -0400, bulia byak wrote:
On 9/20/09, Diederik van Lierop <mail@...1689...> wrote:
This one I was able to reproduce too, on a windows XP machine. Attached patch is an improved version of my previous patch and fixes this problem, let's hope that it also fixes the issues with images not being shown.
Can anyone please confirm that this patch is free of the no-display problem the original one had, on the same system?
Sorry, I still see the same problem after applying the patch: no PNG or JPG displayed with 32 bit Linux.
Tav
Index: /home/diedenrezi/eclipse/InkscapeSVN/src/libnr/nr-compose-transform.cpp =================================================================== --- /home/diedenrezi/eclipse/InkscapeSVN/src/libnr/nr-compose-transform.cpp (revision 22261) +++ /home/diedenrezi/eclipse/InkscapeSVN/src/libnr/nr-compose-transform.cpp (working copy) @@ -40,6 +40,7 @@
/* Fixed point precision */ #define FBITS 12 +#define FBITS_HP 18 // In some places we need a higher precision
void nr_R8G8B8A8_N_EMPTY_R8G8B8A8_N_TRANSFORM (unsigned char *px, int w, int h, int rs, const unsigned char *spx, int sw, int sh, int srs, @@ -168,10 +169,10 @@ static void nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_0 (unsigned char *px, int w, int h, int rs, const unsigned char *spx, int sw, int sh, int srs, - const long *FFd2s, unsigned int alpha) + const long long *FFd2s, unsigned int alpha) { - unsigned char *d0; - int FFsx0, FFsy0; + unsigned char *d0; + long long FFsx0, FFsy0; int x, y;
d0 = px; @@ -180,15 +181,15 @@
for (y = 0; y < h; y++) { unsigned char *d; - long FFsx, FFsy; + long long FFsx, FFsy; d = d0; FFsx = FFsx0; FFsy = FFsy0; for (x = 0; x < w; x++) { long sx, sy; - sx = FFsx >> FBITS; + sx = long(FFsx >> FBITS_HP); if ((sx >= 0) && (sx < sw)) { - sy = FFsy >> FBITS; + sy = long(FFsy >> FBITS_HP); if ((sy >= 0) && (sy < sh)) { const unsigned char *s; unsigned int a; @@ -224,11 +225,11 @@ static void nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_n (unsigned char *px, int w, int h, int rs, const unsigned char *spx, int sw, int sh, int srs, - const long *FFd2s, const long *FF_S, unsigned int alpha, int dbits) + const long long *FFd2s, const long *FF_S, unsigned int alpha, int dbits) { int size; unsigned char *d0; - int FFsx0, FFsy0; + long long FFsx0, FFsy0; int x, y;
size = (1 << dbits); @@ -242,7 +243,7 @@
for (y = 0; y < h; y++) { unsigned char *d; - long FFsx, FFsy; + long long FFsx, FFsy; d = d0; FFsx = FFsx0; FFsy = FFsy0; @@ -252,9 +253,9 @@ r = g = b = a = 0; for (i = 0; i < size; i++) { long sx, sy; - sx = (FFsx + FF_S[2 * i]) >> FBITS; + sx = (FFsx >> FBITS_HP) + (FF_S[2 * i] >> FBITS); if ((sx >= 0) && (sx < sw)) { - sy = (FFsy + FF_S[2 * i + 1]) >> FBITS; + sy = (FFsy >> FBITS_HP) + (FF_S[2 * i + 1] >> FBITS); if ((sy >= 0) && (sy < sh)) { const unsigned char *s; unsigned int ca; @@ -302,6 +303,7 @@ { int dbits; long FFd2s[6]; + long long FFd2s_HP[6]; // with higher precision int i;
if (alpha == 0) return; @@ -310,6 +312,7 @@
for (i = 0; i < 6; i++) { FFd2s[i] = (long) (d2s[i] * (1 << FBITS) + 0.5); + FFd2s_HP[i] = (long long) (d2s[i] * (1 << FBITS_HP) + 0.5);; }
if (dbits == 0) { @@ -320,7 +323,7 @@ return; } #endif - nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_0 (px, w, h, rs, spx, sw, sh, srs, FFd2s, alpha); + nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_0 (px, w, h, rs, spx, sw, sh, srs, FFd2s_HP, alpha); } else { int xsize, ysize; long FFs_x_x_S, FFs_x_y_S, FFs_y_x_S, FFs_y_y_S; @@ -351,7 +354,7 @@ return; } #endif - nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_n (px, w, h, rs, spx, sw, sh, srs, FFd2s, FF_S, alpha, dbits); + nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_n (px, w, h, rs, spx, sw, sh, srs, FFd2s_HP, FF_S, alpha, dbits); } }
The latest patch works fine for me, no shifting and no disappearance. What would others say?
Diederik van Lierop wrote:
... For now I've fixed my patch (see the attachement) by limiting it to the non-MMX c++ code. This fixes the bitmap pixel alignment issue for some users, but not for everyone. There must be someone reading this who can port this to the assembly code too. It really is only a few lines of code! Until then this bug cannot be completely solved, .... unless we start using Cairo (?)
I would recommend timing to see whether the MMX code is (still) really much faster than the normal C++ code. If it's not significantly faster than the old 32bit C++ code I would go with the higher quality code.
As for porting the code to assembly, if it is indeed making use of MMX I have doubts about the feasibility/usefulness of porting the code, as the assembly would likely not be as efficient for 64bit integers as it would be for 32bit integers (assuming the right instructions do exist). But of course this would have to be tested.
On 09/24/2009 08:52 AM, Jasper van de Gronde wrote:
Diederik van Lierop wrote:
... For now I've fixed my patch (see the attachement) by limiting it to the non-MMX c++ code. This fixes the bitmap pixel alignment issue for some users, but not for everyone. There must be someone reading this who can port this to the assembly code too. It really is only a few lines of code! Until then this bug cannot be completely solved, .... unless we start using Cairo (?)
I would recommend timing to see whether the MMX code is (still) really much faster than the normal C++ code. If it's not significantly faster than the old 32bit C++ code I would go with the higher quality code.
As for porting the code to assembly, if it is indeed making use of MMX I have doubts about the feasibility/usefulness of porting the code, as the assembly would likely not be as efficient for 64bit integers as it would be for 32bit integers (assuming the right instructions do exist). But of course this would have to be tested.
Yes, valid points indeed! So I prepared a patch to do some measurements and got some interesting results: when running Fedora 10 i386 virtualized on a Fedora 10 x86_64 host, the non-mmx code actually appears to be faster by 10 - 15%! But before we decide to kick out some of the mmx optimalizations (only the ones that are being tested here), we would need more measurements because my setup might not be representative. So to all the bug reporters who had their images disappearing due to my previous patch: I know you're running with the mmx optimized code ;-). Please test the attached patch against rev. 22287 and report back the measurement data. This is what you should get on the console:
... avg. time spent rendering; with mmx -> 4.10622 msec | without mmx -> 3.5991 msec avg. time spent rendering; with mmx -> 4.10563 msec | without mmx -> 3.59859 msec avg. time spent rendering; with mmx -> 4.10459 msec | without mmx -> 3.59769 msec
Only the last line is relevant.
PS: Don't use this patch for production work, because everything will be rendered twice (with and without mmx optimalisations). This will slow down the rendering. I also tried running the non-mmx optimized code before the other to eliminate any caching effects, but the results look similar
If we can get rid of this specific mmx optimized assembly code, then we can solve the misalignment in rendering the bitmaps for everyone!
Kind regards,
Diederik
Index: src/libnr/nr-compose-transform.cpp =================================================================== --- src/libnr/nr-compose-transform.cpp (revision 22287) +++ src/libnr/nr-compose-transform.cpp (working copy) @@ -345,14 +345,37 @@ }
#ifdef WITH_MMX - if (NR_PIXOPS_MMX) { - /* WARNING: MMX composer REQUIRES w > 0 and h > 0 */ - nr_mmx_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_n (px, w, h, rs, spx, sw, sh, srs, FFd2s, FF_S, alpha, dbits); - return; - } + GTimeVal t1, t2, t3; + static double total_mmx = 0; + static double total_no_mmx = 0; + static double n = 0; + + if (NR_PIXOPS_MMX) { + g_get_current_time(&t1); + + /* WARNING: MMX composer REQUIRES w > 0 and h > 0 */ + nr_mmx_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_n (px, w, h, rs, spx, sw, sh, srs, FFd2s, FF_S, alpha, dbits); + + g_get_current_time(&t2); + } #endif - nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_n (px, w, h, rs, spx, sw, sh, srs, FFd2s, FF_S, alpha, dbits); - } + nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_N_TRANSFORM_n (px, w, h, rs, spx, sw, sh, srs, FFd2s, FF_S, alpha, dbits); +#ifdef WITH_MMX + if (NR_PIXOPS_MMX) { + g_get_current_time(&t3); + double dt12 = ((t2.tv_sec - t1.tv_sec) * G_USEC_PER_SEC + (t2.tv_usec - t1.tv_usec)) / 1000.0; + double dt23 = ((t3.tv_sec - t2.tv_sec) * G_USEC_PER_SEC + (t3.tv_usec - t2.tv_usec)) / 1000.0; + total_mmx += dt12; + total_no_mmx += dt23; + n++; + std::cout << "avg. time spent rendering; with mmx -> " << total_mmx/n << " msec | without mmx -> " << total_no_mmx/n << " msec" << std::endl; + } else { + std::cout << "sorry, no NR_PIXOPS_MMX" << std::endl; + } +#else + std::cout << "sorry, no WITH_MMX" << std::endl; +#endif + } }
void nr_R8G8B8A8_P_R8G8B8A8_P_R8G8B8A8_P_TRANSFORM (unsigned char *px, int w, int h, int rs,
On 09/26/2009 12:16 AM, Diederik van Lierop wrote:
Yes, valid points indeed! So I prepared a patch to do some measurements and got some interesting results: when running Fedora 10 i386 virtualized on a Fedora 10 x86_64 host, the non-mmx code actually appears to be faster by 10 - 15%! But before we decide to kick out some of the mmx optimalizations (only the ones that are being tested here), we would need more measurements because my setup might not be representative. So to all the bug reporters who had their images disappearing due to my previous patch: I know you're running with the mmx optimized code ;-). Please test the attached patch against rev. 22287 and report back the measurement data. This is what you should get on the console:
... avg. time spent rendering; with mmx -> 4.10622 msec | without mmx -> 3.5991 msec avg. time spent rendering; with mmx -> 4.10563 msec | without mmx -> 3.59859 msec avg. time spent rendering; with mmx -> 4.10459 msec | without mmx -> 3.59769 msec
Only the last line is relevant.
PS: Don't use this patch for production work, because everything will be rendered twice (with and without mmx optimalisations). This will slow down the rendering. I also tried running the non-mmx optimized code before the other to eliminate any caching effects, but the results look similar
If we can get rid of this specific mmx optimized assembly code, then we can solve the misalignment in rendering the bitmaps for everyone!
The measurements which I reported before were on a virtual machine, but now I can confirm the negative impact of the mmx optimized code on a native Fedora 9 installation too!
My previous mail got sent before I finished typing ...
On 09/26/2009 12:16 AM, Diederik van Lierop wrote:
Yes, valid points indeed! So I prepared a patch to do some measurements and got some interesting results: when running Fedora 10 i386 virtualized on a Fedora 10 x86_64 host, the non-mmx code actually appears to be faster by 10 - 15%! But before we decide to kick out some of the mmx optimalizations (only the ones that are being tested here), we would need more measurements because my setup might not be representative. So to all the bug reporters who had their images disappearing due to my previous patch: I know you're running with the mmx optimized code ;-). Please test the attached patch against rev. 22287 and report back the measurement data. This is what you should get on the console:
... avg. time spent rendering; with mmx -> 4.10622 msec | without mmx -> 3.5991 msec avg. time spent rendering; with mmx -> 4.10563 msec | without mmx -> 3.59859 msec avg. time spent rendering; with mmx -> 4.10459 msec | without mmx -> 3.59769 msec
Only the last line is relevant.
PS: Don't use this patch for production work, because everything will be rendered twice (with and without mmx optimalisations). This will slow down the rendering. I also tried running the non-mmx optimized code before the other to eliminate any caching effects, but the results look similar
If we can get rid of this specific mmx optimized assembly code, then we can solve the misalignment in rendering the bitmaps for everyone!
The measurements which I reported before were on a virtual machine, but now I can confirm the negative impact of the mmx optimized code on a native Fedora 9 installation too!
These are the results which I obtained using an old laptop (it took about three hours to compile Inkscape on that laptop :-( )
avg. time spent rendering; with mmx -> 10.1511 msec | without mmx -> 7.2687 msec
Again the mmx-optimized assembly code actually appears to be slower!
So if no one comes up with evidence showing opposite results then I will remove these specific optimizations (but not before our v0.47 obviously). This will allow for fixing the pixel misalignment issue, besides making Inkscape more responsive for some users.
Diederik
On 20/9/09 16:29, bulia byak wrote:
On 9/20/09, Diederik van Lierop <mail@...1689...> wrote:
This one I was able to reproduce too, on a windows XP machine. Attached patch is an improved version of my previous patch and fixes this problem, let's hope that it also fixes the issues with images not being shown.
Can anyone please confirm that this patch is free of the no-display problem the original one had, on the same system?
No problem on 32bit OS X 10.5.8 (which apparently has some 64bit support), but had no problem with original patch either.
~suv
De : bulia byak <buliabyak@...400...>
On 9/20/09, Diederik van Lierop wrote:
This one I was able to reproduce too, on a windows XP machine. Attached patch is an improved version of my previous patch and fixes this problem, let's hope that it also fixes the issues with images not being shown.
Can anyone please confirm that this patch is free of the no-display problem the original one had, on the same system?
Same problem with this patch on Ubuntu 9.04, 32bits. No bitmap displayed in the previews (including OCAL) and on the canvas. -- Nicolas
participants (6)
-
bulia byak -
Diederik van Lierop -
Jasper van de Gronde -
Nicolas Dufour -
Tavmjong Bah -
~suv