Created attachment 136684 [details] [review]
Don't update DateUpdated stamp when playback finishes

Created attachment 136685 [details] [review]
Corrected patch

Thanks Alexander.  The one thing about this patch is that it will break save-rating/playcount, but I guess we can patch this when that lands.

You have

if (!transient_fields.IsSupersetOf (fields_changed)) {

but can't fields_changed be null, which would cause an System.ArgumentNullException there?

(In reply to comment #3)
> Thanks Alexander.  The one thing about this patch is that it will break
> save-rating/playcount, but I guess we can patch this when that lands.

I just checked, the ratings/playcounts are saved when the song's metadata is edited (e.g. a new rating is set). Am I missing something?

> You have
> 
> if (!transient_fields.IsSupersetOf (fields_changed)) {
> 
> but can't fields_changed be null, which would cause an
> System.ArgumentNullException there?

Nope, it's never null because fields_changed is a params argument. If nothing is supplied, it's just empty.

I agree however that when fields_changed is empty, we should change DateAdded. For example, the track editor doesn't supply any fields to the Save method. I will update the patch.

Created attachment 136792 [details] [review]
Update DateAdded when Save() is called without arguments

If you rate a track outside of the editor (via menu, etc) or the play/skip count is updated by playing/skipping, that is the scenario that we don't want to write to file (unless you have write-ratings/counts-to-file checked..) right?

I think in the end we'll need another timestamp column for TransientsUpdated, to track when the play/skip/rating is changed.

(In reply to comment #6)
> If you rate a track outside of the editor (via menu, etc) or the play/skip
> count is updated by playing/skipping, that is the scenario that we don't want
> to write to file (unless you have write-ratings/counts-to-file checked..)
> right?
> 
> I think in the end we'll need another timestamp column for TransientsUpdated,
> to track when the play/skip/rating is changed.

I think we *do* want to save when the rating is changed. The patch from bug 532650 would then be able to do its job and write the rating to the file.

What we don't want is triggering file write on every track change, and this patch fixes it.

Gabriel, ignore my previous comment, I misunderstood you :X

(In reply to comment #6)
> If you rate a track outside of the editor (via menu, etc) or the play/skip
> count is updated by playing/skipping, that is the scenario that we don't want
> to write to file (unless you have write-ratings/counts-to-file checked..)
> right?

Right, except that we still don't want to write when play/skip count is updated, we only want to write when the rating is changed (if the write ratings/counts option is on). This has been discussed in bug 532650, see for example comment #13.

This means we have 3 types of metadata:

 * the usual suspects (title, album, etc) - trigger file write on all changes (if the write metadata is on)
 * transients (Score, SkipCount, LastSkipped, PlayCount, LastPlayed) - never trigger any writes
 * rating - triggers file write only if the write ratings/counts option is on

> I think in the end we'll need another timestamp column for TransientsUpdated,
> to track when the play/skip/rating is changed.

Actually we would need a RatingUpdated stamp. However, I think this would introduce unnecessary complexity for no real benefits. Ratings are not changed too often, saving all metadata when a rating is changed is IMHO not a big deal.

My patch works like this: transients don't cause any file saves, the other metadata changes (including ratings) update the DateUpdated stamp which may cause the file write depending on the options.

(In reply to comment #8)
> Gabriel, ignore my previous comment, I misunderstood you :X
> 
> (In reply to comment #6)
> > If you rate a track outside of the editor (via menu, etc) or the play/skip
> > count is updated by playing/skipping, that is the scenario that we don't want
> > to write to file (unless you have write-ratings/counts-to-file checked..)
> > right?
> 
> Right, except that we still don't want to write when play/skip count is
> updated, we only want to write when the rating is changed (if the write
> ratings/counts option is on). This has been discussed in bug 532650, see for
> example comment #13.

That behavior sounds wonky to me.  If you never rate or edit a song, but you have write-to-file enabled, you'd expect your playcounts to actually be saved but they wouldn't be.

> > I think in the end we'll need another timestamp column for TransientsUpdated,
> > to track when the play/skip/rating is changed.
> 
> Actually we would need a RatingUpdated stamp. However, I think this would
> introduce unnecessary complexity for no real benefits. Ratings are not changed
> too often, saving all metadata when a rating is changed is IMHO not a big deal.

Right, that's not a problem.  The problem is that if had write-ratings=off and you update the DateUpdated stamp because the rating changed, then the SaveMetadataJob will be triggered for that track unnecessarily.

Another problem is the ability to apply the save-ratings retroactively; when the user first checks the box, if we had a DateRatingUpdated field, we could determine all the tracks that now should be written b/c their ratings have changed since import.

Thoughts?  I may just be looking at this wrong or over analyzing.

(In reply to comment #9)
> That behavior sounds wonky to me.  If you never rate or edit a song, but you
> have write-to-file enabled, you'd expect your playcounts to actually be saved
> but they wouldn't be.

I agree, that's a bit of a dilemma: what is worse, updating files on each track change (because the playcount is changed) or never updating the file's playcount because it has never been rated or edited?...

May be a solution could be to separate settings for "Write ratings" and "Write playcounts"? People who want their playcounts up-to-date will have them, while those who want just ratings won't have their files unnecessarily re-written on each track change.

> Right, that's not a problem.  The problem is that if had write-ratings=off and
> you update the DateUpdated stamp because the rating changed, then the
> SaveMetadataJob will be triggered for that track unnecessarily.
> 
> Another problem is the ability to apply the save-ratings retroactively; when
> the user first checks the box, if we had a DateRatingUpdated field, we could
> determine all the tracks that now should be written b/c their ratings have
> changed since import.

I like this idea, saving all ratings to files would make a neat feature! I guess this discussion should be moved to bug 532650, this one is about something else. I still think that my last patch fixes it, however I agree that the RatingsField should be added to the list of the transient_fields to avoid unnecessary file saves when rating is changed. I will update the patch.

Created attachment 136976 [details] [review]
Don't update the file when the rating is changed

Alex, thanks a bunch for the patch.

However, I have tried it, and it doesn't work for me (I keep getting this on the console:)

[Debug 16:23:31.674] Starting - Saving Metadata to File
[Debug 16:23:31.690] Saving metadata for Cass & Slide - Perception (on Unknown Album) <00:09:28.2560000> [file:///home/knocte/Documents/iDocs/Musica/mp3/Varios/Trance%20II/Perception.mp3]
[Debug 16:23:31.695] Finished - Saving Metadata to File

Andrés, I just checked and the patch works fine on my PC.

Could you add Console.WriteLine before `DateUpdated = DateTime.Now` to see if it's called?

Just added this to your patch:

 if (fields_changed.Length == 0 || !transient_fields.IsSupersetOf(fields_changed)) {
+    Console.WriteLine ("___going to save DateUpdated");
     DateUpdated = DateTime.Now;
 }
 
 bool is_new = (TrackId == 0);
 if (is_new) {
+    Console.WriteLine ("___going to save DateUpdated & DateAdded");
     DateAdded = DateUpdated = DateTime.Now;
 }


And yes, it gets called:

[Debug 17:05:15.305] Starting - Saving Metadata to File
[Debug 17:05:15.312] Saving metadata for Dave Brubeck - Take Five (on Unknown Album) <00:05:27.9240000> [file:///home/knocte/Documents/iDocs/Musica/mp3/Varios/Jazz/Take%20Five.mp3]
___going to save DateUpdated
[Debug 17:05:15.321] Finished - Saving Metadata to File

Thanks Andrés, could you also add this:

                foreach (var field in fields_changed) {
                    Console.WriteLine (field.Name);
                }
                Console.WriteLine (Environment.StackTrace);

...and paste the output here.

___going to save DateUpdated...
   at System.Environment.get_StackTrace() in /usr/src/packages/BUILD/mono-2.4/mcs/class/corlib/System/Environment.cs:line 202
   at Banshee.Collection.Database.DatabaseTrackInfo.Save(Boolean notify, Hyena.Query.QueryField[] fields_changed) in /home/knocte/Documents/iDocs/Proyectos/Banshee/arbolGIT/banshee/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs:line 158
   at Banshee.Collection.Database.DatabaseTrackInfo.Save() in /home/knocte/Documents/iDocs/Proyectos/Banshee/arbolGIT/banshee/src/Core/Banshee.Services/Banshee.Collection.Database/DatabaseTrackInfo.cs:line 127
   at Banshee.Collection.TrackInfo.SavePlaybackError(StreamPlaybackError value) in /home/knocte/Documents/iDocs/Proyectos/Banshee/arbolGIT/banshee/src/Core/Banshee.Core/Banshee.Collection/TrackInfo.cs:line 460
   at Banshee.MediaEngine.PlayerEngineService.OnEngineEventChanged(Banshee.MediaEngine.PlayerEventArgs args) in /home/knocte/Documents/iDocs/Proyectos/Banshee/arbolGIT/banshee/src/Core/Banshee.Services/Banshee.MediaEngine/PlayerEngineService.cs:line 242
   at Banshee.MediaEngine.PlayerEngine.RaiseEventChanged(Banshee.MediaEngine.PlayerEventArgs args) in /home/knocte/Documents/iDocs/Proyectos/Banshee/arbolGIT/banshee/src/Core/Banshee.Services/Banshee.MediaEngine/PlayerEngine.cs:line 179
   at Banshee.MediaEngine.PlayerEngine.OnEventChanged(Banshee.MediaEngine.PlayerEventArgs args) in /home/knocte/Documents/iDocs/Proyectos/Banshee/arbolGIT/banshee/src/Core/Banshee.Services/Banshee.MediaEngine/PlayerEngine.cs:line 167
   at Banshee.GStreamer.PlayerEngine.OnError(IntPtr player, UInt32 domain, Int32 code, IntPtr error, IntPtr debug) in /home/knocte/Documents/iDocs/Proyectos/Banshee/arbolGIT/banshee/src/Backends/Banshee.GStreamer/Banshee.GStreamer/PlayerEngine.cs:line 310
   at Gtk.Application.gtk_main()
   at Gtk.Application.Run() in /usr/src/packages/BUILD/gtk-sharp-2.12.6/gtk/generated/AboutDialog.cs:line 1
   at Banshee.Gui.GtkBaseClient.Run() in /home/knocte/Documents/iDocs/Proyectos/Banshee/arbolGIT/banshee/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs:line 148
   at Banshee.Gui.GtkBaseClient.Startup() in /home/knocte/Documents/iDocs/Proyectos/Banshee/arbolGIT/banshee/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs:line 74
   at Hyena.Gui.CleanRoomStartup.Startup(Hyena.Gui.StartupInvocationHandler startup) in /home/knocte/Documents/iDocs/Proyectos/Banshee/arbolGIT/banshee/src/Libraries/Hyena.Gui/Hyena.Gui/CleanRoomStartup.cs:line 54
   at Banshee.Gui.GtkBaseClient.Startup() in /home/knocte/Documents/iDocs/Proyectos/Banshee/arbolGIT/banshee/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs:line 69
   at Banshee.Gui.GtkBaseClient.Startup(System.String[] args) in /home/knocte/Documents/iDocs/Proyectos/Banshee/arbolGIT/banshee/src/Core/Banshee.ThickClient/Banshee.Gui/GtkBaseClient.cs:line 59
   at Nereid.Client.Main(System.String[] args) in /home/knocte/Documents/iDocs/Proyectos/Banshee/arbolGIT/banshee/src/Clients/Nereid/Nereid/Client.cs:line 54
___going to save DateUpdated...DONE

From IRC:

(05:38:14 PM) knocte: hugh, seems like there are no fields_changed
(05:38:23 PM) knocte: pasting new comment...
(05:40:08 PM) knocte: ok, my stacktrace comes from SavePlaybackError because, right now, I have my pulseaudio screwed
(05:40:19 PM) knocte: that's maybe the reason why the patch is not working for me
(05:40:25 PM) knocte: but anyways we should fix this case

Created attachment 138276 [details]
Another trace, this time having pulseaudio working

This demonstrates that the patch doesn't fix the issue for me.

(In reply to comment #18)
> Created an attachment (id=138276) [edit]
> Another trace, this time having pulseaudio working
> 
> This demonstrates that the patch doesn't fix the issue for me.
> 
Could it be because you are now playing tracks that were previously marked with errors? Changing playback error currently triggers the full track save.

Could you try to play tracks without errors? Also, do you print out the names of the fields?

Created attachment 138648 [details] [review]
Just in case, this is the patch plus the CWLs

(In reply to comment #19)
> (In reply to comment #18)
> > Created an attachment (id=138276) [edit]
> > Another trace, this time having pulseaudio working
> > 
> > This demonstrates that the patch doesn't fix the issue for me.
> > 
> Could it be because you are now playing tracks that were previously marked with
> errors? Changing playback error currently triggers the full track save.

Seems you're right. I just tried to play a track not previously marked with an error, and this is the result with this patch:

[Debug 21:49:34.390] Player state change: Idle -> Loading
[Debug 21:49:34.964] Player state change: Loading -> Loaded
[Debug 21:49:35.016] Player state change: Loaded -> Playing
[Debug 21:49:35.049] Creating Pango.Layout, configuring Cairo.Context
[Debug 21:49:35.049] Creating Pango.Layout, configuring Cairo.Context
[Debug 21:49:36.039] TrackInfoDisplay RenderAnimation: 27.00 FPS
[Debug 21:49:36.040] TrackInfoDisplay RenderAnimation: 27.00 FPS

That is, no saves.

> Could you try to play tracks without errors? Also, do you print out the names
> of the fields?

Yes, I was printing the names of the fields, but seems in my traces fields_changed.Length was 0 because they didn't appear. I've changed the patch to add fields_changed.Length in the CWL.

Anyway, I keep seeing unwanted saves, and now I'm referring to the startup of the application. I will attach another trace as a file (too big to put it here).

Created attachment 138649 [details]
logs of unwanted saves at the startup of the app

This is the log using the previous patch.

I guess we all agree in confirming the bug.

(In reply to comment #20)
> Anyway, I keep seeing unwanted saves, and now I'm referring to the startup of
> the application. I will attach another trace as a file (too big to put it
> here).

Do you always see these saves on startup or it happened only once? If former, does it always save the same tracks? 

Created attachment 138680 [details] [review]
Don't update the file when the rating or the playbackerror is changed 

Ok, I slightly modified Alex's patch, in order to take in account the PlaybackError case. I have tested it and it works.
Implementation notes: I couldn't just add a new boolean value because it would have changed the meaning of some overloads, therefore I decided to you binary flags. Please, be sure to point me out errors, problems, improvements, whatever! Thanks.

(In reply to comment #23)
> Do you always see these saves on startup or it happened only once?

No, it seems those files were in some list, pending to be saved...

> If former, does it always save the same tracks?

s/to you/to use/

Created attachment 138697 [details] [review]
 Enter the path to the file on your computer

Oooops, made a mistake in last patch.

Argh, that one is not correct either, sorry for the bugspam guys...

(In reply to comment #24)
> Ok, I slightly modified Alex's patch, in order to take in account the
> PlaybackError case. I have tested it and it works.
> Implementation notes: I couldn't just add a new boolean value because it would
> have changed the meaning of some overloads, therefore I decided to you binary
> flags. Please, be sure to point me out errors, problems, improvements,
> whatever! Thanks.

I'm not sure that's the best approach. If you really want to avoid saving when there's a playback error, it's better to introduce a new QueryField for the PlaybackError and pass it to Save() from SavePlaybackError()

Regardless, I think that's another issue that should be filed and tracked separately. My last patch already fixes the original problem (currently, *all* track changes trigger file writes). 

What you reported later in comment #12 is a very special case that doesn't affect most users (the track cannot be played, which results in a file write).

To sum up: I suggest that my last patch is committed because it fixes the problem that affects all users; and a new bug is created to track the issue when playing a song results in stream error which causes a file save.

Created attachment 138704 [details] [review]
Don't update the file when the rating or the playbackerror is changed

Ok, this is the patch that supposedly corrects the ones above (and it includes CWLs).

BUT, I've just realised that I found a case in which the line "DateUpdated = DateTime.Now;" *is reached* but it didn't change the DateModified field of the file. Now I'm confused, because then none of the patches here is entirely correct. Any help with explaining this?

Thanks

(In reply to comment #28)
> To sum up: I suggest that my last patch is committed because it fixes the
> problem that affects all users; and a new bug is created to track the issue
> when playing a song results in stream error which causes a file save.

Ok, agreed, as long as we talk about comment#28 first. Can you try to reproduce the scenario? I think it happened when importing tracks.

Oops, I meant comment#29...

Created attachment 139003 [details]
Log (DateUpdated and DateAdded being assigned while no change went to the files)

About comment#29...:

Alex, on IRC you asked me "can you reproduce that reliably" and I reply: YES.

Just apply the 3rd attachment to this bug on your source, and import a folder with 2 songs. Results? The log that I'm attaching here. As you can see, DateAdded and DateUpdated are assigned, but the files still have their DateModified field in the filesystem intact. I haven't explored Banshee's sources enough to know how this could happen, I was just wondering that maybe you had a better idea?

(In reply to comment #32)
> As you can see,
> DateAdded and DateUpdated are assigned, but the files still have their
> DateModified field in the filesystem intact. I haven't explored Banshee's
> sources enough to know how this could happen, I was just wondering that maybe
> you had a better idea?

I can explain what happens:

When you import files, StreamTagger.TrackInfoMerge() is called from DatabaseImportManager.ImportTrack() (see line 177) and it updates the track's LastSyncedStamp (see line 174)

Because the timestamp is stored in the database as a unix time and has a resolution of 1 second, the DateAdded and DateUpdated timestamps end up to be the same as LastSyncedStamp which results in the SaveTrackMetadataJob not being triggered.

I think this is the correct behaviour: when the track is imported it already gets the right metadata and file name during the import; there is no need to update them again.

I'm going to commit the patch from comment #11 and close the bug - this one already got too many comments for such a trivial problem and as a result is pretty hard to follow.

Please feel free to open a new bug for the other issue you reported (file save is triggered on stream error)

*** Bug 590742 has been marked as a duplicate of this bug. ***

Andres, the patch in bug 590946 should fix the issue you reported (file writes when playback errors occur). Could you check if it works for you?

(In reply to comment #35)
> Andres, the patch in bug 590946 should fix the issue you reported (file writes
> when playback errors occur). Could you check if it works for you?

Alex, sorry for being so late to reply. It turns out I had no way to test that again because I no longer had a buggy-gstreamer system. Today I tested with a machine that doesn't have MP3 codecs and it worked (so, the datemodified bit was not modified), but I'm not sure this is the same kind of playback error we were testing against. What I see in the console is:

** Message: don't know how to handle audio/mpeg, mpegversion=(int)1, layer=(int)3
[Error 19:43:46.725] GStreamer stream error: CodecNotFound

(In reply to comment #36)
> ... but I'm not sure this is the same kind of playback error we
> were testing against.

I was right, this was not the way I got this problem. So I finally reproduced it again and file a new bug for it: bug 612512.