Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade 1.3.1 : Unknow Artist #887

Closed
jbdesbas opened this issue Aug 31, 2021 · 16 comments
Closed

Upgrade 1.3.1 : Unknow Artist #887

jbdesbas opened this issue Aug 31, 2021 · 16 comments
Labels

Comments

@jbdesbas
Copy link

Hi,

Since 1.3.1 upgrade : covers were not displayed (404 error), so I make a database-reset : all songs are now reconized as "Unknow Artist". I double-check : music files are correctly tagged.

Regards,

@paulijar
Copy link
Collaborator

Thanks for the report.

Which version of ownCloud or Nextcloud are you using? Are your music files residing on some external storage, or locally on the same server with your cloud instance? Is there anything relevant in owncloud.log/nextcloud.log?

@jbdesbas
Copy link
Author

jbdesbas commented Sep 1, 2021

Thanks for your response and your amazing work.

I use NextCloud 22.1.0 (docker-compose install) upgraded to 22.1.1 but same issue. I use Object Storage (S3) as primary storage.
The music app worked like a charm before 1.3.1 upgrade (1.2.1).

Here is some logs entries wich seem related ?

[PHP] Error: Undefined array key "comments" at /var/www/html/custom_apps/music/lib/Utility/DetailsHelper.php#40
GET /apps/music/api/file/7083/details
from ***.***.***.*** by jb at 2021-08-31T12:51:45+00:00

[PHP] Error: Undefined array key "audio" at /var/www/html/custom_apps/music/lib/Utility/DetailsHelper.php#39
GET /apps/music/api/file/7083/details
from ***.***.***.*** by jb at 2021-08-31T12:51:45+00:00

[PHP] Error: Undefined array key "comments" at /var/www/html/custom_apps/music/lib/Utility/DetailsHelper.php#40
GET /apps/music/api/file/7146/details
from ***.***.***.*** by jb at 2021-08-31T12:51:39+00:00

[PHP] Error: Undefined array key "audio" at /var/www/html/custom_apps/music/lib/Utility/DetailsHelper.php#39
GET /apps/music/api/file/7146/details
from ***.***.***.*** by jb at 2021-08-31T12:51:39+00:00

@paulijar
Copy link
Collaborator

paulijar commented Sep 2, 2021

It seems that, for some reason, metadata cannot be extracted from the files on your S3 storage. No related changes have been made on Music v1.3.1 so my best guess is that the actual breakage has happened on some Nextcloud update. It just hasn't been obvious before the Music app update, because Music app has been using previously cached data. But the Music app update always clears the cache, which could have made the problem very visible.

The common problem with the external storages is that many of them don't support seeking within the files with fseek. This, on the other hand, is needed to be able to extract the metadata. But for S3 storages, this should have got fixed a few NC major versions ago and I couldn't find any open related issues from the Nextcloud server github.

@jbdesbas
Copy link
Author

jbdesbas commented Sep 2, 2021

OK, understood, I think you are right, thanks!

I tried to delete and re-upload some tracks with no success. Curiously, metadata are correctly displayed from the web file browser (in the side bare, thank to the Metadata app).

I close the issue since is not related to Music, thank again.

@jbdesbas jbdesbas closed this as completed Sep 2, 2021
@paulijar
Copy link
Collaborator

paulijar commented Sep 2, 2021

I don't know but it's possible that the Metadata app makes a local copy of the file before extracting the metadata; that could be used to work around any issues faced when analyzing a file directly on a remote storage. Maybe Music could do this too, when opening the details pane for a single file. But doing it for each and every file when scanning the library would be horrendously slow.

@jbdesbas
Copy link
Author

jbdesbas commented Sep 2, 2021

Yes I agree, and it can be expensive for traffic paying buckets.
For your information, i just tried with Audio Player app (never installed before today) : metadata (artist, album, cover, tracks number) are correctly used.

@jbdesbas
Copy link
Author

jbdesbas commented Sep 3, 2021

Hi again,

