Re: RFC: Operation Options API proposal

[Guillaume Emont <gemont igalia com>]

Hi!

Thanks for your answers, I replied in-line.

Guij

On 24/03/2011 10:33, Iago Toral wrote:
> 
> On Wed, 23 Mar 2011 14:50:32 +0100, Guillaume Emont <gemont igalia com>
> wrote:
> (...)
>> Capabilities
>> ------------
>>
>> The consensus seems to be reached on the necessity of a separate GrlCaps
>> type,
>> that would handle the capabilities of a Source.
>>
>> I would see an API similar to GrlData, but without having GrlCaps
>> being an
>> object.
>>
>> ::
>>
>>   /* setters _new()/_free are for sources to use, getters are for
>>    * applications */
>>
>>   GrlCaps *grl_caps_new (void);
>>   void grl_caps_free (GrlCaps *caps);
>>
>>   /* Using GValue here for consistency with GrlData, but I wonder
>> whether a
>>    * simple gpointer wouldn't be enough */
>>   void grl_caps_set_value (GrlCaps *caps, const gchar *name, GValue
>> *value);
>>   GValue *grl_caps_get_value (GrlCaps *caps, const gchar *name);
> 
> If we have get/set_* then I think we do not really need a get/set_value
> and could go with a get/set_data that would receive and return a
> pointer. BTW, out of curiosity, do we have an example caps that could
> need this GValue/pointer API?

I don't think that any specific cap would need that. My (vague)
understanding is that using GValues may ease the work of binding
writers, but not too sure. Is there a binding writer in the room?

Also, to ensure it's clear, I think the idea of a get/set_value or
get/set_data is complementary with custom setters/getters, as these
would be implemented using the generic getter/setter.

Also, the generic getter/setter could be useful if people want to add
some custom caps that only their own plugin and application would need
to know (without having to modify core)
> 
>>   /* nice helpers for the main types */
>>   void grl_caps_set_string (GrlCaps *caps, const gchar *name, gchar
>> *value);
>>   gchar *grl_caps_get_string (GrlCaps *caps, const gchar *name);
>>
>>   void grl_caps_set_boolean (GrlCaps *caps, const gchar *name, gboolean
>> value);
>>   gboolean grl_caps_get_boolean (GrlCaps *caps, const gchar *name);
>>
>>   void grl_caps_set_int (GrlCaps *caps, const gchar *name, gint value);
>>   gint grl_caps_get_int (GrlCaps *caps, const gchar *name);
>>
>>   void grl_caps_set_float (GrlCaps *caps, const gchar *name, gfloat
>> value);
>>   gfloat grl_caps_get_float (GrlCaps *caps, const gchar *name);
> 
> The caps I have on mind right now are things like filter types, sorting
> options, operation flags, ... I wonder how the API would be used, for
> example, to inform the user that a certain filter is supported.

In this model, that's up to the people who add new caps to define and
document their exact meaning (e.g. in the documentation of the
setter/getter they would add).
e.g. for filter types you would have
typedef enum {
  GRL_FILTER_TYPE_NONE = 0,
  GRL_FILTER_TYPE_BOX = 1 << 0,
  GRL_FILTER_TYPE_AUDIO = 1 << 1,
  GRL_FILTER_TYPE_VIDEO = 1 << 2,
  GRL_FILTER_TYPE_PICTURE = 1 << 3,
} GrlFilterType

  /* Types on which one can filter */
  GrlFilterType grl_caps_get_filter_type (GrlCaps *caps);

That could allow a user to know whether this type is supported.

The implementer of a new caps would also need to add some code in
GrlOptions to add the corresponding options. In that code, he would
check that the caps agree with it, so that the user could do:
  options = grl_operation_options_new (caps);
  if (grl_operation_options_filter_on_type (options,
GRL_FILTER_TYPE_VIDEO)) {
   // ....
  }

