Addition of Gdk_New_From_Data into Gdk.Pixbuf

[Anne et Damien Carbonne]

Hi,

As Gdk_New_From_Data is currently missing from Gdk.Pixbuf, I created a
binding for it (in a child package of Gtk.Pixbuf).
Would you consider adding this directly into to Gtk.Pixbuf?
Is the attached code sufficient or would you prefer a diff?

Regards,

Damien Carbonne


------------------------------------------------------------------------------------
-- Specification

-- with System;

   type Guchar_Array_Ptr is access all Guchar_Array;

   type Gdk_Destroy_Notify is access procedure
     (Pixels : in out Guchar_Array_Ptr;
      Data : System.Address);

   function Gdk_New_From_Data
     (Data            : Guchar_Array_Ptr;
      Colorspace      : Gdk_Colorspace := Colorspace_RGB;
      Has_Alpha       : Boolean := False;
      Bits_Per_Sample : Gint := 8;
      Width           : Gint;
      Height          : Gint;
      Rowstride       : Gint;
      Destroy_Fn      : Gdk_Destroy_Notify := null;
      Destroy_Fn_Data : System.Address := System.Null_Address)
      return Gdk_Pixbuf;
   --  Creates a new GdkPixbuf out of in-memory image data.
   --  Currently only RGB images with 8 bits per sample are supported.
   --
   --  Width and Height must be > 0
   --  Rowstride is the distance in bytes between row starts.
   --  A typical value is 4 * Width when ther is an Alpha channel.
   --  Destroy_Fn is an access to a procedure used to free data when the
   --  pixbuf's reference count drops to zero, or null if teh data
should not
   --  be freed.
   --  Destroy_Fn_Data is the closure data to pass to the destroy
notification
   --  procedure.
   --  This function returns the newly-created GdkPixbuf structure with a
   --  reference count of 1


------------------------------------------------------------------------------------
-- Body

