Yes, on 64 bit Windows a different type should be used for that function. It's impossible that it works or worked with a gulong and I think we can still change it for Win64-only. Any other oppinions?

But I don't think this is the only place where Win64 will have problems, I could imagine that many people assumed sizeof(int)<=sizeof(long)==sizeof(void *), which simply isn't true on Win64 unfortunately.

Created attachment 169527 [details] [review]
patch

Deprecate old gulong functions, add new uintptr functions, and fix up all the examples and tests.

Annoyingly, there's no G_TYPE_UINTPTR, so I used G_TYPE_GUINT64.

This should be backward compatible.

Comment on attachment 169527 [details] [review]
patch

Yes, looks good.

Doesn't really make things much prettier :-/

- can't we just #define _set_xwindow_id to
  _set_xwindow_id_uintptr? The new function
  name isn't exactly pretty?

- should we use this opportunity to make
  the function name more generic (so that
  the only X reference is in the interface
  name)? e.g. _set_window_handle() or
  somesuch?

- maybe add per-platform defines for
  easier discoverability? (possibly with
  dummy typedefs for id types if right
  headers haven't been included)

- some API: keywords in the commit
  message would be nice, makes it
  easier to compile release notes

Created attachment 169888 [details] [review]
updated patch

Updated patch.

I agree about the per-platform defines, although that can be added later.

Comment on attachment 169888 [details] [review]
updated patch

>@@ -353,8 +376,19 @@ gst_x_overlay_set_xwindow_id (GstXOverlay * overlay, gulong xwindow_id)
> 
>   klass = GST_X_OVERLAY_GET_CLASS (overlay);
> 
>-  if (klass->set_xwindow_id) {
>-    klass->set_xwindow_id (overlay, xwindow_id);
>+  if (klass->set_window_handle) {
>+    klass->set_window_handle (overlay, handle);
>+  } else {
>+#ifndef GST_REMOVE_DEPRECATED
>+#ifdef GST_DISABLE_DEPRECATED
>+#define set_xwindow_id set_xwindow_id_disabled
>+#endif
>+    if (sizeof (guintptr) <= sizeof (gulong) && klass->set_xwindow_id) {
>+      klass->set_xwindow_id (overlay, handle);
>+    } else {
>+      g_warning ("Refusing to cast guintptr to smaller gulong");
>+    }
>+#endif
>   }
> }

I wonder if the size check should be skipped here if the actual handle value to be set fits into a gulong, to preserve maximum backwards compatibility (but maybe this only affects win64 anyway which was broken before so we don't really care too much?)?


>@@ -366,19 +400,43 @@ gst_x_overlay_set_xwindow_id (GstXOverlay * overlay, gulong xwindow_id)
>  * This will post a "have-xwindow-id" element message on the bus.
>  *
>  * This function should only be used by video overlay plugin developers.
>+ *
>+ * Deprecated: Use gst_x_overlay_got_xwindow_id_intptr() instead.
>  */
>+#ifndef GST_REMOVE_DEPRECATED
>+#ifdef GST_DISABLE_DEPRECATED
>+void gst_x_overlay_got_xwindow_id (GstXOverlay * overlay, gulong xwindow_id);
>+#endif
> void
> gst_x_overlay_got_xwindow_id (GstXOverlay * overlay, gulong xwindow_id)
> {
>+  gst_x_overlay_got_xwindow_id (overlay, xwindow_id);
>+}
>+#endif
>+
>+/**
>+ * gst_x_overlay_got_window_handle:
>+ * @overlay: a #GstXOverlay which got a window
>+ * @handle: a platform-specific handle referencing the window
>+ *
>+ * This will post a "have-xwindow-id" element message on the bus.
>+ *
>+ * This function should only be used by video overlay plugin developers.
>+ */
>+void
>+gst_x_overlay_got_window_handle (GstXOverlay * overlay, guintptr handle)
>+{
>   GstStructure *s;
>   GstMessage *msg;
> 
>   g_return_if_fail (overlay != NULL);
>   g_return_if_fail (GST_IS_X_OVERLAY (overlay));
> 
>-  GST_LOG_OBJECT (GST_OBJECT (overlay), "xwindow_id = %lu", xwindow_id);
>-  s = gst_structure_new ("have-xwindow-id", "xwindow-id", G_TYPE_ULONG,
>-      xwindow_id, NULL);
>+  GST_LOG_OBJECT (GST_OBJECT (overlay), "xwindow_id = %" G_GUINTPTR_FORMAT,
>+      handle);
>+  s = gst_structure_new ("have-xwindow-id",
>+      "xwindow-id", G_TYPE_ULONG, (unsigned long) handle,
>+      "xwindow-id-uintptr", G_TYPE_UINT64, (guint64) handle, NULL);

Should we rename this field to match the new function names? (We might also want to provide API to detect this kind of message and parse the info later).

Looks good to me otherwise. Might want to sprinkle in some more GST_WARNINGs if elements don't implement the new vfunc yet or call the old methods.

(In reply to comment #6)
> I wonder if the size check should be skipped here if the actual handle value to
> be set fits into a gulong, to preserve maximum backwards compatibility (but
> maybe this only affects win64 anyway which was broken before so we don't really
> care too much?)?

My thought was that only Win64 is affected, and presumably anything on Windows will get fixed quickly.

> Should we rename this field to match the new function names? (We might also
> want to provide API to detect this kind of message and parse the info later).

Definitely change -uintptr.  Changing the structure name seems like not much benefit.

> Looks good to me otherwise. Might want to sprinkle in some more GST_WARNINGs if
> elements don't implement the new vfunc yet or call the old methods.

Good idea.  Will fix up and push shortly.

commit 6dc02137fb8e952c79479f0c1154831c1b761410
Author: David Schleef <ds@schleef.org>
Date:   Sun Sep 5 15:17:47 2010 -0700

    xoverlay: Add guintptr versions of functions
    
    And deprecate the gulong versions.  This is to support platforms
    where sizeof(unsigned long) < sizeof(void *).  Fixes #627565.
    
    API: Add gst_x_overlay_set_window_handle()
    API: Deprecate: gst_x_overlay_set_xwindow_id()
    API: Add gst_x_overlay_got_window_handle()
    API: Deprecate: gst_x_overlay_got_xwindow_id()
    API: Add GstXOverlay::set_window_handle()
    API: Deprecate: GstXOverlay::set_xwindow_id()

plugins-gl up to date as well

commit 6ae0117c3dce8b23e302eb7592be42a19d41e9fd
Author: Stefan Kost <ensonic@users.sf.net>
Date:   Thu Sep 16 15:00:29 2010 +0300

    xoverlay: require base from git and update to new API