Writing rating to file causes file corruption

[Mirian Margiani]

If the option "Write rating to file" is activated, music files are destroyed when you
- change tracks in the playlist
- change the rating anywhere
- stop playback

I could reproduce it using OGG files, I'm not sure if it affects other file types as well. I didn't want to crap my music collection...

Steps to reproduce:
- add two OGG files to the playlist
- start the first one, wait a few seconds
- change rating of one file
- click on the next one, wait...
- change back to the first track -> it shouldn't start

EDIT: even easier steps:
- first way
    - add single Ogg file, don't play
    - change rating
    - try -> it shouldn't start
- second way
    - add single Ogg file, DO play, stop
    - change rating
    - try -> it shouldn't start

Trying to play the corrupted file with mpv results in "Failed to recognize file format". Metadata can be read, though.

It might be a concurrency problem because "writeTrackRatingToFile(...)" in core/mediaitem/tag.cpp is called very often. This even if nothing changed at all.

Interestingly, FLAC and OGG both use XiphComment but they are implemented differently:

...
    else if (TagLib::FLAC::File* file = dynamic_cast<TagLib::FLAC::File*>(fileref.file()))
    {
      TagLib::Ogg::XiphComment* vorbis_comments = file->xiphComment(true);

      vorbis_comments->addField("FMPS_RATING", QStringToTaglibString(QString::number( rating )));
    }
    else if (TagLib::Ogg::XiphComment* tag = dynamic_cast<TagLib::Ogg::XiphComment*>(fileref.file()->tag()))
    {
      tag->addField("FMPS_RATING", QStringToTaglibString(QString::number( rating )));
    }
...

I don't know what it does but looks like it might be the culprit...

[SebastienAmar (sebastien-amardeilh)]

Hi thanks for reporting and sorry for this issue.

I will try to fix it as soon as possible. Regards

[SebastienAmar (sebastien-amardeilh)]

Hi,I tried to reproduce the problem with the same steps you described but everything works fine for me.

Can you run yarock with debug activated (./yarock --debug &) and sand me the log file.

Thanks for your contribution. Regards.

[Mirian Margiani (millimarg)]

Well, I forgot the important part... you have to change the rating.

steps to reproduce:
- add single Ogg file, don't play
- change rating
- try -> it shouldn't start

or:
- add single Ogg file
- play, stop
- change rating
- try -> it shouldn't start

When you didn't start the track, there's an error message: "[Tag] taglib access failed : ==="

I experimented with flac and mp3 as well. The problem occurred only with Ogg files.

[SebastienAmar (sebastien-amardeilh)]

Hi. Thanks for your help.

1) Bug reproduction
-------------------
  I have done many tests and I have still no errors.

2) Remarks
-----------
** When you didn't start the track, there's an error message: "[Tag] taglib access failed : ==="

  This message means your file cannot be read (at least by tag reading part).
  Maybe it appears after the file is corrupted ??

** It might be a concurrency problem because "writeTrackRatingToFile(...)"
  in core/mediaitem/tag.cpp is called very often. This even if nothing
  changed at all.

  This action shall be done only when you click on the stars from the playlist. Only once if one file is selected. I never see multiples calls, maybe you're right it's a good track.

3) Analysis
-----------
  Log file : I have checked all your log file and no specific error appears and the actions sequence seems good.

  Code analysis : I have checked all the code and nothing seems wrong for now

  Ogg corruption : Can you send me a corrupted ogg file you have in order to see if I can find what is wrong. Thanks.
  => we can use "ogginfo" (ogginfo -v file.ogg) to have information (before and after rating with yarock)

Regards,
Sebastien

[Mirian Margiani (millimarg)]

Well, now it's becoming weird. I did a clean recompile, and the rating is now being save only once (not "all the time" anymore). Then I tested with a very small Ogg file, which was not corrupted... The TagLib error did not occur anymore, too.