-- with Ada.Unchecked_Conversion;
-- with Ada.Unchecked_Deallocation;

   type Proxy_Data is record
      Pixels : Guchar_Array_Ptr;
      Destroy_Fn : Gdk_Destroy_Notify;
      Destroy_Fn_Data : System.Address;
   end record;
   --  This is used as a proxy object to do minimum conversions from C types
   --  to Ada types.
   --  By copying Pixels (access) in this record, we avoid warnings at
   --  instantiation of System.Address_To_Access_Conversions
   --  with unconstrained arrays.

   type Proxy_Data_Ptr is access Proxy_Data;

   procedure Proxy_Destroy
     (Pixels : System.Address;
      Data : System.Address); -- expected to be of type Proxy_Data_Ptr

   -----------------------
   -- Gdk_New_From_Data --
   -----------------------

   function Gdk_New_From_Data
     (Data            : Guchar_Array_Ptr;
      Colorspace      : Gdk_Colorspace := Colorspace_RGB;
      Has_Alpha       : Boolean := False;
      Bits_Per_Sample : Gint := 8;
      Width           : Gint;
      Height          : Gint;
      Rowstride       : Gint;
      Destroy_Fn      : Gdk_Destroy_Notify := null;
      Destroy_Fn_Data : System.Address := System.Null_Address)
      return Gdk_Pixbuf
   is
      function Internal
        (Data            : System.Address;
         Colorspace      : Gdk_Colorspace;
         Has_Alpha       : GBoolean;
         Bits_Per_Sample : Gint;
         Width           : Gint;
         Height          : Gint;
         Row_Stride      : Gint;
         Destroy_Fn      : System.Address;
         Destroy_Fn_Data : System.Address)
         return Gdk_Pixbuf;
      pragma Import (C, Internal, "gdk_pixbuf_new_from_data");
      use type System.Address;
   begin
      if Destroy_Fn /= null then
         -- Create a proxy data only if the user has provided
         -- a destroy function.
         -- If only destroy_fn_data has been provided, this is an error
         declare
            C_Data : constant Proxy_Data_Ptr := new Proxy_Data;
         begin
            C_Data.Destroy_Fn := Destroy_Fn;
            C_Data.Destroy_Fn_Data := Destroy_Fn_Data;
            C_Data.Pixels := Data;

            return Internal
              (Data.all'Address,
               Colorspace, Boolean'Pos (Has_Alpha), Bits_Per_Sample,
               Width, Height, Rowstride,
               Proxy_Destroy'Address, C_Data.all'Address);
         end;
      else
         pragma Assert (Destroy_Fn_Data = System.Null_Address);
         return Internal
           (Data.all'Address,
            Colorspace, Boolean'Pos (Has_Alpha), Bits_Per_Sample,
            Width, Height, Rowstride,
            System.Null_Address, System.Null_Address);
      end if;
   end Gdk_New_From_Data;

   -------------------
   -- Proxy_Destroy --
   -------------------

   procedure Proxy_Destroy
     (Pixels : System.Address;
      Data : System.Address)
   is
      use type System.Address;
      function To_Ada is new
        Ada.Unchecked_Conversion (System.Address, Proxy_Data_Ptr);
      procedure Free is new
        Ada.Unchecked_Deallocation (Proxy_Data, Proxy_Data_Ptr);
      Ada_Data : Proxy_Data_Ptr := To_Ada (Data);
   begin
      pragma Assert (Ada_Data /= null);
      pragma Assert (Pixels = Ada_Data.Pixels.all'Address);
      Ada_Data.Destroy_Fn.all (Ada_Data.Pixels, Ada_Data.Destroy_Fn_Data);
      Free (Ada_Data);
   end Proxy_Destroy;

------------------------------------------------------------------------------------

_______________________________________________
gtkada mailing list
gtkada@...
http://lists.adacore.com/mailman/listinfo/gtkada

[Arnaud Charlet]

> As Gdk_New_From_Data is currently missing from Gdk.Pixbuf, I created a
> binding for it (in a child package of Gtk.Pixbuf).
> Would you consider adding this directly into to Gtk.Pixbuf?
> Is the attached code sufficient or would you prefer a diff?

A patch (diff) against svn trunk as well as a ChangeLog would be better.

The patch will probably need to be reworked before integration, since
the API you provide is pretty low-level (in particular the handling of
Destroy_Fn/Destroy_Fn_Data).

I'd suggest either remove the two last parameters, or perhaps provide a single
extra parameter: Destroy_Fn, removing the use of user data from the spec
(and still use it under the hood in the body).

The spec (comments) should probably also reflect/explain this business
of destroy/user data.

Arno
_______________________________________________
gtkada mailing list
gtkada@...
http://lists.adacore.com/mailman/listinfo/gtkada

[Anne et Damien Carbonne]

Thanks for answer,

I tried to provide a higher level of abstraction than the C one.
But I left a System.Address for user data!

1) There would be the solution to use an interface for destruction, but
that would be Ada 2005.
I guess it would not be acceptable!

   type Gdk_Finalizer is interface;
   tytpe Gdk_Finalizer_ref is access all Gdk_Finalizer'Class;
   procedure Clean (Finalizer : in out | access Finalizer;
                    Pixels : in out Guchar_Array_Ptr);

   function Gdk_New_From_Data
     (Data            : Guchar_Array_Ptr;
      Colorspace      : Gdk_Colorspace := Colorspace_RGB;
      Has_Alpha       : Boolean := False;
      Bits_Per_Sample : Gint := 8;
      Width           : Gint;
      Height          : Gint;
      Rowstride       : Gint;
      Finalizer       : Gdk_Finalizer_Ref)
      return Gdk_Pixbuf;


