[gst-devel] First video overlay interface proposal

[Ronald Bultje]

Hi,

comments on their way. ;).

On Thu, 2003-10-16 at 00:19, Julien MOUTTE wrote:
[..]

> struct _GstVideoOverlayResource
> {
>   gint resource_type;

Could you typedef this?

typedef enum {
  GST_VIDEO_OVERLAY_RESOURCE_TYPE_XID,
  GST_VIDEO_OVERLAY_RESOURCE_TYPE_DIRECTFB,
} GstVideoOverlayResourceType;

Or so... But then with shorter names. ;).

>   union
>   {
>     struct
>     {
>       glong xid;
>     } x_resource;

Look at X.h, this is defined as a gulong or a CARD32 (unsigned int)
depending on the platform (32/64 bit). I'd prefer if we just defined the
type as XID and let #include X11/X.h figure out the rest. From the
comments, I understand that (by protocol) XID must be unsigned and 32
bit, so you probably want to make it a guint32 instead of a glong.

>     struct
>     {
>       gpointer display_layer;
>     } dfb_resource;
>   } resource_data;
> };

You know more about dfb than I do, so I suppose this is fine. ;).

> struct _GstVideoOverlay {
> };

You need to define that the struct exists without filling contents, so
"struct _GstVideoOverlay;", without the brackets.

> struct GstVideoOverlayIface {
>   GTypeInterface parent_class;
>   
>   /* public virtual methods */
>   void (*set_video_out) (GstVideoOverlay *overlay,
>                          GstVideoOverlayResource *video_out);

Hm... Naming... The name video_out is "sort of" weird, because it gives
me (personally) the feeling that this is suited primarily for output
sinks. I'd prefer a name 'set_video_overlay ()' or so. This is minor
nagging, but ohwell...

>   void (*push_ui_event) (GstVideoOverlay *overlay, GstEvent *event);

I agree with Dave here. This shouldn't be part of the overlay interface.

>   void (*set_geometry)  (GstVideoOverlay *overlay, gint width, gint height);
>   void (*get_geometry)  (GstVideoOverlay *overlay, gint *width, gint *height);

What does this do? And why do we need this? In the case of X, if I give
an XID to the plugin, then the plugin can figure everything out from the
plugin. Or is this meant for cases where you want to change size when a
new video has been loaded and you want to rescale the window so that it
will have the size of the new video? In that case, isn't it easier to
set the size of your widget, which changes the size of its GdkWindow,
which triggers a size change in the plugin automatically (since it knows
the XID)? I'm not sure how this works for dFB, I'm just trying to figure
out why we would need each such function. ;).

>   /* signals */
>   void (*have_video_out)  (GstVideoOverlay *overlay,
>                            GstVideoOverlayResource *video_out);

Here, you probably want to describe the order of signal/functions. Do
you need to call set_video_out () before or after this signal has been
fired? Or are the two related in another way (mutually exclusive or so).
Or are they not related at all?

>   void (*have_size)       (GstVideoOverlay *overlay, gint width, gint height);
>   void (*frame_displayed) (GstVideoOverlay *overlay);

I'd add an comment that this one (frame_displayed) is optional. At least
for v4l (I think for dxr3, too), you don't get signals when the overlay
received the next frame, so you can never trigger this signal. I.e., it
can be here, but assure that its optional and informational, not
compulsary.

> };

Good work! Thanks,

Ronald

-- 
Ronald Bultje <rbultje at ronald.bitfreak.net>
Linux Video/Multimedia developer