Allegro.cc - Online Community

Allegro.cc Forums » Allegro Development » Stretch blit bug?

This thread is locked; no one can reply to it. rss feed Print
Stretch blit bug?
Torbjörn Olsson
Member #6,792
January 2006

The stretch blit in cstretch.c behaves differently compared with the one in istretch.c.

You can see the difference in the files I have attached.

Basically what I want to to is to enlarge a bitmap 10 times. In the istretch.c version each pixel becomes a 10x10 area, but using cstretch.c the pixels around the borders are not expanded as much as those in the middle.

If I could I would use istretch.c, but I am not developing for x86, so I can not.

My questions:

Is this a known bug?
Will there be/is there a solution?

Best regards
Torbjörn Olsson

UPDATE:

Ok, it seems to me that cstretch.c is different by design. I changed this code to make it work like I want it to:

(Note the code with the HACK-WARNING comment)

/*
* Stretch blit work-horse.
*/
static void _al_stretch_blit(BITMAP *src, BITMAP *dst,
int sx, int sy, int sw, int sh,
int dx, int dy, int dw, int dh,
int masked)
{
int x, y, fixup;
int i1, i2, dd;
int xinc, yinc;
int dxbeg, dxend;
int dybeg, dyend;
int sxofs, dxofs;
void (*stretch_line)(unsigned long dptr, unsigned char *sptr);

ASSERT(src);
ASSERT(dst);

if ((sw <= 0) || (sh <= 0) || (dw <= 0) || (dh <= 0))
return;

if (dst->clip) {
dybeg = ((dy > dst->ct) ? dy : dst->ct);
dyend = (((dy + dh) < dst->cb) ? (dy + dh) : dst->cb);
if (dybeg >= dyend)
return;

dxbeg = ((dx > dst->cl) ? dx : dst->cl);
dxend = (((dx + dw) < dst->cr) ? (dx + dw) : dst->cr);
if (dxbeg >= dxend)
return;
}
else {
dxbeg = dx;
dxend = dx + dw;
dybeg = dy;
dyend = dy + dh;
}

/* Bresenham algorithm uses difference between points, not number of points. */
sw--;
sh--;
dw--;
dh--;

/* How much to add to src-coordinates, when moving to the next dst-coordinate. */
if (dw == 0)
xinc = 0;
else {
xinc = sw / dw;
sw %= dw;
}

if (dh == 0)
yinc = 0;
else {
yinc = sh / dh;
sh %= dh;
}

/* Walk in x direction until dxbeg and save Bresenham state there. */
i2 = (dd = (i1 = 2 * sw) - dw) - dw;

if(1)//HACK-WARNING
{
i1 = 2 * sw + 2;
dd = 2 * sw - 2 * dw;
i2 = dd;
}

for (x = dx, y = sx; x < dxbeg; x++, y += xinc) {
if (dd >= 0)
y++, dd += i2;
else
dd += i1;
}

/* Save Bresenham algorithm state with offset fixups. */
switch (bitmap_color_depth(dst)) {
case 15:
case 16:
fixup = sizeof(short);
break;
case 24:
fixup = 3;
break;
case 32:
fixup = sizeof(long);
break;
default:
fixup = 1;
break;
}
sxofs = y * fixup;
dxofs = x * fixup;
_al_stretch.i1 = i1;
_al_stretch.i2 = i2;
_al_stretch.dd = dd;
_al_stretch.dw = (dxend - dxbeg) * fixup;
_al_stretch.sinc = xinc * fixup;

/* Find out which stretcher should be used. */
if (masked) {
switch (bitmap_color_depth(dst)) {
#ifdef ALLEGRO_COLOR8
case 8:
if (is_linear_bitmap(dst))
stretch_line = stretch_masked_line8;
else {
#ifdef GFX_MODEX
stretch_line = stretch_masked_linex;
#else
return;
#endif
}
break;
#endif
#ifdef ALLEGRO_COLOR16
case 15:
stretch_line = stretch_masked_line15;
break;
case 16:
stretch_line = stretch_masked_line16;
break;
#endif
#ifdef ALLEGRO_COLOR24
case 24:
stretch_line = stretch_masked_line24;
break;
#endif
#ifdef ALLEGRO_COLOR32
case 32:
stretch_line = stretch_masked_line32;
break;
#endif
default:
return;
}
}
else {
switch (bitmap_color_depth(dst)) {
#ifdef ALLEGRO_COLOR8
case 8:
if (is_linear_bitmap(dst))
stretch_line = stretch_line8;
else {
#ifdef GFX_MODEX
stretch_line = stretch_linex;
#else
return;
#endif
}
break;
#endif
#ifdef ALLEGRO_COLOR16
case 15:
stretch_line = stretch_line15;
break;
case 16:
stretch_line = stretch_line16;
break;
#endif
#ifdef ALLEGRO_COLOR24
case 24:
stretch_line = stretch_line24;
break;
#endif
#ifdef ALLEGRO_COLOR32
case 32:
stretch_line = stretch_line32;
break;
#endif
default:
return;
}
}

ASSERT(stretch_line);

/* Walk in y direction until we reach first non-clipped line. */
i2 = (dd = (i1 = 2 * sh) - dh) - dh;

if(1)//HACK-WARNING
{
i1 = 2 * sh + 2;
dd = 2 * sh - 2 * dh;
i2 = dd;
}

for (x = dy, y = sy; x < dybeg; x++, y += yinc) {
if (dd >= 0)
y++, dd += i2;
else
dd += i1;
}

/* Stretch all non-clipped lines. */
bmp_select(dst);
for (; x < dyend; x++, y += yinc) {
(*stretch_line)(bmp_write_line(dst, x) + dxofs, src->line[y] + sxofs);
if (dd >= 0)
y++, dd += i2;
else
dd += i1;
}
bmp_unwrite_line(dst);
}

