What do you need gimp_file_delete_thumbnail() for?

You shouldn't need gimp_file_has_valid_thumbnail() since the core will
check the validity of the thumbnail for you. I will look into API for
deleting and renaming thumbnails. Not sure if we really want a PDB API
for this. Alternatively we could move some functions from
app/core/gimpimagefile.[ch] into libgimp.

Updated gimp_file_load_thumbnail(). Should work for RGB images, still
needs to be extended for other image types:

2003-05-15  Sven Neumann  <sven@gimp.org>

  * tools/pdbgen/pdb/fileops.pdb (file_load_thumbnail_invoker): use
  GimpImagefile to load a thumbnail. Only works for RGB images yet.
  This adresses bug #113033.

  * app/pdb/fileops_cmds.c: regenerated.

  * app/file/file-utils.[ch]: removed file_utils_readXVThumb(). It
  is not needed any longer since GimpImagefile also handles the
  old-style .xvpics.

2003-05-15  Sven Neumann  <sven@gimp.org>

  * app/pdb/fileops_cmds.c
  * tools/pdbgen/pdb/fileops.pdb: fixed docs for file_save_thumbnail
  (bug #113033).

The following change is completely untested:

2003-05-16  Sven Neumann  <sven@gimp.org>

   * app/pdb/fileops_cmds.c
   * tools/pdbgen/pdb/fileops.pdb: this change should fix
   file_load_thumbnail() for images with alpha channel by blending
   the resulting tempbuf on a checkerboard (bug #113033).

Assuming the change is correct, the existing PDB functions are now
completely converted to the new thumbnailing scheme. The remaining
parts are an enhancement request; changing the bug-report accordingly.

The GAP VCR Navigator dialog needs a way to check
for vaild thumbnail
for The 'Smart Thumbnail Update' feature.

The full thumbnail update does load all frames of an animation as 
gimp image
and then calls gimp_file_save_thumbnail to create the thumbnail.
the smart update can skip load and thumbnail creation
for imagefiles on disk that have a vaild thumbnail.

Keep in mind that the GAP VCR Navigator (and some other GAP functions)
mainly deals with imagefiles
in a way like a filemanager does,
and usually does not open the images (execpt the current one)
into the GIMP.

I think GAP should at least support the same Thumbnail handling as the
GIMP-core does.

One thing that GAP will do, is introduce a new gimprc Parameter
(video-thumbnails) where the user can select:

 - "none" GAP does NOT create new Thumbnails at all.
    Frames with no thumbnails are presented by an empty standard icon
    in the VCR Navigator dialog.
    Some Users may preferre this, because they dont like too much
    thumbnail files in their home directory  ~/.thumbnails

 - "png" GAP controlled save actions create Thumbnails in 
    the new PNG based standard.
    this should be the default.


>What do you need gimp_file_delete_thumbnail() for?

For all GAP controlled delete image-diskfile ops
GAP should remove Thumbnails for both standards,
regardless of gimprc Thumbnail settings.

(Some Users (like me) dont have a demon
 for thumbnail garbage cleanup)


For all GAP controlled copy and rename image-diskfile operations
GAP should also copy/rename Thumbnails for both standards
(regardless of the gimprc Thumbnail settings)

==> In the the new standard the thumbnailfile 
    must be updated because the PNG-Tag:
    Thumb::URI
    has to change on rename and copy.

    For perfromance reasons it is not a good idea
    to recreate thumbnails by loading (all the) renamed/copied
    originalsized image(s) into the GIMP just to create the same 
thumbnail
    with the new Thumb::URI Tag(s).
    (especially in GAP where copy/rename fileops
    are done on many frames at once.)

Writing these lines i can see that i forgot one more new API
in this feature request.

  gimp_file_copy_thumbnail(old_filename, new_filename)


The new requested procedures
  gimp_file_has_valid_thumbnail
  gimp_file_delete_thumbnail
  gimp_file_copy_thumbnail
  gimp_file_rename_thumbnail
  
can be part of the public GIMP Libraries, linkable
with Plug-Ins (A PDB API is not reqired here)


See gimp-gap-1.3.14a_pre37  gap/gap_thumbnail.c
for more implementation details.
(http://wolfganghofer.tripod.com/gap)

This code already implements what GAP needs
for thumbnail handling,
but i will keep this feature request open
because i think an API to the GIMP
is the better solution.

A few remarks:

You can check for a valid thumbnail by trying to load the thumbnail.
If there is no valid thumbnail, the function will return FALSE. That
should be sufficient, isn't it?

I would not introduce a new gimprc parameter for gap thumbnails. The
gimprc already allows to enable/disable saving of thumbnails. That
should be good enough. More options will only confuse people.

I don't think we want to add deletion of thumbnails to the core.
It would probably make more sense to move some of the gimpimagefile
functionality into libgimp. The same holds true for copying and renaming.

> You can check for a valid thumbnail by trying to load the thumbnail.
> If there is no valid thumbnail, the function will return FALSE. That
> should be sufficient, isn't it?

Good idea, if the thumbnail load procedure returns false on
out of date thumbnails too
(both for PNG and .xvpics) it is best that i
drop the request for gimp_file_has_valid_thumbnail

> I would not introduce a new gimprc parameter for gap thumbnails. The
> gimprc already allows to enable/disable saving of thumbnails.

OK, accepted (i'll change this in the next gimp-gap patch
and check only the GIMP standard gimprc keyword
thumbnail-size "none")

>It would probably make more sense to move some of the gimpimagefile
>functionality into libgimp. The same holds true for copying and 
renaming.

OK

The PDB call gimp_file_load_thumbnail always FAILS.
the main reason for this bug seems to be
that  imagefile->state  is 0 (== GIMP_IMAGEFILE_STATE_UNKNOWN)
when 
   gimp_imagefile_get_new_preview is called
To load the thumbnail, the imagefile must have at least
state 3 (GIMP_IMAGEFILE_STATE_EXISTS)
   CALLING TREE:
   file_load_thumbnail_invoker
     gimp_viewable_get_preview
       gimp_imagefile_get_new_preview
         gimp_imagefile_read_png_thumb

Suggested Bugfix (1):

  the file_load_thumbnail_invoker
  Should first check if the passed filename exist

  and set state to
    GIMP_IMAGEFILE_STATE_EXISTS
  according to this test.
  ==> my bugfix adds a call to:
         gimp_imagefile_update (imagefile, size);
      
      before the call of gimp_viewable_get_preview.
      gimp_imagefile_update  makes the check and sets the state.

Suggested Bugfix (2):
  After fixing (1), gimp_file_load_thumbnail
  crashed at temp_buf_free.
  the reason is that temp_buf_free is called twice for
  the same temp_buf and crashes in the 2nd call.
  The first call to temp_buf_free is an implicite one,
  that is hidden somewhere in the call to:
     g_object_unref (imagefile);

  ==> my bugfix assumes that g_object_unref
      is OK and calls temp_buf_free only for
      the other TempBuf pointers used in this procedure.
      My bugfix worked fine at my tests, but may be the wrong way,
      because i am not familiar with the object design of the 
      GIMP core.

     
      The changes to fix (1) and (2) should be applied to 
  tools/pdbgen/pdb/fileops.pdb
  (i just have gimp-1.3.14 sources and
   CVS Blame annotated version of app/pdb/fileops_cmds.c
   with the first bugfix attempt for thubnail loading. (by sven)
   -- forgot to download fileops.pdb --
   therefore i cant send
   a patch for the tools/pdbgen/pdb/fileops.pdb
  )
  but here is how the CODE for the file_load_thumbnail_invoker
  should look after the pdbgen sourcecode generation:

---------------------------- START gimp-
1.3.14/app/pdb/fileops_cmds.c:file_load_thumbnail_invoker ------
static Argument *
file_load_thumbnail_invoker (Gimp     *gimp,
                             Argument *args)
{
  gboolean success = TRUE;
  Argument *return_args;
  gchar *filename;
  gint32 width = 0;
  gint32 height = 0;
  gint32 num_bytes = 0;
  guint8 *thumb_data = NULL;

  gchar *uri;
  GimpImagefile *imagefile = NULL;
  TempBuf *temp_buf = NULL;

  filename = (gchar *) args[0].value.pdb_pointer;
  if (filename == NULL)
    success = FALSE;

  if (success)
    {
      TempBuf *temp_buf_check = NULL;

      uri = g_filename_to_uri (filename, NULL, NULL);

      if (uri)
       imagefile = gimp_imagefile_new (gimp, uri);

      if (imagefile)
       {
         gimp_imagefile_update (imagefile, 
GIMP_THUMBNAIL_SIZE_NORMAL);

 	 temp_buf = gimp_viewable_get_preview (GIMP_VIEWABLE 
(imagefile),
 						   
GIMP_THUMBNAIL_SIZE_NORMAL,
 					       
GIMP_THUMBNAIL_SIZE_NORMAL);
 	 /* dont call g_object_unref (imagefile) too soon, 
          * because it implicite calls temp_buf_free
          */
       }

      if (temp_buf)
       {
 	 width  = temp_buf->width;
 	 height = temp_buf->height;

 	 switch (temp_buf->bytes)
 	   {
 	   case 3:
 	     break;

 	   case 4:
 	     {
    #define INT_MULT(a,b,t)  ((t) = (a) * (b) + 0x80, ((((t) >> 8) + 
(t)) >> 8))
    #define INT_BLEND(a,b,alpha,t)  (INT_MULT((a)-(b), alpha, t) + 
(b))
     
               TempBuf *checks = temp_buf_new_check (width, height,
 						
	GIMP_GRAY_CHECKS,
                                                        
GIMP_SMALL_CHECKS
 						      );
               guchar  *src  = temp_buf_data (temp_buf);
               guchar  *dest = temp_buf_data (checks);
                   
               num_bytes = width * height;
               while (num_bytes--)
                 {
                   register glong t;
    
                   dest[0] = INT_BLEND (src[0] , dest[0] , src[3], t);
                   dest[1] = INT_BLEND (src[1] , dest[1] , src[3], t);
                   dest[2] = INT_BLEND (src[2] , dest[2] , src[3], t);
                   src  += 4;
                   dest += 3;
                 }
    
               if(!imagefile)
                   temp_buf_free (temp_buf);

               temp_buf_check = checks;
               temp_buf = checks;
             }
             break;
                 
           default:
             g_assert_not_reached ();
             break;
         }

         num_bytes  = 3 * width * height;
         thumb_data = g_memdup (temp_buf_data (temp_buf), num_bytes);
    
         if(imagefile)
         {
 	   g_object_unref (imagefile);
           
           /* note: g_object_unref does already call temp_buf_free 
implicte !
            * dont call temp_buf_free again, it would lead to a crash
            */
           if(temp_buf_check)
              temp_buf_free (temp_buf_check);
             
         }
         else
         {
            temp_buf_free (temp_buf);
         }

         success = TRUE;
       }
      else
       {
         success = FALSE;
       }
    
       g_free (uri);

    }

  return_args = procedural_db_return_args (&file_load_thumbnail_proc, 
success);

  if (success)
    {
      return_args[1].value.pdb_int = width;
      return_args[2].value.pdb_int = height;
      return_args[3].value.pdb_int = num_bytes;
      return_args[4].value.pdb_pointer = thumb_data;
    }

  return return_args;
}
---------------------------- END   gimp-
1.3.14/app/pdb/fileops_cmds.c:file_load_thumbnail_invoker -----