I downgrade Music to 1.2.1, upload new tracks (to avoid cache issue) : everything is OK like before 👍
So maybe it can be related to Music upgrade ? 😕

Regards,

@jbdesbas jbdesbas reopened this Sep 3, 2021
@paulijar
Copy link
Collaborator

paulijar commented Sep 3, 2021

Ok, thanks for testing. So apparently there is something interesting going on after all.

Is the assumption about this being related to the S3 storage still correct? That is, if you place any audio files in the Nextcloud internal storage, then those can be scanned properly also by v1.3.1?

If you set the logging level to 'debug' for a short while and try to view details of some track, is there anything more shown in the nextcloud.log?

@paulijar paulijar added the Bug label Sep 3, 2021
@paulijar
Copy link
Collaborator

paulijar commented Sep 3, 2021

@jbdesbas Okay, I found one suspect change which could be relevant here. We have updated the getID3 library used to analyze the metadata in v1.3.1 and the new version contains at least one code change related to the use of the fseek function. One interesting test could be if you could take the 3rdparty directory from Music v1.2.1 and replace the corresponding directory in v1.3.1 with it.

@paulijar
Copy link
Collaborator

paulijar commented Sep 3, 2021

Even more focused test case on my hunch could be to comment out this block of code from the version of getID3 included in Music v1.3.1:

if ($result !== 0) { // fseek returns 0 on success
throw new getid3_exception('cannot fseek('.$pos.'). resource/stream does not appear to support seeking', 10);
}

@paulijar
Copy link
Collaborator

paulijar commented Sep 5, 2021

As a related change, the new Music v1.3.2 can now use the second level parent folder of the file as the fallback for the artist name. That is, if you organize your music files to folders like <path to library>/<artist name>/<album name>/<track name>.mp3, then that <artist name> will be used to organize the Albums view if the metadata is not present or cannot be extracted from the file.

This, of course, is not really a fix for the reported problem, but it may improve the situation a bit while waiting for the proper fix. If the root cause of the problem is where I assume it to be, then it probably should be fixed in the Nextcloud server. I could try to provide a pull request for this, but first I would need to set-up a S3 storage to be able to test the solution. It will take at least a week before I have time to even start to look into this.

@jbdesbas
Copy link
Author

Hi, thanks for those explanations. Do you still need me to test ?

@paulijar
Copy link
Collaborator

Well, I'm probably able to test this myself within the next few days. But if you have time to test before that, then that could give us a small head start and show if I'm on the right track.

@paulijar
Copy link
Collaborator

@jbdesbas I was now able to set up a S3 storage and confirm my assumptions, so there's no longer reason for you to test this. But if you want to monkey-patch your installation, you can comment out the line

throw new getid3_exception('cannot fseek('.$pos.'). resource/stream does not appear to support seeking', 10);

... and get the metadata extraction to work like on Music v1.2.1.

Now, the thing is, the metadata extraction with getID3 wasn't really working with S3 storages even on v1.2.1. But it just happened to produce correct results for files with ID3v2 tags. On the other hand, ID3v1 tags could not be extracted. The underlying issue here is, that the recent versions of Nextcloud provide support to seek files from the beginning of the file on S3 storage, but seeking from the end of the file is not supported. The seeking from the end of the file is needed for ID3v1 (and probably some other tag types), but not for ID3v2. The older versions of getID3 weren't checking if the seeking actually worked, and hence, they happily extracted ID3v2 and silently failed on ID3v1. The new version of getID3 added checking for the return value of fseek and it now gives up on a file if the seeking (from either end) doesn't seem to work.

@paulijar
Copy link
Collaborator

paulijar commented Oct 1, 2021

This should now be fixed in Nextcloud versions 22.2.0, 21.0.5, and 20.0.13, which all were released yesterday.

@paulijar paulijar closed this as completed Oct 1, 2021
@jbdesbas
Copy link
Author

Upgraded Nextcloud 22.2.0 and Music 1.4 --> I confirm that it works well with S3 storage ! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants