Created attachment 331770 [details] [review]
player: add API to change http user agent

Note that the unit test is racy atm (see FIXME); not sure of the proper way to test this.

Created attachment 331784 [details] [review]
player: add API to change http user agent

Review of attachment 331784 [details] [review]:

::: gst-libs/gst/player/gstplayer.h
@@ +195,3 @@
+const gchar * gst_player_get_http_user_agent          (GstPlayer    * player);
+void          gst_player_set_http_user_agent          (GstPlayer    * player,
+                                                       const gchar *agent);

This is not only for HTTP but also for RTSP and probably others.

I think it would also be nice to not add a million such configuration functions, but maybe create some kind of "configuration object" instead (could be based on a GstStructure like the GstBufferPoolConfig for example) for the things that you configure once and don't change during playback.

::: tests/check/libs/player.c
@@ +1649,3 @@
+  src = gst_bin_get_by_interface (GST_BIN (pipeline), GST_TYPE_URI_HANDLER);
+  /* FIXME: this test is racy: the souphttpsrc element may be destroyed before
+   * the error is reported from the player. */

You could connect to source-setup of the pipeline instead maybe

(In reply to Sebastian Dröge (slomo) from comment #4)

> I think it would also be nice to not add a million such configuration
> functions, but maybe create some kind of "configuration object" instead
> (could be based on a GstStructure like the GstBufferPoolConfig for example)
> for the things that you configure once and don't change during playback.

Other such configurations I'm thinking of would be for example the latency property on rtspsrc, cookies, username/password

(In reply to Sebastian Dröge (slomo) from comment #4)
> Review of attachment 331784 [details] [review] [review]:
> 
> ::: gst-libs/gst/player/gstplayer.h
> @@ +195,3 @@
> +const gchar * gst_player_get_http_user_agent          (GstPlayer    *
> player);
> +void          gst_player_set_http_user_agent          (GstPlayer    *
> player,
> +                                                       const gchar *agent);
> 
> This is not only for HTTP but also for RTSP and probably others.
> 
> I think it would also be nice to not add a million such configuration
> functions, but maybe create some kind of "configuration object" instead
> (could be based on a GstStructure like the GstBufferPoolConfig for example)
> for the things that you configure once and don't change during playback.

Makes sense yeah.

Something like this then?

void gst_player_config_set_user_agent (GstStructure *config, const gchar *user_name);
const gchar * gst_player_config_get_user_agent (GstStructure *config);

void gst_player_set_config (GstPlayer *player, GstStructure *config);

Or something every more generic based on key/value? That would save us from adding API for each new option but maybe kinda defeat the purpose of GstPlayer of being a very high level easy API.

> ::: tests/check/libs/player.c
> @@ +1649,3 @@
> +  src = gst_bin_get_by_interface (GST_BIN (pipeline), GST_TYPE_URI_HANDLER);
> +  /* FIXME: this test is racy: the souphttpsrc element may be destroyed
> before
> +   * the error is reported from the player. */
> 
> You could connect to source-setup of the pipeline instead maybe

Good idea, that seems to work. Thanks!

(In reply to Guillaume Desmottes from comment #6)
>  
> Something like this then?
> 
> void gst_player_config_set_user_agent (GstStructure *config, const gchar
> *user_name);
> const gchar * gst_player_config_get_user_agent (GstStructure *config);
> 
> void gst_player_set_config (GstPlayer *player, GstStructure *config);
> [...]

That's exactly what I was thinking of, yes. Just also add a get_config(). Basically mirror the buffer pool configuration API ;)

Setting the config should only be allowed when the player is stopped.

Created attachment 332008 [details] [review]
player: add API to change http user agent

Introducing a new 'config' API similar to GstBufferPoolConfig.

Review of attachment 332008 [details] [review]:

::: gst-libs/gst/player/gstplayer.c
@@ +2540,3 @@
+  if (user_agent) {
+    if (g_object_class_find_property (G_OBJECT_GET_CLASS (source),
+            "user-agent")) {

You should check if it actually takes a string :)

@@ +4141,3 @@
+ *
+ * Returns: %TRUE when the configuration could be set.
+ * Since 1.12

Since: with colon, and this seems like it will go into 1.10 ;)

(In reply to Sebastian Dröge (slomo) from comment #9)
> Review of attachment 332008 [details] [review] [review]:
> 
> ::: gst-libs/gst/player/gstplayer.c
> @@ +2540,3 @@
> +  if (user_agent) {
> +    if (g_object_class_find_property (G_OBJECT_GET_CLASS (source),
> +            "user-agent")) {
> 
> You should check if it actually takes a string :)

Indeed. Done.

> @@ +4141,3 @@
> + *
> + * Returns: %TRUE when the configuration could be set.
> + * Since 1.12
> 
> Since: with colon, and this seems like it will go into 1.10 ;)

ooops; fixed.

Created attachment 332076 [details] [review]
player: add API to change http user agent

Introducing a new 'config' API similar to GstBufferPoolConfig.

Attachment 332076 [details] pushed as 6e39cef - player: add API to change http user agent