Markus Wick <[email protected]> writes:

> Am 2014-03-19 06:09, schrieb Keith Packard:
>> +        glamor_pixmap_loop(src_priv, src_box_x, src_box_y) {
>> +            glamor_pixmap_loop(dst_priv, dst_box_x, dst_box_y) {
>> +                for (b = 0; b < nbox; b++) {
>
> Are you sure that this loop order is safe?
> Eg what happens, when we copy the first box by box_size/2?
> I think we will overwrite the second part of the first box at first and 
> then try to copy the overwritten content into the second box.
>
> To fix this issue, we have to loop over the boxes in opposition to the 
> copy order. :/

> What will happen if we have two boxes defined and the copy of the first 
> box will overlapp the second box? Shall we copy the new or the old 
> content of the second box? If it's defined to be the new content, we 
> also have to move the box loop to the top.

Good catch! Fortunately, the 'upsidedown' and 'reverse' parameters tell
us which order to use, so the loops turn into this rather larger bit of code:

        int src_x_start, src_x_end, src_x_step;
        int src_y_start, src_y_end, src_y_step;
        int dst_x_start, dst_x_end, dst_x_step;
        int dst_y_start, dst_y_end, dst_y_step;

        if (reverse) {
            src_x_start = glamor_pixmap_wcnt(src_priv) - 1;
            src_x_end = -1;
            src_x_step = -1;

            dst_x_start = glamor_pixmap_wcnt(dst_priv) - 1;
            dst_x_end = -1;
            dst_x_step = -1;
        } else {
            src_x_start = 0;
            src_x_end = glamor_pixmap_wcnt(src_priv);
            src_x_step = 1;

            dst_x_start = 0;
            dst_x_end = glamor_pixmap_wcnt(dst_priv);
            dst_x_step = 1;
        }

        if (upsidedown) {
            src_y_start = glamor_pixmap_hcnt(src_priv) - 1;
            src_y_end = -1;
            src_y_step = -1;

            dst_y_start = glamor_pixmap_hcnt(dst_priv) - 1;
            dst_y_end = -1;
            dst_y_step = -1;
        } else {
            src_y_start = 0;
            src_y_end = glamor_pixmap_hcnt(src_priv);
            src_y_step = 1;

            dst_y_start = 0;
            dst_y_end = glamor_pixmap_hcnt(dst_priv);
            dst_y_step = 1;
        }

        glMatrixMode(GL_PROJECTION);
        glLoadIdentity();

        for (b = 0; b < nbox; b++) {
            for (src_box_y = src_y_start; src_box_y != src_y_end; src_box_y += 
src_y_step)
                for (src_box_x = src_x_start; src_box_x != src_x_end; src_box_x 
+= src_x_step) {
                    BoxPtr src_box = glamor_pixmap_box_at(src_priv, src_box_x, 
src_box_y);
                    glBindFramebuffer(GL_READ_FRAMEBUFFER, 
glamor_pixmap_fbo_at(src_priv, src_box_x, src_box_y)->fb);

                    for (dst_box_y = dst_y_start; dst_box_y != dst_y_end; 
dst_box_y += dst_y_step)
                        for (dst_box_x = dst_x_start; dst_box_x != dst_x_end; 
dst_box_x += dst_x_step) {
                            int     dx1, dx2, dy1, dy2;

This is pretty easy to verify as well. Enable the #define MAX_FBO_SIZE
32 line in glamor_priv.h and run x11perf with -depth 32 to force an
off-screen pixmap to be allocated:

$ x11perf -copywinwin500 -depth 32 -sync -reps 20

-- 
[email protected]

Attachment: pgp0Lftn45l9s.pgp
Description: PGP signature

_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Reply via email to