Now it seems to work like I want it to, but can you see any problems with my changes?

A J
Member #3,025
December 2002
avatar

which looks better ?
does cstrech make a nicer looking image?

___________________________
The more you talk, the more AJ is right. - ML

Michael Faerber
Member #4,800
July 2004
avatar

I'm not experienced with the Allegro internals code and therefore won't comment on the code itself, but if you post code, you should use the [ code] and [ /code] tags. See Mockup information.

1/*
2* Stretch blit work-horse.
3*/
4static void _al_stretch_blit(BITMAP *src, BITMAP *dst,
5int sx, int sy, int sw, int sh,
6int dx, int dy, int dw, int dh,
7int masked)
8{
9int x, y, fixup;
10int i1, i2, dd;
11int xinc, yinc;
12int dxbeg, dxend;
13int dybeg, dyend;
14int sxofs, dxofs;
15void (*stretch_line)(unsigned long dptr, unsigned char *sptr);
16 
17ASSERT(src);
18ASSERT(dst);
19 
20if ((sw <= 0) || (sh <= 0) || (dw <= 0) || (dh <= 0))
21return;
22 
23if (dst->clip) {
24dybeg = ((dy > dst->ct) ? dy : dst->ct);
25dyend = (((dy + dh) < dst->cb) ? (dy + dh) : dst->cb);
26if (dybeg >= dyend)
27return;
28 
29dxbeg = ((dx > dst->cl) ? dx : dst->cl);
30dxend = (((dx + dw) < dst->cr) ? (dx + dw) : dst->cr);
31if (dxbeg >= dxend)
32return;
33}
34else {
35dxbeg = dx;
36dxend = dx + dw;
37dybeg = dy;
38dyend = dy + dh;
39}
40 
41/* Bresenham algorithm uses difference between points, not number of points. */
42sw--;
43sh--;
44dw--;
45dh--;
46 
47/* How much to add to src-coordinates, when moving to the next dst-coordinate. */
48if (dw == 0)
49xinc = 0;
50else {
51xinc = sw / dw;
52sw %= dw;
53}
54 
55if (dh == 0)
56yinc = 0;
57else {
58yinc = sh / dh;
59sh %= dh;
60}
61 
62/* Walk in x direction until dxbeg and save Bresenham state there. */
63i2 = (dd = (i1 = 2 * sw) - dw) - dw;
64 
65if(1)//HACK-WARNING
66{
67i1 = 2 * sw + 2;
68dd = 2 * sw - 2 * dw;
69i2 = dd;
70}
71 
72for (x = dx, y = sx; x < dxbeg; x++, y += xinc) {
73if (dd >= 0)
74y++, dd += i2;
75else
76dd += i1;
77}
78 
79/* Save Bresenham algorithm state with offset fixups. */
80switch (bitmap_color_depth(dst)) {
81case 15:
82case 16:
83fixup = sizeof(short);
84break;
85case 24:
86fixup = 3;
87break;
88case 32:
89fixup = sizeof(long);
90break;
91default:
92fixup = 1;
93break;
94}
95sxofs = y * fixup;
96dxofs = x * fixup;
97_al_stretch.i1 = i1;
98_al_stretch.i2 = i2;
99_al_stretch.dd = dd;
100_al_stretch.dw = (dxend - dxbeg) * fixup;
101_al_stretch.sinc = xinc * fixup;
102 
103/* Find out which stretcher should be used. */
104if (masked) {
105switch (bitmap_color_depth(dst)) {
106#ifdef ALLEGRO_COLOR8
107case 8:
108if (is_linear_bitmap(dst))
109stretch_line = stretch_masked_line8;
110else {
111#ifdef GFX_MODEX
112stretch_line = stretch_masked_linex;
113#else
114return;
115#endif
116}
117break;
118#endif
119#ifdef ALLEGRO_COLOR16
120case 15:
121stretch_line = stretch_masked_line15;
122break;
123case 16:
124stretch_line = stretch_masked_line16;
125break;
126#endif
127#ifdef ALLEGRO_COLOR24
128case 24:
129stretch_line = stretch_masked_line24;
130break;
131#endif
132#ifdef ALLEGRO_COLOR32
133case 32:
134stretch_line = stretch_masked_line32;
135break;
136#endif
137default:
138return;
139}
140}
141else {
142switch (bitmap_color_depth(dst)) {
143#ifdef ALLEGRO_COLOR8
144case 8:
145if (is_linear_bitmap(dst))
146stretch_line = stretch_line8;
147else {
148#ifdef GFX_MODEX
149stretch_line = stretch_linex;
150#else
151return;
152#endif
153}
154break;
155#endif
156#ifdef ALLEGRO_COLOR16
157case 15:
158stretch_line = stretch_line15;
159break;
160case 16:
161stretch_line = stretch_line16;
162break;
163#endif
164#ifdef ALLEGRO_COLOR24
165case 24:
166stretch_line = stretch_line24;
167break;
168#endif
169#ifdef ALLEGRO_COLOR32
170case 32:
171stretch_line = stretch_line32;
172break;
173#endif
174default:
175return;
176}
177}
178 
179ASSERT(stretch_line);
180 
181/* Walk in y direction until we reach first non-clipped line. */
182i2 = (dd = (i1 = 2 * sh) - dh) - dh;
183 
184if(1)//HACK-WARNING
185{
186i1 = 2 * sh + 2;
187dd = 2 * sh - 2 * dh;
188i2 = dd;
189}
190 
191for (x = dy, y = sy; x < dybeg; x++, y += yinc) {
192if (dd >= 0)
193y++, dd += i2;
194else
195dd += i1;
196}
197 
198/* Stretch all non-clipped lines. */
199bmp_select(dst);
200for (; x < dyend; x++, y += yinc) {
201(*stretch_line)(bmp_write_line(dst, x) + dxofs, src->line[y] + sxofs);
202if (dd >= 0)
203y++, dd += i2;
204else
205dd += i1;
206}
207bmp_unwrite_line(dst);
208}

--
"The basic of informatics is Microsoft Office." - An informatics teacher in our school
"Do you know Linux?" "Linux? Isn't that something for visually impaired people?"

Elias
Member #358
May 2000

You used code tags now, but still killed the indentation :P

Anyway, it seems to be a bug. Looking at the changelogs, the functional part of the code in cstretch.c hasn't been touched since 6 years (that's how far SVN logs go back..), so the problem must be the initial implementation of the bresenham scaler.

[Edit: http://websvn.bafserv.com/wsvn/allegro/allegro/branches/4.2/src/c/cstretch.c?op=blame&rev=0&sc=0]

Anyone up to fixing it?

I can't notice any difference when adding your if(1) {} part.. probably I missed something there.

--
"Either help out or stop whining" - Evert

Thomas Harte
Member #33
April 2000
avatar

Quote:

Anyone up to fixing it?

Any chance of just completely replacing _al_stretch_blit? Besides being hard to follow - and Allegro's doesn't even seem to be the common form - Bresenham trades divides outside the inner loop for conditionals within it. So on modern CPUs it isn't a smart move, especially since average BITMAP sizes are increasing over time.

I'll have a quick hack at a more conventional _al_stretch_blit, if everyone else is busy...

Elias
Member #358
May 2000

Sure. As I understand it, the asm code will be dropped more sooner than later anyway, so no need to try to match the cstrech.c version with the istretch.c one.

But maybe you shold wait for Peter, he apparently fixed something in istretch.c a while back:

Quote:

r3020 | tjaden | 2003-02-15 02:29:39 +0100 (Sat, 15 Feb 2003) | 4 lines

Compensate for a problem in stretch_bitmap where the first column or row can
get an extra pixel than it should do for little -> big scaling. Fixed by me
and Eric. Bug reported by David Gowers and AJ.

The change is marked as 3020 here: http://websvn.bafserv.com/wsvn/allegro/allegro/branches/4.2/src/i386/istretch.c?op=blame&rev=0&sc=0

And I assume, they use sort of the same algorithm, just implemented a bit differently, so someone understanding the code might be able to fix it easily..

--
"Either help out or stop whining" - Evert

Torbjörn Olsson
Member #6,792
January 2006

Thanks for your replies.

AJ: The istretch version is preferable from my point of view.

Michael Faerber: Thanks for the tip.

Elias (first post): Did you look at the scaling around the borders of the bitmap?

Thomas Harte: Great, please let me know how it goes.

Elias (second post): Do you mean istretch will be dropped? Then I guess its time to fix cstretch soon.

Best regards
Torbjörn Olsson

Evert
Member #794
November 2000
avatar

From a code maintenance point of view, having both a C version of the code and an ASM version is a bad idea - especially if we want to rewrite things anyway.

As for bugs in the C version of the code, I've never really noticed any but then again, I hardly use scaling anyway.

Elias
Member #358
May 2000

Quote:

Elias (first post): Did you look at the scaling around the borders of the bitmap?

Yes, it looked wrong both with and without that if(1) block for me. (Couldn't notice any difference at all.. but I didn't check for possible compilation problems, so maybe I looked twice at the same version?)

--
"Either help out or stop whining" - Evert

Torbjörn Olsson
Member #6,792
January 2006

Evert: I agree 100%

Elias: There are two if(1)-blocks (one for vertical and one for horisontal), did you use both of them?

Elias
Member #358
May 2000

Ah, yes, I had only seen one, and put at the wrong place :P From the example it looks like it fixes the problem.

So - since you commented it as "HACK" - is this a proper fix or not? If yes, maybe you could rewrite it without the if(1) and make a patch.

Thomas: In that case, it wouldn't be necessary to rewrite it - but if you create an improved and better understandable bitmap scaler, we would of course use that.

--
"Either help out or stop whining" - Evert

Torbjörn Olsson
Member #6,792
January 2006

Since I don't completely understand the code, I can not be sure that it works in all cases.

I have tested it a bit, but only for very simple test cases.

But even if my solution does work, it is probably not the best possible, so if someone with more experience with the code could take a look at it, and maybe even be able to optimize it a bit, that would be great.

Go to: