Tk Source Code

Copied from comp.lang.tcl
Author: [email protected]

I put together an example of c code that demonstrates the problem.

Compile the c code below to bugdemo.[info shared], then do something along the lines of:

% load ${path}/bugdemo.so
% set imgN [image create photo]
% set imgU [image create photo]
% canvas .c -bg blue
% grid .c
% .c create image 10 10 -image $imgN
% .c create image 30 10 -image $imgU
% ::bugDemo::normalData $imgN
% ::bugDemo::unusualData $imgU


/*
 * bugdemo.c --
 * Demo of problem with alpha channel and unusal image data format.
 * The intent is to create a 10x10 image with all pixels orange (#ff7f00)
 * and a 6x6 area in the center that is semi-transparent.
 * We do this in two ways: once with the "normal" data format
 * and once with the RGBA channels on separate planes.
 */

#include <tcl.h>
#include <tk.h>


unsigned char normImg[400] = {
    255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,
    255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,

    255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,
    255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,

    255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 100,
    255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 255,  255, 127, 0, 255,

    255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 100,
    255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 255,  255, 127, 0, 255,

    255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 100,
    255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 255,  255, 127, 0, 255,

    255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 100,
    255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 255,  255, 127, 0, 255,

    255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 100,
    255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 255,  255, 127, 0, 255,

    255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 100,
    255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 100,  255, 127, 0, 255,  255, 127, 0, 255,

    255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,
    255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,

    255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,
    255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,  255, 127, 0, 255,


};


unsigned char unusualImg[400] = {
    255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
    255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
    255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
    255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
    255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
    255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
    255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
    255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
    255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
    255, 255, 255, 255, 255, 255, 255, 255, 255, 255,

    127, 127, 127, 127, 127, 127, 127, 127, 127, 127,
    127, 127, 127, 127, 127, 127, 127, 127, 127, 127,
    127, 127, 127, 127, 127, 127, 127, 127, 127, 127,
    127, 127, 127, 127, 127, 127, 127, 127, 127, 127,
    127, 127, 127, 127, 127, 127, 127, 127, 127, 127,
    127, 127, 127, 127, 127, 127, 127, 127, 127, 127,
    127, 127, 127, 127, 127, 127, 127, 127, 127, 127,
    127, 127, 127, 127, 127, 127, 127, 127, 127, 127,
    127, 127, 127, 127, 127, 127, 127, 127, 127, 127,
    127, 127, 127, 127, 127, 127, 127, 127, 127, 127,

    0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
    0, 0, 0, 0, 0, 0, 0, 0, 0, 0,

    255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
    255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
    255, 255, 100, 100, 100, 100, 100, 100, 255, 255,
    255, 255, 100, 100, 100, 100, 100, 100, 255, 255,
    255, 255, 100, 100, 100, 100, 100, 100, 255, 255,
    255, 255, 100, 100, 100, 100, 100, 100, 255, 255,
    255, 255, 100, 100, 100, 100, 100, 100, 255, 255,
    255, 255, 100, 100, 100, 100, 100, 100, 255, 255,
    255, 255, 255, 255, 255, 255, 255, 255, 255, 255,
    255, 255, 255, 255, 255, 255, 255, 255, 255, 255
};


static int normalData (clientData, interp, objc, objv)
ClientData clientData;
Tcl_Interp *interp;
int objc;
Tcl_Obj *const objv[];
{
    Tk_PhotoHandle img;
    Tk_PhotoImageBlock imgData;

    if(objc != 2) {
        Tcl_WrongNumArgs(interp, 1, objv, "image");
        return TCL_ERROR;
    }
    if ((img = Tk_FindPhoto(interp, Tcl_GetString(objv[1]))) == NULL) {
        Tcl_SetObjResult(interp, Tcl_ObjPrintf("could not find image: %s", Tcl_GetString(objv[1])));
        return TCL_ERROR;
    }

    imgData.width = 10;
    imgData.height = 10;
    imgData.pitch = 40;
    imgData.pixelSize = 4;
    imgData.offset[0] = 0;
    imgData.offset[1] = 1;
    imgData.offset[2] = 2;
    imgData.offset[3] = 3;
    imgData.pixelPtr = normImg;

    Tk_PhotoPutBlock(interp, img, &imgData, 0, 0, 10, 10, TK_PHOTO_COMPOSITE_SET);

    return TCL_OK;
}