> 
>>   /* nice wrappers like: */
>>   void grl_caps_set_frobuz (GrlCaps *caps, GrlFrobuz *frobuz);
>>   GrlFrobuz *grl_caps_get_frobuz (GrlCaps *caps);
>>
>>   /* wrappers we already want */
>>
>>   /* whether source supports pagination (skip & count)
>>   void grl_caps_set_pagination (GrlCaps *caps, gboolean pagination);
>>   gboolean grl_caps_get_pagination (GrlCaps *caps);
>>   /* flags the source can honour. Only GRL_RESOLVE_FAST_ONLY makes
>> sense, since
>>    * the other flags are entirely handled by core */
>>   void grl_caps_set_flags (GrlCaps *caps, GrlMetadataResolutionFlags
>> flags);
>>   GrlMetadataResolutionFlags grl_caps_get_falgs (GrlCaps *caps);
>>
>>
>> Operation Options
>> -----------------
>>
>>
>> ::
>>
>>   /* new/free and setters is for programs, getters are for sources.
>>    */
>>
>>   GrlOperationOptions *grl_operation_options_new (GrlCaps *caps);
>>   void grl_operation_options_free (GrlOperationOptions *options);
>>
>>   /* Return TRUE if options could be set, FALSE if that was not allowed
>> by caps
>>    */
>>   gboolean grl_operation_options_set_value (GrlOperationOptions *options,
>>                                             const gchar *name,
>>                                             GValue *value);
>>   GValue grl_operation_options_get_value (GrlOperationOptions *options,
>>                                           const gchar *name);
>>
>>   /* nice helpers for the main types */
>>   gboolean grl_operation_options_set_string (GrlOperationOptions
>> *options,
>>                                              const gchar *name,
>>                                              gchar *value);
>>   gchar *grl_operation_options_get_string (GrlOperationOptions *options,
>>                                            const gchar *name);
>>
>>   gboolean grl_operation_options_set_boolean (GrlOperationOptions
>> *options,
>>                                               const gchar *name,
>>                                               gboolean value);
>>   gboolean grl_operation_options_get_boolean (GrlOperationOptions
>> *options,
>>                                               const gchar *name);
>>
>>   gboolean grl_operation_options_set_int (GrlOperationOptions *options,
>>                                           const gchar *name,
>>                                           gint value);
>>   gint grl_operation_options_get_int (GrlOperationOptions *options,
>>                                       const gchar *name);
>>
>>   gboolean grl_operation_options_set_float (GrlOperationOptions *options,
>>                                             const gchar *name,
>>                                             gfloat value);
>>   gfloat grl_operation_options_get_float (GrlOperationOptions *options,
>>                                           const gchar *name);
>>
>>   /* nice wrappers like: */
>>   gboolean grl_operation_options_set_frobuz (GrlOperationOptions
>> *options,
>>                                              GrlFrobuz *frobuz);
>>   GrlFrobuz *grl_operation_options_get_frobuz (GrlOperationOptions
>> *options);
>>
>>   /* wrappers we already want */
>>
>>   /* Note that count is signed, so that it can cleanly be set to -1 to
>> mean "no
>>    * limit" */
>>   gboolean grl_operation_options_set_pagination (GrlOperationOptions
>> *options,
>>                                                  guint skip, gint count);
>>   void grl_operation_options_get_pagination (GrlOperationOptions
>> *options,
>>                                              guint *skip, gint *count);
>>
>>   void grl_operation_options_set_flags (GrlOperationOptions *options,
>>                                         GrlMetadataResolutionFlags
>> flags);
>>   GrlMetadataResolutionFlags
>>   grl_operation_options_get_falgs (GrlOperationOptions *options);
>>
>> :Comments: We might want reference counting on GrlOperationOptions, so
>> that
>>            the application can choose between dropping its reference
>>            immediately (and not have to free the options when the
>> operation has
>>            terminated) or keep it to use the same GrlOperationOptions
>>            instance for several operations.
> 
> That's a good point, but in that case should we not just go with
> GObject? If we actually decide that GrlCaps and GrlOptions should have a
> common ancestor as well, then that sounds like another reason advising
> the use of objects.
We could. I don't know if we care about the memory overhead. If we're OK
with a GObject, I think that we could inherit from GrlData that already
provides a good part of what we need.
> 
>> ====================
>> Existing API changes
>> ====================
>>
>>
>> API modifications for media source
>> ----------------------------------
>>
>> Caps::
>>
>>   GrlCaps *grl_media_source_get_caps (GrlMediaSource *source,
>>                                       GrlSupportedOps operation);
>>
>> Use::
>>
>>   guint grl_media_source_browse (GrlMediaSource *source,
>>                                  GrlMedia *container,
>>                                  const GList *keys,
>>                                  GrlOperationOptions *options,
>>                                  GrlMediaSourceResultCb callback,
>>                                  gpointer user_data);
>>
>>   /* And the same thing for the other operations. */
>>
>> First idea to help the user adjust their options to what the source
>> supports::
>>
>>   gboolean
>>   grl_media_source_supported_options (GrlMediaSource *source,
>>                                       GrlSupportedOps operation,
>>                                       GrlOperationOptions *options,
>>                                       GrlOperationOptions
>> **effective_options);
> 
> I like this better.

