Created attachment 109657 [details] [review]
Fix CRITICAL warnings and Not Playing state with Stop media key

Tested on 0.11.5, applies to HEAD with very little fuzz.

Wait a bit, better patch incoming.

We can fix streaming metadata failure after Stop in the same patch.

-	player->priv->playing_entry = rhythmdb_entry_ref (entry);
+	player->priv->playing_entry = (entry ? rhythmdb_entry_ref (entry) : NULL);

This is unnecessary.  'entry' will never be NULL here.

+	if (!entry) {
+		return;
+	}
+

.. or here.

Also, the playback state management code is an awful tangled mess that really needs to be hacked to bits and rewritten.  Fixing one thing in it usually just breaks something else.

I didn't add them in for fun, John. :)

I have CRITICAL errors stemming from rhythmdb_entry_ref() on a NULL entry in both of those places. Unless something has changed between 0.11.5 and trunk then they are absolutely needed to make this patch work. (Whether entry should ever be NULL in those places is another matter. As you point out, the state management code is a complete mess. But unless you can suggest a better fix, this is what I have to run with.)

There are more serious issues like gst_player_close() being called on Stop but gst_player_open() is never called again after. After fixing that I realised that the GST backend was never intended to be opened or closed more than once because the logic for resource management is just broken. I nearly have a surgical fix for that.

rb_player_open is called in rb_shell_player_open_location and rb_shell_player_open_playlist_url.  Not sure how that equates to 'never called again after'.

Those aren't currently being called when the Stop / Play buttons are used in sequence.

The problem is that Rhythmbox isn't properly clearing state when the Stop button is pressed. When it goes back into rb_shell_player_playpause, rb_shell_player_get_playing_entry returns an old entry (when it should return NULL) which blocks rb_shell_player_set_playing_entry (subsequently trickling down to rb_player_open).

And, when this is fixed, the secondary problem is that rb_player_gst_open has this clause:

	if (mp->priv->playbin == NULL) {
		if (!rb_player_gst_construct (mp, error))
			return FALSE;
	} else {
		if (!rb_player_close (player, NULL, error))
			return FALSE;
	}

Currently mp->priv->playbin is never freed until the object is finalized, so rb_player_close is called immediately inside rb_player_open. I'm not sure if the intention was to have the backend destroyed on Stop or to simply free the playbin object in rb_player_gst_close.

Even though Rhythmbox appears to work fine in this state, rb_player_opened() returning FALSE causes further problems elsewhere, including in streaming radio title handling.

Created attachment 109790 [details] [review]
Rev 2: Fix CRITICAL warnings and Not Playing state with Stop media key

OK, this one should do the trick.

It is not an architecturally correct solution as it does not resolve the inconsistency in the GST backend open/closed state. However, I think it's better for me to avoid making large changes for fear of breaking other features.

This patch only targets rb_shell_player_stop.

Fixes the error for me on an HP DV2000 and DV6000.

I've committed something fairly similar to this that also fixes a few other problems and makes a tiny start at unravelling the playback state logic code.