2) The other solution would consist in defining an abstract base class
instead of an interface.

   type Gdk_Finalizer is abstract tagged null record;
   procedure Clean (Finalizer : in out | access Finalizer;
                    Pixels : in out Guchar_Array_Ptr);

   function Gdk_New_From_Data
     (Data            : Guchar_Array_Ptr;
      Colorspace      : Gdk_Colorspace := Colorspace_RGB;
      Has_Alpha       : Boolean := False;
      Bits_Per_Sample : Gint := 8;
      Width           : Gint;
      Height          : Gint;
      Rowstride       : Gint;
      Finalizer       : Gdk_Finalizer_Ref)
      return Gdk_Pixbuf;

In both cases the user could attach its own data for handling
destruction of allocated memory, by deriving Finalizer.

3) If we want to only allow destruction of the allocated data (without
distinction), then we could suppress the second parameter of the destroy
procedure.

   type Gdk_Finalizer is access procedure (Pixels : in out
Guchar_Array_Ptr);

   function Gdk_New_From_Data
     (Data            : Guchar_Array_Ptr;
      Colorspace      : Gdk_Colorspace := Colorspace_RGB;
      Has_Alpha       : Boolean := False;
      Bits_Per_Sample : Gint := 8;
      Width           : Gint;
      Height          : Gint;
      Rowstride       : Gint;
      Finalizer       : Gdk_Finalizer)
      return Gdk_Pixbuf;


Is that what you have in mind?
I suppose one could find examples where this second parameter would be
missing!

Or do you imagine that data would be automatically destroyed by the Ada
binding?
In which case one would not need to pass the 2 last parameters
(destroy_fn and destroy_fn_data).

4) We could then simply pass a boolean telling whether memory must be
deallocated or not.

   function Gdk_New_From_Data
     (Data              : Guchar_Array_Ptr;
      Colorspace        : Gdk_Colorspace := Colorspace_RGB;
      Has_Alpha         : Boolean := False;
      Bits_Per_Sample   : Gint := 8;
      Width             : Gint;
      Height            : Gint;
      Rowstride         : Gint;
      Auto_Destroy_Data : Boolean := True)
      return Gdk_Pixbuf;


Any of these solutions would fit the usage I did of this binding.

What do you have in mind exactly ? One of those solutions or another one ?

Best regards,
Damien Carbonne


Arnaud Charlet a écrit :

>> As Gdk_New_From_Data is currently missing from Gdk.Pixbuf, I created a
>> binding for it (in a child package of Gtk.Pixbuf).
>> Would you consider adding this directly into to Gtk.Pixbuf?
>> Is the attached code sufficient or would you prefer a diff?
>>    
>
> A patch (diff) against svn trunk as well as a ChangeLog would be better.
>
> The patch will probably need to be reworked before integration, since
> the API you provide is pretty low-level (in particular the handling of
> Destroy_Fn/Destroy_Fn_Data).
>
> I'd suggest either remove the two last parameters, or perhaps provide a single
> extra parameter: Destroy_Fn, removing the use of user data from the spec
> (and still use it under the hood in the body).
>
> The spec (comments) should probably also reflect/explain this business
> of destroy/user data.
>
> Arno
>
>
>  

_______________________________________________
gtkada mailing list
gtkada@...
http://lists.adacore.com/mailman/listinfo/gtkada

[Anne et Damien Carbonne]

Arnaud Charlet a écrit :As Gdk_New_From_Data is currently missing from
Gdk.Pixbuf, I created a

>> binding for it (in a child package of Gtk.Pixbuf).
>> Would you consider adding this directly into to Gtk.Pixbuf?
>> Is the attached code sufficient or would you prefer a diff?
>>    
>
> A patch (diff) against svn trunk as well as a ChangeLog would be better.
>
> The patch will probably need to be reworked before integration, since
> the API you provide is pretty low-level (in particular the handling of
> Destroy_Fn/Destroy_Fn_Data).
>
> I'd suggest either remove the two last parameters, or perhaps provide a single
> extra parameter: Destroy_Fn, removing the use of user data from the spec
> (and still use it under the hood in the body).
>
> The spec (comments) should probably also reflect/explain this business
> of destroy/user data.
>
> Arno
>  

I checked out current (2009-07-14) svn trunk. I have attached a possible