static int unusualData (clientData, interp, objc, objv)
ClientData clientData;
Tcl_Interp *interp;
int objc;
Tcl_Obj *const objv[];
{
    Tk_PhotoHandle img;
    Tk_PhotoImageBlock imgData;

    if(objc != 2) {
        Tcl_WrongNumArgs(interp, 1, objv, "image");
        return TCL_ERROR;
    }
    if ((img = Tk_FindPhoto(interp, Tcl_GetString(objv[1]))) == NULL) {
        Tcl_SetObjResult(interp, Tcl_ObjPrintf("could not find image: %s", Tcl_GetString(objv[1])));
        return TCL_ERROR;
    }

    imgData.width = 10;
    imgData.height = 10;
    imgData.pitch = 10;
    imgData.pixelSize = 1;
    imgData.offset[0] = 0;
    imgData.offset[1] = 100;
    imgData.offset[2] = 200;
    imgData.offset[3] = 300;
    imgData.pixelPtr = unusualImg;

    Tk_PhotoPutBlock(interp, img, &imgData, 0, 0, 10, 10, TK_PHOTO_COMPOSITE_SET);

    return TCL_OK;
}


int Bugdemo_Init (interp)
Tcl_Interp *interp;
{
    if(Tcl_InitStubs(interp, TCL_VERSION, 0) == NULL) {
        return TCL_ERROR;
    }
    if(Tk_InitStubs(interp, TK_VERSION, 0) == NULL) {
        return TCL_ERROR;
    }

    if(Tcl_PkgRequire(interp, "Tk", TK_VERSION, 0) == NULL) {
        return TCL_ERROR;
    }

    Tcl_CreateNamespace(interp, "::bugDemo", NULL, NULL);
    Tcl_CreateObjCommand(interp, "::bugDemo::normalData", normalData, NULL, NULL);
    Tcl_CreateObjCommand(interp, "::bugDemo::unusualData", unusualData, NULL, NULL);

    return TCL_OK;
}

I personally judge the danger to break present code as quite high.

I would propose to change the description to tell, that the size field is used to determine the presence of an alpha channel.

The current presentation of the issue within the bug section is IMHO not optimal.

Changed the docs to describe the issue: [674e00a5] and [0020d703].

The "real fix" might introduce compatibility problems and can be considered a new feature (see previous comments). For this reason, I changed the ticket to a RFE.

I really like chw's idea. It would give us the flexibility in data layout the docs promise - and the current code fails to provide.

I agree that it's wrong to use the pixelSize field to decide how many channels the image has. 'pixelSize' should be treated like the 'pitch' filed: as an address increment and nothing else. Using the offset array to discriminate the different gray/RGB/alpha cases looks like The Right Thing to me.

chw's proposal might break existing code, though. Consider:

  imgBlock.pixelSize = 3;
  imgBlock.offset[0] = 0;
  imgBlock.offset[1] = 1;
  imgBlock.offset[2] = 2;
  imgBlock.offset[3] = 3;

As an immediate fix, I'd propose to change the docs to describe how things currently are: [9ba7d7f8].

Long term, I think chw's idea is the way to go.

No idea if this is a good one but shouldn't we try to discriminate these four cases from the offset array:

1. gray image w/o alpha: all offsets are equal
2. gray image with alpha: all RGB offsets are equal, alpha offset is different
3. RGB image w/o alpha: all RGB offsets are different, alpha offset is equal to one of R or G or B
4. RGB image with alpha: all offsets are different

The pixelSize is the address increment between pixels within a (horizontal) line but does not account for gray vs. RGB plus/minus alpha consideration since that is the purpose of the offset array. The pitch is the address increment between lines of pixels.

I do see this as a bug: the documentation clearly says that such a format should be supported: "The pixelSize field specifies the address difference between two horizontally adjacent pixels. Often it is 3 or 4, but it can have any value. [...] The offset array contains the offsets from the address of a pixel to the addresses of the bytes containing the red, green, blue and alpha (transparency) components. These are normally 0, 1, 2 and 3, but can have other values, e.g., for images that are stored as separate red, green and blue planes."

It might be "just" a documentation bug, but still a bug. Personally, I don't really need this feature, but I like it when documentation describes things how they are and not how they were intended to be ;-).

As for changing the handling of the alphaOffset, I DO se a danger: chances are, that there is code out there, that relies on this (mis)feature. i.e. code that does not provide any alpha data at all, if alphaOffset is larger than pixelSize. Such code will result in a segmentation fault if a future version will try to copy the data for the alpha channel when there is no such data.

IMO, the simplest (only?) thing to do is to modify the doc to describe the problem and leave things as they are - at least until 9.0.

Actually, this would be a RFE in stead of a bug: this format has never been supported despite it not being decoumented as such. However, it would be easy to start supporting this format. See [776371f4a27c97cd], this no longer handles the alphaOffset to be invalid when it is bigger than the pixelsize. I see no danger in changing that.