Actually, that's a remain from the old document that wasn't supposed to
be there ;). With caps, I guess it would be more logical to have:
  gboolean
  grl_caps_fixate_options (GrlCaps *caps,
                           static GrlOperationOptions *options,
                           GrlOperationOptions **new_options);
  With a way to create options without caps (grl_operation_options_new
(GrlCaps *caps) would accept NULL as a parameter)


> 
>> Alternative idea, similar to GstCaps in gstreamer, simpler to
>> implement for
>> plugins, slightly less flexible::
>>
>>   GrlOperationOptions *
>>   grl_media_source_supported_options (GrlMediaSource *source,
>>                                       GrlSupportedOps operation);
>>
>>   GrlOperationOptions *
>>   grl_operation_options_intersect (GrlOperationOptions *options1,
>>                                    GrlOperationOptions *options2);
>>
>>
>> ===================
>> Remaining Questions
>> ===================
>>
>> Supposing we all agree on the above (I doubt we will without at least
>> some
>> slight modifications), some questions are still to be answered:
>>
>>  - Should we use options for metadata as well, or wait until we rework
>> the class hierarchy?
> 
> I think we need options for metadata, since we have operation flags
> there already IIRC.

Ok. The reason I ask that question is because that could mean a lot of
code duplication.
An alternative would be to implement most of the stuff only in
GrlMetadataSource, since GrlMediaSource currently inherits from it.

> 
>>  - Should we have an externally visible common ancestor to
>> GrlOperationOptions
>>    and GrlCaps? (one that would implement the generic setters and
>> getters)
> 
> They are both semantically similar, so I think from an implementation
> POV having a common ancestor makes sense. Then again, from the user's
> point of view they are still different things in the end, so I am not so
> sure if making that externally visible makes sense.
> 
Well, if we go with GObject, I think we need to have that visible.

>>  - Should we use GValue or gpointer?
>>
>> Please tell me if there are other aspects that I forgot.
> 
> GValue would make things more complicated for the user with no gain, I'd
> go with gpointer.

As I said above, GValue might be useful for bindings, but I'm not too
sure, I'd love to have some comments from somebody who understands that
better than I do (maybe you?). Also, if we inherit from GrlData, it's
already there
> 
> I think the proposal is good. I would start the implementation and then,
> if we find issues we may need to adjust some bits.
Before starting, I think we need to ensure we agree on a few more points:
 - inherit from GrlData? (I'd say yes)
 - put all the stuff in GrlMetadataSource (and use grl_metadata_source_*
methods on media sources as well)? (I'd say yes as well, and we need to
plan quickly when to fix that class hierarchy)