Suggested Bugfix (3):

  After fixing (1) and (2) i could see that 
  transparent parts of the thumbnail data contain only
  trash (== unitialized data) instead of the expected chekboard.

  ==> call of temp_buf_new_check  
    mismatch par 3. and 4
    3. param is check_type   (GIMP_GRAY_CHECKS)
    4. param is check_size   (GIMP_SMALL_CHECKS)
    

  ==> the implementation of temp_buf_new_check (gimp-1.3.14)
    does not work at all.

   the patch_temp_buf.c  fixes temp_buf_new_check
   please note that the patch is done against gimp-1.3.14 
   (not against current CVS) because i have no CVS access.

---------------------------- START  patch_temp_buf.c  (gimp-
1.3.14/app/base/temp-buf.c) ----------
--- temp-buf.c.orig.1.3.14	Sat Jun  7 12:11:17 2003
+++ temp-buf.c	Sun Jun  8 11:50:27 2003
@@ -236,23 +236,23 @@
 {
   TempBuf *newbuf;
   guchar  *data;
-  guchar   check_shift = 0;
+  guchar   check_pixels = 0;
   guchar   fg_color = 0;
   guchar   bg_color = 0;
-  gint     i, j;
+  gint     ii, i, j;
 
   g_return_val_if_fail (width > 0 && height > 0, NULL);
 
   switch (check_size)
     {
     case GIMP_SMALL_CHECKS:
-      check_shift = 2;
+      check_pixels = 8;
       break;
     case GIMP_MEDIUM_CHECKS:
-      check_shift = 4;
+      check_pixels = 12;
       break;
     case GIMP_LARGE_CHECKS:
-      check_shift = 6;
+      check_pixels = 16;
       break;
     }
  
@@ -286,13 +286,20 @@
   newbuf = temp_buf_new (width, height, 3, 0, 0, NULL);
   data = temp_buf_data (newbuf);
 
-  for (i = 0, j = 1; i <= height; j++)
+  for (ii=0, j = 0; j < height; j++)
     {
-      for (i = 1; i <= width; i++)
-	{
-	  *(data + i - 1) = ((j & check_shift) && (i & check_shift)) 
-	                    ? fg_color : bg_color;
-	}	  
+      ii = ((j / check_pixels) & 1) ? 0 : check_pixels;
+      
+      for (i = 0+ii ; i < width+ii ; i++)
+	{  
+           register guchar   color = 0;
+
+           color = ((i / check_pixels) &1)  ?  fg_color : bg_color;
+                            
+	  *(data++) = color;
+	  *(data++) = color;
+	  *(data++) = color;
+	}
     } 
 
   return newbuf;
@@ -506,6 +513,8 @@
   if (temp_buf->data)
     g_free (temp_buf->data);
 
+  temp_buf->data = NULL;
+  
   if (temp_buf->swapped)
     temp_buf_swap_free (temp_buf);
 
---------------------------- END  patch_temp_buf.c  (gimp-
1.3.14/app/base/temp-buf.c) ----------

Here is an additional wish:
  
  please add one parameter to gimp_file_load_thumbnail
  where the caller can choose beetween
  
    GIMP_THUMBNAIL_SIZE_NORMAL or GIMP_THUMBNAIL_SIZE_LARGE.
  
  (I think today GAP is the only user of the gimp_file_load_thumbnail
   procedure, so this API change should be no big risk)
 
   this way GAP can display huge thumbnails
   in better quality (PNG 256x256).
   This also would make sense for the planed thumbnail based video
   playback GAP-feature.
   
   For Performance Reasons it wolud be better choice to implement
   gimp_file_load_thumbnail as libgimp procedure.
   The current PDB Implementation might be too slow
   for video playback on slow machines.

(temp) set severity to normal
because gimp_file_load_thumbnail does not work in the
current PDB implementation and should be fixed before gimp 1.4
release is out. the other things are just feature requests.

(the GAP Player module is nearly done, and can play
at speed of 16 frames/sec using the fixed PDB implementation
of gimp_file_load_thumbnail on normal sized thumbnail files
and PII 300MHz Machine) 

Adding PATCH keyword to bugs which reference code capable of solving any
outstanding issues in the bug

Dave.

Having discussed this with Sven, I agree (with him) that plug-ins
should be able to load, create, save, update and all the rest plug-ins
without asking the gimp core to do it. The solution is to bunch all
the thumbnail functions from the core into a libgimpthumbnail.so, and
link against it for the plug-ins who want thumbnail functionality.

This would be pretty straightforward, I think, and should be done
before 2.0 if possible.

Dave.

This change should fix the problems with the existing API:

2003-08-25  Sven Neumann  <sven@gimp.org>

  Fixed some issues with the PDB thumbnail functions spotted by
  Wolfgang Hofer and loosely based on patches he provided:
	
  * tools/pdbgen/pdb/fileops.pdb (file_load_thumbnail): keep a
  reference on the GimpImagefile as long as we need the associated
  preview temp_buf. Call gimp_imagefile_update() before requesting
  the preview.

  * app/pdb/fileops_cmds.c: regenerated.

  * app/base/temp-buf.c (temp_buf_new_check): use a checkerboard
  algorithm similar to the one the displayshell-render code uses.


The library for thumbnail loading is almost done. I hope to get it
into  CVS later this week.

Most of the code for the thumbnail API seems to be in CVS. Is there
anything left to do in this bug report?

Dave.

Yes, there are one or two things left but these will probably make it
into CVS today.

I haven't yet closed this bug for a couple of reasons. One was missing
documentation. This has been fixed in the meantime. A couple of things
are still missing though:

(1)  gimp_thumbnail_load_thumb() needs to reviewed. It should use
     gimp_thumbnail_peek_image() and gimp_thumbnail_peek_thumb()
     instead of reimplementing the tests.

(2)  I'd like to add a function to set an image file from a thumbnail.
     This wouldn't be used from GIMP but it would be a trivial
     addition that would allow to implement a thumbnail browser/manager
     on top of libgimpthumb.

(3)  Support for reading .xvpics was dropped when GimpImagefile was 
     ported to GimpThumbnail. I'm in favor of not adding it again but
     I'm undecided. I have the code ready to add it again (in the 
     core, not in libgimpthumb) but I'm not sure if I should add back.

I'd like to get some feedback on point (3) of my last comment.
Basically I am willing to close this report now and make sure (1) and
(2) are taken care of before 2.0 is released.

I'm in favour of dropping .xvpics support entirely.In any case, all 
we would do (presumably) is read the .xvpics thumbnail, and generate 
a .thumbnails thumb from it. 

So I vote for this to be closed as-is.

Dave.

What the old implementation was doing was to read the .xvpics thumb
and display it as possibly outdated because it doesn't have a way to
determine if the thumb is uptodate. The bad thing about the .xvpics
thumbs is that limited colors and the fact that transparency is not
supported. Instead the checkerboard is stored the thumbnail :(

Closing this as FIXED. If there are issues with the new API or its
implementation, please open a new report.