Re: [TCLCORE] TIP 714: image driver photo formats

[Emiliano <emi...@gm...>]

On Thu, 3 Apr 2025 10:41:53 +0200
Harald Oehlmann <har...@el...> wrote:

IMO this is the opportunity to fix some issues wrt the image type registration
and its implementation.

The low hanging fruit here is the current registration. It is implemented as
a single linked list, and the current implementation simply adds the newly
registered type at the front of the list unconditionally. What happens if you
add a new type with an already existing name? Let the code speak:

  $ cat badimage.c
  #include <tk.h>
  static const Tk_ImageType badType = {
      "bitmap", NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL
  };
  int Badimage_Init(Tcl_Interp *interp) {
      Tcl_InitStubs(interp, TCL_VERSION, 0);
      Tk_InitStubs(interp, TK_VERSION, 0);
      Tk_CreateImageType(&badType);
      return TCL_OK;
  }

  $ gcc -DUSE_TK_STUBS -DUSE_TCL_STUBS -shared -o libbadimage.so badimage.c -L/usr/local/lib -ltclstub -L/usr/local/lib -ltkstub

  $ wish9.1
  % image types
  photo bitmap 
  % load ./libbadimage.so
  % image types
  bitmap photo bitmap

In this case, the list is walked from the start adding the .name member of
the Tk_ImageInfo structure until the .nextPtr member is null. Same with
[image create $type]: the list is walked until $type matches the .name
member. The result is

* [image types] always grows, and can get duplicates.
* the first (last added) of the duplicated type names shadows the later ones.

This last fact is easy to test

  % image create bitmap
  Violación de segmento (`core' generado)

I have a fix for the duplicated names issue. Since TIP#714 implements a
hash table to hold the registered Tk_ImageInfoProc, I've changed the
linked list for a(n internal) wrapper struct to hold both Tk_ImageType and
Tk_ImageInfoProc, and using the hash table to hold these wrappers.
If there are no questions, I'm going to commit it in a couple days in the
tip-714-alt branch, so it can be reviewed.

Using a hash table preserve the behaviour that newly registered types with
an existing name overwrites the old one. Also, I think this should
be documented.

Another issue identified has to do with the fact that image type managers
are implemented at thread level; newly registered image types are visible
in all interpreters in the same thread. Also, there's no provision to remove
an image type. These gotchas makes image type managers not well suited
to be implemented as extensions, since extensions are limited to
the interpreter which loads them (is this assumption true?). Interactively:

  # loading in a child interp, visible from parent interp
  $ wish9.1
  % interp create i
  i
  % i eval {load ./libbadimage.so}
  % i eval {image types}
  bitmap photo bitmap
  % image types
  bitmap photo bitmap

But this is subject for another TIP. Also, the photo image type format list
is also thread data; [package require ::img::jpeg] makes Tk photo image
supports jpeg in all the interpreters in the thread in which Tk is loaded.

> It now features the great intuitive result:
> 
> % image types photo
> format {svg ppm png gif default} file {svg ppm png gif} write {ppm png gif}

I think it can be a bit more descriptive. What's the difference from file,
format and write here? Also, I think should be documented too.

-- 
Emiliano <emi...@gm...>