Re: Gnome Sound API (urgent)

[Owen Taylor <otaylor redhat com>]

Martin Baulig <martin home-of-linux org> writes:

> Hi Christian,
> 
> can you please have a short look at the new GNOME Sound API which
> I just committed in libgnome/gnome-sound.h ?

Looks like the API there is a bit different from what you
have here, you've added:

 void gnome_sound_cache_add_sample (GnomeSoundSample *sample, GError **error);

 void gnome_sound_cache_remove_sample (GnomeSoundSample *sample, GError **error);

 GnomeSoundSample *gnome_sound_sample_new_from_cache (const char *name,
                                                      GError **error);

Could you explain how the caching works? Do things get added
to the cache by default? Does new_from_cache() return NULL
if the sample isn't already in the cache?
 
> Unfortunately, this stuff is a little bit urgent since I want to
> make the libgnome release tonight. We can still change the API
> later on if absolutely necessary, but we should try hard to do it
> correctly in the the first place.
> 
> Below is what's currently in gnome-sound.h - if you want to use
> GStreamer as backend or at least have support for this, we need
> to make sure that the `GnomeSoundPlugin' struct suits your needs.

Hmm, do we have API docs for this anywhere? I don't see
them in CVS. Generally, when posting API for review, it's
nice to post the API docs along with the headers. 
 
> =====
> typedef struct _GnomeSoundSample GnomeSoundSample;
> typedef struct _GnomeSoundPlugin GnomeSoundPlugin;
> 
> struct _GnomeSoundPlugin {
>     gulong version;
>     const gchar *name;
> 
>     gboolean (*init) (const gchar *backend_args,
> 		      GError **error);
>     void (*shutdown) (GError **error);
> 
>     void (*play_file) (const gchar *filename,
> 		       GError **error);
> 
>     GnomeSoundSample * (*sample_new) (const gchar *name,
> 				      GError **error);
>     int (*sample_write) (GnomeSoundSample *gs,
> 			 guint n_bytes, gpointer bytes,
> 			 GError **error);
>     void (*sample_write_done) (GnomeSoundSample *gs,
> 			       GError **error);
> 
>     GnomeSoundSample * (*sample_new_from_file) (const gchar *filename,
> 						GError **error);
>     void (*sample_play) (GnomeSoundSample *gs,
> 			 GError **error);
>     gboolean (*sample_is_playing) (GnomeSoundSample *gs,
> 				   GError **error);
>     void (*sample_stop) (GnomeSoundSample *gs,
> 			 GError **error);
>     void (*sample_wait_done) (GnomeSoundSample *gs,
> 			      GError **error);
>     void (*sample_release) (GnomeSoundSample *gs,
> 			    GError **error);
> };
> 
> typedef enum
> {
>     /* no sound driver */
>     GNOME_SOUND_ERROR_NODRIVER,
>     /* not implemented */
>     GNOME_SOUND_ERROR_NOT_IMPLEMENTED,
>     /* device busy */
>     GNOME_SOUND_ERROR_DEVICE_BUSY,
>     /* I/O eror */
>     GNOME_SOUND_ERROR_IO,
>     /* Insufficient permissions */
>     GNOME_SOUND_ERROR_PERMS,
>     /* misc error */
>     GNOME_SOUND_ERROR_MISC
> } GnomeSoundError;
> 
> #define GNOME_SOUND_ERROR gnome_sound_error_quark ()
> GQuark gnome_sound_error_quark (void) G_GNUC_CONST;
> 
> void gnome_sound_init (const gchar *driver_name,
> 		       const gchar *backend_args,
> 		       const GnomeSoundPlugin *opt_plugin,
> 		       GError **error);

Who calls this? How would the app know what driver to use?

What's the purpose of providing either the plugin directly
or via a module?

Functions that have a GError **error, should generally
have a boolean return corresponding to whether error
was set for convenience (though if they return something
with no obvious invalid value, then the *error != NULL
can be the only error indication) 

====
        module = g_module_open (module_name, G_MODULE_BIND_LAZY);
        if (!module) {
            g_warning (G_STRLOC ": Can't open sound plugin `%s': %s",
                       name, g_module_error ());
            return;
        }

        if (!g_module_symbol (module, "gnome_sound_plugin", &plugin_ptr)) {
            g_warning (G_STRLOC ": Not a valid sound plugin: `%s'", name);
            g_module_close (module);
            return;
        }
====

I don't really understand the distinction here between when
you set *error and when you g_warning().
 
> void gnome_sound_shutdown (GError **error);

Shouldn't calling init when there is already a sound plugin
either shut down the current plugin or warn? 
 
> void gnome_sound_play (const char *filename,
> 		       GError **error);

Could/Should this be implemented completely in the wrapper in
terms GnomeSoundSample?
 
> GnomeSoundSample *
> gnome_sound_sample_new_from_file (const char *filename,
> 				  GError **error);

I don't like the fact that if an app holds a
GnomeSoundSample across gnome_sound_shutdown() 
gnome_sound_init(), we die a horrible death without
a warning message.

Also, GnomeSoundSample really needs to be a GObject for
this to be a language-bindable API. Basically there
are three possible memory management scenarios:

 - GObject
 - Mutable, copy-by-value
 - Immutable, reference counted

I really think you probably should be using the GModule
API for dynamically loaded types to handle keeping
the module open while it has GnomeSoundSample objects
around.

You can look at gtkimmodule.c and gtkthemes.c for examples.
 
> GnomeSoundSample *
> gnome_sound_sample_new (const char *sample_name,
> 			GError **error);

What's the purpose of the name?
Do they have to be unique within a process?
Are they used to share samples across processes?
Can I create an anonymous sample by passing NULL?
  (If not, people are going to be create ad-hoc UUID's
   at times.)
 
> int
> gnome_sound_sample_write (GnomeSoundSample *gs,
> 			  guint n_bytes, gpointer bytes,
> 			  GError **error);
>
> void
> gnome_sound_sample_write_done (GnomeSoundSample *gs,
> 			       GError **error);

Is it an error if I call play() before calling
write? Can I write() to a sample loaded with
new_file()? Can I write() after calling write_done()?
 
Looks like the implementation doens't have an
g_return_if_fail() in it.... the more error trapping
you put in the wrapper layer, the easier it is for
people to write robust plugins.

[ Question: since when did we go to 4-character BSD for gnome-libs?
  I thought gnome-libs was supposed to be linux-kernel style, though
  there were random influxes of gtk+ style ] 

Just my thoughts looking over the API and the C file.

I'd feel a lot better about putting this in at this point
if we had an implementation of this to see how it worked.... 

Regards,
                                        Owen