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

[Harald O. <har...@el...>]

Dear Emiliano,

thanks for the message. Please allow me to reply inline.

Am 09.04.2025 um 02:25 schrieb Emiliano:
> 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.

Yes, great !

> 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.

Thank you for working on this. IMHO, this is a bug of the current 
implementation and is not TIP 714 related.
Could you open a bug ticket and provide the fix there?
I prefer to have the discussion archived somewhere and a ticket is a 
great place.

I think, even with the current data structure, it would be possible to 
avoid double registrations, but the list must be scanned for the name 
entry. Currently, Tk_CreateImageType can not return an error on double 
registration.

> 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.

I think, the thread issue is also a bug and may be handled in a ticket.
Tk is slowly getting thread safe and this is another step in this direction.

> 
>> 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.

The current description is in TIP 714. Is this ok for you?

I like the "image types photo" command. It is intuitive.

I am not happy with the registration of an additional function pointer 
by "Tk_ImageInfoProc". IMHO, this has the same purpose as 
"Tk_CreateImageType", to pass a C function pointer for an image type 
handler. I would prefer to extend the passed data structure.

I am planning to copy the great "image type photo" code to the 
"tip-714-image-driver-info" branch. Is this a good idea?

There are other tasks in front of us:
A procedure to unregister an image type handler and cleaning up all its 
resources. One structural possibility would be to pass a pointer to a 
delete procedure with Tk_ImageInfoProc. Also, a TSD data pointer may be 
passed for its own thread memory.
The unregister functionality will require an unregister procedure for 
photo format handlers. So, this is a bigger bunch.

But all starts with the list of function pointers passed with 
"Tk_CreateImageType". Perhaps, a new "Tk_CreateImageType" may pass a 
memory with a version field, so, we may later extend it.

Any comment welcome!
Harald