Created attachment 212224 [details] [review]
Fix len_arg_index for array arguments

Don't set len_arg_index for arrays without the length annotation given.
This fixes methods like Clutter.Texture.set_from_rgb_data() and
Clutter.Image.set_data()

This looks like an annotation error in Clutter though?

          <parameter name="data" transfer-ownership="none">
            <doc xml:whitespace="preserve">image data in RGBA type colorspace.</doc>
            <array zero-terminated="0" c:type="guchar*">
              <type name="guint8" c:type="guchar"/>
            </array>
          </parameter>

That claims the array is zero terminated, so how does that function actually trigger a problem?

That said, I see that the code for handling non-zero-terminated arrays is clearly wrong when there is no length field or not a single one (I guess in that case the length is height*rowstride).

This needs a test case in g-i (tests/gimarshallingtests.[hc]) and in pygobject (tests/test_gi.py TestArray). If you want to work on that, I'd be grateful, otherwise I'll find some time for that in the next days.

Thanks for the fix!

(In reply to comment #2)
> This looks like an annotation error in Clutter though?
> 
>           <parameter name="data" transfer-ownership="none">
>             <doc xml:whitespace="preserve">image data in RGBA type
> colorspace.</doc>
>             <array zero-terminated="0" c:type="guchar*">
>               <type name="guint8" c:type="guchar"/>
>             </array>
>           </parameter>
> 
> That claims the array is zero terminated, so how does that function actually
> trigger a problem?

The girparser thinks different:
http://git.gnome.org/browse/gobject-introspection/tree/girepository/girparser.c#n1918

Just take this simple testcase:


import sys
from gi.repository import GdkPixbuf
from gi.repository import Clutter

Clutter.init([])

pixbuf = GdkPixbuf.Pixbuf.new_from_file(sys.argv[1])
texture = Clutter.Texture()
texture.set_from_rgb_data(
        pixbuf.get_pixels(),
        pixbuf.get_has_alpha(),
        pixbuf.get_width(),
        pixbuf.get_height(),
        pixbuf.get_rowstride(),
        4 if pixbuf.get_has_alpha() else 3,
        Clutter.TextureFlags.NONE
)

TypeError: set_from_rgb_data() takes exactly 7 arguments (8 given)


This happens because the first argument (the texture itself) is taken as
length argument

> That said, I see that the code for handling non-zero-terminated arrays is
> clearly wrong when there is no length field or not a single one (I guess in
> that case the length is height*rowstride).
> 
> This needs a test case in g-i (tests/gimarshallingtests.[hc]) and in pygobject
> (tests/test_gi.py TestArray). If you want to work on that, I'd be grateful,
> otherwise I'll find some time for that in the next days.

I'll try to build a proper test case tonight/tomorrow!

(In reply to comment #2)
>             <array zero-terminated="0"
                                     ^^^
> That claims the array is zero terminated

*blush* Don't believe a word I'm saying, I should actually read the value too, not just the key.

So yes, this method is indeed a good example, sorry.

Created attachment 212419 [details] [review]
Fix len_arg_index for array arguments

Don't set len_arg_index for arrays without the length annotation given.
This fixes methods like Clutter.Texture.set_from_rgb_data() and
Clutter.Image.set_data()

Created attachment 212420 [details] [review]
tests: Add another array marshalling test

A test case with an array in-argument that is neither zero terminated,
nor fixed length, nor blessed with a length argument.

This is, for example, the case in clutter_texture_set_from_rgb_data()

Perfect, thank you! Looks good, both patches committed.