[sldev] Updated streaming patch, would like to commit

[Carlo Wood]

Some general remarks...

On Wed, Apr 01, 2009 at 01:20:14AM +0200, Dale Glass wrote:
> +
> +	LLMediaBase * getStreamMedia() { return mInternetStreamMedia; }

Are we not following const correctness?

Normally this should be:

   	LLMediaBase* getStreamMedia() const { return mInternetStreamMedia; }

>  public:
>  	F32 mMaxWindGain; // Hack.  Public to set before fade in?
>  
> diff --git a/indra/llmedia/llmediaimplgstreamer.cpp b/indra/llmedia/llmediaimplgstreamer.cpp
> index 51614c5..d4e8fc2 100644
> --- a/indra/llmedia/llmediaimplgstreamer.cpp
> +++ b/indra/llmedia/llmediaimplgstreamer.cpp
> @@ -73,7 +73,8 @@ LLMediaImplGStreamer () :
>  	mTextureFormatType ( LL_MEDIA_UNSIGNED_INT_8_8_8_8_REV ),
>  	mPump ( NULL ),
>  	mPlaybin ( NULL ),
> -	mVideoSink ( NULL )
> +	mVideoSink ( NULL ),
> +	mLastTitle ( "" )

A std::string is empty by default, there is no need to list
it in the initialization list like this.

>  #ifdef LL_GST_SOUNDSINK
>  	,mAudioSink ( NULL )
>  #endif // LL_GST_SOUNDSINK
> @@ -300,6 +301,8 @@ LLMediaImplGStreamer::bus_callback (GstBus     *bus,
>  		case GST_STATE_PAUSED:
>  			break;
>  		case GST_STATE_PLAYING:
> +			impl->mLastTitle = "";
> +

impl->mLastTitle.clear(); is faster.

> +	case GST_MESSAGE_TAG:
> +	{
> +		DEBUGMSG("GST message tag");
> +		GstTagList *new_tags = NULL;
> +
> +		gst_message_parse_tag( message, &new_tags );
> +
> +		if ( new_tags )
> +		{
> +			gchar *title = NULL;
> +			if ( gst_tag_list_get_string(new_tags, GST_TAG_TITLE, &title) )
> +			{
> +				gst_tag_list_free(new_tags);
> +				std::string newtitle(title);
> +
> +				if ( newtitle != impl->mLastTitle && newtitle != "" )

    ... && !newtitle.empty()

is faster, in fact I'd turn this around:

    if (!newtitle.empty() && newtitle != impl->mLastTitle)

> +				{
> +					impl->mLastTitle = newtitle;
> +					LLMediaEvent event( impl, impl->mLastTitle );
> +					impl->getEventEmitter().update( &LLMediaObserver::onMediaTitleChange, event );
> +				}
> +
> +				g_free(title);

I'd move this up, to directly below the std::string newtitle(title)

> +class LLTitleObserver
> +	:	public LLMediaObserver
> +{
> +public:
> +	void init(std::string url);

Pass by value?? This isn't java or LSL :p
Surely you meant:

   	void init(std::string const& url);

> +	/*virtual*/ void onMediaTitleChange(const EventType& event_in);
> +private:
> +	LLMediaBase* mMediaSource;
> +};
> +
> +static LLTitleObserver sTitleObserver;
> +
> +static LLRegisterWidget<LLMediaRemoteCtrl> r("media_remote");

static is not used anymore in C++. You'd normally an anonymous
namespace. More importantly however, global variables are the
root of all evil. I'm worried when I see things like this.

-- 
Carlo Wood <carlo at alinoe.com>