Sonerezh / sonerezh

A self-hosted, web-based application to stream your music, everywhere.
https://www.sonerezh.bzh
GNU Affero General Public License v3.0
767 stars 122 forks source link

Use GetID3 package from Composer #346

Closed MightyCreak closed 6 years ago

MightyCreak commented 6 years ago

The goal of this PR is to use GetID3 package from Composer instead of the old package included right now, and fix #343.

Here's what I've done:

This might not be the right way of doing things between CakePHP and Composer, I don't know much about those. Feel free to tell me how it should be done if there's a better way.

lGuillaume124 commented 6 years ago

When we tried to update this lib from 1.9.7-20130705 to the very next version we noticed it was a little bit longer to fetch the cover arts. That's why we didn't upgrade it but it seems to be required now…

MightyCreak commented 6 years ago

I haven't seen a big difference (but I haven't profiled either). I'll check that.

For sure the package is heavier (I guess it handles more format as well), but I don't think it's really a problem if a package is taking 200 kB instead of 100 kB (numbers are fake). If you don't have enough space for that on your server, I guess you'll have other problems 😁

About the cover, a simple optimization could be to check for the cover image first in the album dir? It would be faster for sure, I know it would for me since I always keep a cover.jpg in the album directory, but I guess not everyone does that 😉

MightyCreak commented 6 years ago

Ok so I tested the import of my library (2597 files) with the old and the new GetID3 lib, using the command line (because it's easier to benchmark like that):

That's clearly much longer (x4), but from my perspective, it's not that bad since it only affects the import. It would surely have been nicer if it has been faster, but I think using a package that is easy to upgrade is more valuable that the added delay during the import. Maybe GetID3 v2 that is in preview at the moment tries to address that?

PS: By using the command line, I saw that there was a problem with the new CakePHP path there, so I fixed it: 6e7572d1734f60f4f385e39a5d2d882a78de75b9.

MightyCreak commented 6 years ago

I updated the CHANGELOG as well -- the "Unreleased" section is a good practice I got from https://keepachangelog.com/, so that you don't have to browse all the commits when it's time to do a release.

MightyCreak commented 6 years ago

@lGuillaume124 I know you're in the new database refactor, but since this PR also fixes #343, would you consider merging it?

@gs11 would you like to review it as well?

lGuillaume124 commented 6 years ago

As the « WIP » label was still here I thought you were still working on it.

gs11 commented 6 years ago

LGTM, great work! @MightyCreak

MightyCreak commented 6 years ago

Thanks guys! I was expecting to have done something bad so I didn't remove the "WIP" flag 😅