Metadata handling: incorrect setMeta method

[delthas]

Hello,

I believe the setMeta method you have added is incorrect. The code is:

public void setMeta(String key, String value) throws IOException {
	long instanceMeta = shout_metadata_new();
	if (shout_set_metadata(this.instance, instanceMeta) != SUCCESS) {
		throw new IOException(shout_get_error(this.instance));
	}
	if (shout_metadata_add(instanceMeta, key, value) != SUCCESS) {
		throw new IOException(shout_get_error(this.instance));
	}
}

First, the shout metadata instance should be freed manually according to the docs, with shout_metadata_free:

shout_metadata_t *shout_metadata_new();
Allocates a new metadata structure, or returns NULL if no memory is available. The returned structure should be freed with shout_metadata_free when you are done with it. 

Second, you need to call shout_metadata_add before calling shout_set_metadata, since adding new metadata to the struct does not update the stream metadata, it just updates the metadata struct.

Third, the only parameter that should ever be set with shout_metadata_add is: name. Extract from an IRC conversation on #icecast:

 <cc0> hello, i am using libshout to send metadata updates for an mp3 icecast stream. i understand that for metadata libshout makes an http get request to the icecast2 server with the url-encoded parameters specifying the metadata info
 <cc0> i was wondering whether there were other parameters/fields than "song"? e.g. "url", ... 
 <cc0> i haven't been able to find any info or spec on this
 <ePirat> no
 <ePirat> [...] mp3 metadata only can support single string of metadata
 <cc0> with id3 tags you can add multiple metadata tags to the stream right?
 <ePirat> no
 <ePirat> id3 is impossible to be used for streaming
 <ePirat> what is used for metadata when streaming mp3 is called ICY and only supports a single string
 <cc0> ePirat: okay, i see. ;) so it is reasonable to assume that the only metadata parameter that should ever be sent is "song"?
 <ePirat> yes
 <ePirat> not 100% if you could send artist or something, but if, then icecast would just join them together separated with a "-"

Following these points, depending on the level of abstraction you (@OlegKunitsyn) want to provide for your library, the different ways this issue can be addressed are:

• Add public bindings for shout_metadata_new, shout_metadata_add, shout_set_metadata, and shout_metadata_free, using an opaque object with no public methods or fields, representing an opaque instanceMeta handle;
• Add public bindings for shout_metadata_new, as etiher a Libshout method or a class constructor, that would return an object of class e.g. LibshoutMetadata, which would implement AutoCloseable (close would call shout_metadata_free), and have add and set methods (the set method could also be a method on Libshout);
• Replace the current setMeta method with a single public void setMetadata(String metadata) throws IOException method, which would create a new metadata instance, add metadata for parameter song to value metadata, set it on the stream, then free the instance.

I highly recommend going for the third solution since the only metadata parameter/key that is used is song and removing the parameter will remove the confusion on metadata handling. Also, it is very cumbersome and error-prone to manually track an object initialisation and explicit close, and the cost associated with creating and freeing a single instance of the metadata type is small enough.

I am creating a pull request to implement the third solution and will keep you updated.

[delthas]

The PR #16 fixes the issue. (It incidentally also fixes #13). Please merge! Thanks.

[OlegKunitsyn]

Same exception for me, but... merged, you are welcome