patch usable on the trunk.
I replaced the destroy cb and data with a boolean parameter used to tell
whether pixel data must be automatically freed.
Does it fit what you have in mind?

It would be possible to add another version, that would be generic and
would have an associated user data type.

generic
  type Closure_Type is ...
package Generic_XXX is
   type Destroy_Callback is access procedure
     (Pixels : in out Guchar_Array_Access;
      Closure : in out Closure_Type);

   function New_from_Data
      (Data : Guchar_Array_Access;
       ...
       Closure : Closure_Type) return Gdk_Pixbuf;
end Generic_XXX;

If you think it is worth, I could provide an additional patch with the
generic version.

Damien

diff -rc old/gdk-pixbuf.adb new/gdk-pixbuf.adb
*** old/gdk-pixbuf.adb 2009-07-14 12:00:09.000000000 +0200
--- new/gdk-pixbuf.adb 2009-07-14 14:09:02.000000000 +0200
***************
*** 29,37 ****
--- 29,63 ----
  with Gdk.Cursor;  use Gdk.Cursor;
  with Gdk.Display; use Gdk.Display;
  with Glib.Object; use Glib.Object;
+ with Ada.Unchecked_Deallocation;
 
  package body Gdk.Pixbuf is
 
+    type Proxy_Data is record
+       Pixels : Guchar_Array_Access;
+       --  Access to the Ada allocated memory used by Gdk_New_From_Data.
+       --  Strictly, this is not necessary, as this access could be
+       --  built from the Pixels parameter of the destroy callback.
+       --  However, converting an Address to an access to unconstrained
+       --  array with System.Address_To_Access_Conversions generates
+       --  a warning.
+    end record;
+    pragma Convention (C, Proxy_Data);
+    --  Closure data attached to a pixbuf created from user data.
+
+    type Proxy_Data_Access is access Proxy_Data;
+    pragma Convention (C, Proxy_Data_Access);
+
+    procedure Free is new Ada.Unchecked_Deallocation
+      (Object => Proxy_Data, Name => Proxy_Data_Access);
+
+    procedure Destroy_Pixels
+      (Pixels : System.Address;
+       Data   : Proxy_Data_Access);
+    pragma Convention (C, Destroy_Pixels);
+    --  Low level, C compatible, callback called when the reference
+    --  count of a pixbuf (that was created from user data) reaches 0.
+
     ---------------
     -- Add_Alpha --
     ---------------
***************
*** 276,281 ****
--- 302,321 ----
           Dest_Y);
     end Copy_Area;
 
+    --------------------
+    -- Destroy_Pixels --
+    --------------------
+
+    procedure Destroy_Pixels
+      (Pixels : System.Address;
+       Data   : Proxy_Data_Access)
+    is
+       Tmp : Proxy_Data_Access := Data;
+    begin
+       Free (Data.Pixels);
+       Free (Tmp);
+    end Destroy_Pixels;
+
     ----------
     -- Fill --
     ----------
***************
*** 318,323 ****
--- 358,424 ----
     end Gdk_New;
 
     -----------------------
+    -- Gdk_New_From_Data --
+    -----------------------
+
+    function Gdk_New_From_Data
+      (Data              : Guchar_Array_Access;
+       Colorspace        : Gdk_Colorspace := Colorspace_RGB;
+       Has_Alpha         : Boolean := False;
+       Bits_Per_Sample   : Gint := 8;
+       Width             : Gint;
+       Height            : Gint;
+       Rowstride         : Gint;
+       Auto_destroy_Data : Boolean := True) return Gdk_Pixbuf
+    is
+       function Internal
+         (Data            : System.Address;
+          Colorspace      : Gdk_Colorspace;
+          Has_Alpha       : GBoolean;
+          Bits_Per_Sample : Gint;
+          Width           : Gint;
+          Height          : Gint;
+          Row_Stride      : Gint;
+          Destroy_Fn      : System.Address;
+          Destroy_Fn_Data : System.Address) return System.Address;
+       pragma Import (C, Internal, "gdk_pixbuf_new_from_data");
+
+    begin
+       if Auto_Destroy_Data then
+          declare
+             Closure_Data : constant Proxy_Data_Access := new Proxy_Data;
+          begin
+             Closure_Data.Pixels := Data;
+
+             return Convert
+               (Internal
+                  (Data.all'Address,
+                   Colorspace,
+                   Boolean'Pos (Has_Alpha),
+                   Bits_Per_Sample,
+                   Width,
+                   Height,
+                   Rowstride,
+                   Destroy_Pixels'Address,
+                   Closure_Data.all'Address));
+          end;
+
+       else
+          return Convert
+            (Internal
+               (Data.all'Address,
+                Colorspace,
+                Boolean'Pos (Has_Alpha),
+                Bits_Per_Sample,
+                Width,
+                Height,
+                Rowstride,
+                System.Null_Address,
+                System.Null_Address));
+       end if;
+    end Gdk_New_From_Data;
+
+    -----------------------
     -- Gdk_New_From_File --
     -----------------------
 
diff -rc old/gdk-pixbuf.ads new/gdk-pixbuf.ads
*** old/gdk-pixbuf.ads 2009-07-14 12:00:09.000000000 +0200
--- new/gdk-pixbuf.ads 2009-07-14 14:09:02.000000000 +0200
***************
*** 219,224 ****
--- 219,242 ----
        Error    : out GError);
     --  Load an image from file.
 
+     function Gdk_New_From_Data
+      (Data              : Guchar_Array_Access;
+       Colorspace        : Gdk_Colorspace := Colorspace_RGB;
+       Has_Alpha         : Boolean := False;
+       Bits_Per_Sample   : Gint := 8;
+       Width             : Gint;
+       Height            : Gint;
+       Rowstride         : Gint;
+       Auto_Destroy_Data : Boolean := True) return Gdk_Pixbuf;
+    --  Create a pixbuf out of in-memory image data.
+    --  Currently only RGB images with 8 bits per sample are supported.
+    --  Width and Height must be > 0.
+    --  Rowstride is the distance in bytes between row starts.
+    --  A typical value is 4*Width when there is an Alpha channel.
+    --  If Auto_Destroy_Data is true, passed data will be automatically
+    --  freed when the reference count of the pixbuf reaches 1.
+    --  Otherwise, data is never freed.
+
     function Gdk_New_From_Xpm_Data
       (Data : Interfaces.C.Strings.chars_ptr_array) return Gdk_Pixbuf;
     --  Create an image from a XPM data.
***************
*** 682,686 ****
       (C, Get_Delay_Time, "gdk_pixbuf_animation_iter_get_delay_time");
 
  end Gdk.Pixbuf;
-
- --  missing: gdk_pixbuf_new_from_data
--- 700,702 ----

_______________________________________________
gtkada mailing list
gtkada@...
http://lists.adacore.com/mailman/listinfo/gtkada

[Arnaud Charlet]

> I checked out current (2009-07-14) svn trunk. I have attached a possible
> patch usable on the trunk.
> I replaced the destroy cb and data with a boolean parameter used to tell
> whether pixel data must be automatically freed.
> Does it fit what you have in mind?

Yes, that's close to what I had in mind.

> It would be possible to add another version, that would be generic and
> would have an associated user data type.

Possibly, although nobody has had a need for it yet, so I'd suggest keeping
this possible solution (or the use of an abstract record as you suggested in
your previous email) for later if the need arises.

Thanks for the patch, I'll review it and integrate it!

Arno
_______________________________________________
gtkada mailing list
gtkada@...
http://lists.adacore.com/mailman/listinfo/gtkada

[Arnaud Charlet]

> Thanks for the patch, I'll review it and integrate it!

Reviewed, cleaned up and committed.

Arno
_______________________________________________
gtkada mailing list
gtkada@...
http://lists.adacore.com/mailman/listinfo/gtkada