Created attachment 360391 [details] [review]
Make GstVideoInfo an opaque class structure

So that we generate a ABI compatible structure and make it working.
Also this is the way we do it for almost all other structures of this
kind.

I first tried to keep having a managed structure that properly matches
the C structure ABI generating in the gtk-sharp generator Explicit structure
for the union as we do with non managed structures but this is not
possible and leads to the following assertion in mono:

    Type Gst.Video.VideoInfo/__ABI which has an [ExplicitLayout] attribute cannot have a reference field at the same offset as another field.

This sensibly changes the API but I bet noone will even notice.

Created attachment 360392 [details] [review]
Fix problems with GstVideoInfo

The ABI union member was missing from the C# representation of
GstVideoInfo, which means the size of the C# struct was 32 bytes
smaller than the actual struct. This caused heap corruption when
e.g. calling VideoInfo.Init(), since the binding uses AllocHGlobal to
allocate temporary storage for the GstVideoInfo struct (but only
allocates a number of bytes equal to the size of the C# struct), and
then passes a pointer to the allocated memory to gst_video_info_*()
functions. When gst_video_info_init() is called, it does a memset with
sizeof(GstVideoInfo), which results in an out-of-bounds write.

With this change, padding bytes equal to the size of the ABI union are
added to the generated C# struct.

Also suppressed the auto-generated .Equals() method, and instead
implemented it to call IsEqual() (gst_video_info_is_equal).

Erlend, I changed sensibly your patch concerning the ABI incompatibility issue as it is not really correct as the size of the fields is not the same as the size of the union and if we add more field in the future, we won't be ABI compatible again.

My approach is to have it as an GLib.Opaque object instead of a managed structure, you can find the details in the commit message.

Please, let me know if that approach is good to you.

Created attachment 360440 [details] [review]
VideoInfo: Override Equals() method.

Thibault: looks good! Had to change the patch for Equals(), since with opaque=true, the generated code is now a class and not a struct.

I was wondering if it'd make sense to make GstAudioInfo opaque as well, just for the sake of having similar APIs for the two similar structures? (Before the opaque-change, the VideoInfo had a Zero attribute that could be used to create a zero-initialized struct instance, whereas after, a VideoInfo _must_ be constructed as 'new VideoInfo()'. And the AudioInfo has the same Zero attribute, which is even used in one of the code examples in the samples/ folder).

In addition, I have a question regarding the code that is generated by the GAPI codegen for the opaque structure. I noticed that the gtk-sharp gapi-codegen utility was recently changed to add a `GetFieldOffset()` method. In the case of VideoInfo, the relevant code looks like this:

	public  unsafe uint GetFieldOffset(string field) {
		return (uint) (instance_offset + (uint) Marshal.OffsetOf(typeof(Gst.Video.VideoInfo._GstVideoInfoABI), field));
	}

	[StructLayout (LayoutKind.Sequential)]
	public struct _GstVideoInfoABI {
		private IntPtr _finfo;

		private Gst.Video.VideoInterlaceMode interlace_mode;
		private Gst.Video.VideoFlags flags;
		private int width;
		private int height;
		private UIntPtr size;
		private int views;
		private Gst.Video.VideoChromaSite chroma_site;
		private Gst.Video.VideoColorimetry colorimetry;
		private int par_n;
		private int par_d;
		private int fps_n;
		private int fps_d;
		[MarshalAs (UnmanagedType.ByValArray, SizeConst=4)]
		private ulong[] Offset;
		[MarshalAs (UnmanagedType.ByValArray, SizeConst=4)]
		private int[] Stride;
		[StructLayout(LayoutKind.Explicit)]
		struct __ABI {
			struct __abi{
				private Gst.Video.VideoMultiviewMode multiview_mode;
				private Gst.Video.VideoMultiviewFlags multiview_flags;
				private Gst.Video.VideoFieldOrder field_order;
			}
			[FieldOffset(0)]
			private __abi abi;
			[FieldOffset(0)]
			[MarshalAs (UnmanagedType.ByValArray, SizeConst=4)]
			private IntPtr[] _gstGstReserved;
		}
		private __ABI ABI;
	}

However, the `GetFieldOffset()` method isn't used anywhere that I can see, though. So I was wondering if this is work-in-progress for replacing the use of the glue DLL to get the structure offsets? :-)

- Thanks for you updated patch.
- I agree, AudioInfo should become "opaque", attaching a patch now
- GetFieldOffset was introduced to remove the need of the glue, but it is already used all around when building with meson, were I totally dropped the glue generation (you should use meson, I am gonna remove autotools build soonish I think).

Created attachment 360446 [details] [review]
Make GstVideo/AudioInfo GLib.Opaque structures

So that we generate a ABI compatible structure and make it working.
Also this is the way we do it for almost all other structures of this
kind.

I first tried to keep having a managed structure that properly matches
the C structure ABI generating in the gtk-sharp generator Explicit structure
for the union as we do with non managed structures but this is not
possible and leads to the following assertion in mono:

    Type Gst.Video.VideoInfo/__ABI which has an [ExplicitLayout] attribute cannot have a reference field at the same offset as another field.

This sensibly changes the API but I bet noone will even notice.

This has been pushed and many other fixes have landed around ABI handling.

commit b5beac1217ab86d7c5600d2b4653758ed7b04074
Author: Thibault Saunier <thibault.saunier@osg.samsung.com>
Date:   Mon Sep 25 18:40:29 2017 -0300

    Make GstVideo/AudioInfo GLib.Opaque structures
    
    So that we generate a ABI compatible structure and make it working.
    Also this is the way we do it for almost all other structures of this
    kind.
    
    I first tried to keep having a managed structure that properly matches
    the C structure ABI generating in the gtk-sharp generator Explicit structure
    for the union as we do with non managed structures but this is not
    possible and leads to the following assertion in mono:
    
        Type Gst.Video.VideoInfo/__ABI which has an [ExplicitLayout] attribute cannot have a reference field at the same offset as another field.
    
    This sensibly changes the API but I bet noone will even notice.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=788133