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

Cover Art Fetcher #4851

Merged
merged 24 commits into from
Feb 7, 2023
Merged

Cover Art Fetcher #4851

merged 24 commits into from
Feb 7, 2023

Conversation

fatihemreyildiz
Copy link
Contributor

@fatihemreyildiz fatihemreyildiz commented Jul 11, 2022

Hey everyone!

My goal is to get cover arts of the tracks by using coverartarchive on the "Import Metadata from Musicbrainz" menu, aka Tag Fetcher Menu. I am working on this feature right now. This is the initial PR to get your ideas-thoughts and of course feedback about the GUI of this feature. More information about this PR can be found on our Zulip chat https://mixxx.zulipchat.com/#narrow/stream/109215-gsoc/topic/cover.20art.20lookup

This is the first interaction for now. I have displayed the existing cover art of the track on the Tag Fetcher Menu. The Tag Fetcher Menu seems like this. Tag Fetcher Menu

Right now I can send the metadata of the Suggested track to coverartarchive and get a response via a button. It is better to first decide what should GUI look like? Then we can implement that (or something similar) later on.

For to decide about GUI, we should decide how this feature can be used?

  • I think there should be a button called "look for cover art" which triggers the request when pressed, and can get cover art of the selected Suggested Tags.
  • Should we have separate buttons for the metadata and cover art? Can cover-art be updated separate then the metadata or apply button should update both the cover art and the metadata?

Do you have any other use-case comes to your mind? These few buttons would make this menu crowded and complicated? What do you think generally?

(This feature also needs your feedback about technical stuff, the first thing is how to store the cover-arts, cover-arts has thumbnails should we use them, if the cover art updated, should we save it as a file or embed in the file, should user choose between these storing options and so on...)

@ronso0
Copy link
Member

ronso0 commented Jul 11, 2022

Nice!

The GUI can be as rough as necessary for testing the functionality, don't worry. That can be rearranged and polished later on.

Just some ideas/impressions regarding the GUI:

  • show the current cover and new candidates in a vertical layout:
    • "current:" at the top
    • candidate (blank cover if no candidate has been fetched?)
    • "Get cover art" button at the bottom?
  • maybe it would be worth to rework the WCoverArt instantiation so the right-click menu can be disabled in the musicbrainz dialog?

I think we should fetch an adequate thumbnail first. Smallest is 250px, right? Then try choose bigger version if necessary, depending on the Mixxx GUI scalefactor, so images look sharp on HiDPI screens.
Maybe users want to preview the image in full size. Usually left-click on the cover opens the separate resizable popup. Dunno how that would feel like if the full-size cover needs to be fetched first.

@fatihemreyildiz
Copy link
Contributor Author

Thanks you for the feedback @ronso0

The GUI can be as rough as necessary for testing the functionality, don't worry. That can be rearranged and polished later on.

That's good! So we must decide the GUI at least for the testing.

show the current cover and new candidates in a vertical layout:
"current:" at the top
candidate (blank cover if no candidate has been fetched?)
"Get cover art" button at the bottom?

I think that 2 cover arts is a great idea. Both for technical and UX way. So these two cover arts can have different functionality. One is simply can show the existing cover art, the other can show selected suggested tags.

maybe it would be worth to rework the WCoverArt instantiation so the right-click menu can be disabled in the musicbrainz dialog?

Maybe a child class can do that. Which can only show the cover art and populate the full size of the cover art. Would something like that work?

I think we should fetch an adequate thumbnail first. Smallest is 250px, right? Then try choose bigger version if necessary, depending on the Mixxx GUI scalefactor, so images look sharp on HiDPI screens.
Maybe users want to preview the image in full size. Usually left-click on the cover opens the separate resizable popup. Dunno how that would feel like if the full-size cover needs to be fetched first.

Yeah thumbnails are 250px. I think for displaying the suggested tag cover arts we better use the 250px in order to reduce the performance issues. We thought users can choose the resolution of the cover art. We have a popup for existing cover-arts, maybe something similar can be done for the founded cover arts.

By the way, I was playing with the UI, so I did something like this. With this one button, maybe found cover arts can be displayed if the selected result triggered. What do you think?
CoverArtFetcher

@daschuer
Copy link
Member

The resolution/quality question has some aspects we need to consider.

  • Internet bandwidth
  • Display resolution
  • Storage space
  • Time for fetching
  • rate limits

What is the best strategy?

We may either always fetch all thumbnail covers for all MusicBrainz results, so that they are immediately displayed when selecting a row. When the user has decided to integrate a cover, we may download the desired target size.

Another strategy may be to download only the desired target size on demand and rescale it before displaying.

What should be the default behaviour? What should be the default size?

@ronso0 what do you think?

@daschuer
Copy link
Member

I can think of a few checkboxes:

  • Fetch Cover
  • Use High-Res cover (1MB)
  • Store as track-file-name.jpg
  • Integrate in track file

Is this a good Idea to offer these options?
Do we want to place them on the dialog or in preference?

@Swiftb0y
Copy link
Member

Does Qt have some abstraction that we can query whether the system is on a metered connection? We could use that for deciding the quality option and eagerness of the fetching. So we can save bandwidth when the connection is metered.

@fatihemreyildiz
Copy link
Contributor Author

Does Qt have some abstraction that we can query whether the system is on a metered connection? We could use that for deciding the quality option and eagerness of the fetching. So we can save bandwidth when the connection is metered.

I found this on QT Doc, can it work? I assume it is not because It says since 6.2.

@Swiftb0y
Copy link
Member

Thats pretty much exactly what I wanted, but yes, we can't use it because we're still stuck on Qt5.15

@fatihemreyildiz
Copy link
Contributor Author

Thats pretty much exactly what I wanted, but yes, we can't use it because we're still stuck on Qt5.15

So there should be some TO DO comment lines for the future.

@fatihemreyildiz
Copy link
Contributor Author

The resolution/quality question has some aspects we need to consider.

  • Internet bandwidth
  • Display resolution
  • Storage space
  • Time for fetching
  • rate limits

What is the best strategy?

We may either always fetch all thumbnail covers for all MusicBrainz results, so that they are immediately displayed when selecting a row. When the user has decided to integrate a cover, we may download the desired target size.

Another strategy may be to download only the desired target size on demand and rescale it before displaying.

What should be the default behaviour? What should be the default size?

@ronso0 what do you think?

There is no rate limits according to Coverartarchive API documentation.

That was exactly what I thought in the first place, we should fetch all thumbnails with the MusicBrainz results, and user can see by just clicking on the suggested tags, if they like they can get the cover art.

I have tried the API with the few songs, most of the results are not stable. What I mean by that is there is a main image with the high resolution which was 11MB. Then thumbnails come along, the sizes are generally starts from 1200 and then gets smaller to 500 and 250. There are also 2 keys with the small and large which is equivalent of 250px and 500px. Some Album Release ID's have only small and large. So maybe we might use to fetch small for the first time and users can choose large to integrate.

What do you think?

I can think of a few checkboxes:
Fetch Cover
Use High-Res cover (1MB)
Store as track-file-name.jpg
Integrate in track file
Is this a good Idea to offer these options?
Do we want to place them on the dialog or in preference?

I think we should offer this options later on after this feature is working. And if we do that we should do in the preference. On preferences there could be section for cover art fetcher. If the users want to fetch cover arts they can choose. I think this will affect the UI, I couldn't make sure how to deal with it at the first time but this can be added as a first thing. Then resolution could be chosen on that screen to, small or large. Storage can be added in that section too.

@Swiftb0y
Copy link
Member

So maybe we might use to fetch small for the first time and users can choose large to integrate.

Yes, use the most appropriate size for the UI (depending how big the coverart will be displayed) but use the best quality available for actually storing in mixxx.

@daschuer
Copy link
Member

11 MB for a cover is probably overdone for a 5 MB mp3 file. I cannot effort the HDD to double up the size of my collection because of cover arts. So is there a size that fits to the screen size when double clicking the cover? I think I can effort 200 kB or such for a cover.
@Swiftb0y: Do we also need the option to actually use the 11 MB cover art?
If we store it in a separate folder, the user can delete the big once if he is running out of HDD. Would that be worth to consider?

@Swiftb0y
Copy link
Member

Cover arts are usually only that big when their resolution is large (I'd estimate 11Mb to be about 5000x5000 pixels) or there is almost no compression... Rescaling/compressing the cover in mixxx is probably too complicated. I guess we can add a preference option for the upper limit in terms of size/resolution?

@fatihemreyildiz
Copy link
Contributor Author

fatihemreyildiz commented Jul 12, 2022

I have checked with the few cover art responses. The original Image had big resolution, and it can change album release id to album release id. Small sizes (250px) are approx 20-30 KB and large size (500px) are 80-100 KB. 1200 px are approx 500kb.

I think we should use small and large sizes for fetching and applying. I think large size enough for most use cases.

For the higher resolutions (1200px in this case), I'm not sure what to do. Also about the 11 Mb, I think that was only for an album, this can be changed and maybe could be more or less, so it is better not to use original image returned by the coverartarchive.

I want to use small and large sizes for normal use cases as @daschuer mentioned 200 KB is affordable for a cover art. I think that, we can say this could be the normal use case, but for the higher resolutions, (Let's say a user cares about the quality about the cover art a lot) we can fetch the actual image (we don't know how big it could be) by a button (but this time there will be a lot of buttons and that will affect the usability) or a preference setting such as get the highest-large-small. 1200 can be used too it is not in the responses always.

@ronso0
Copy link
Member

ronso0 commented Jul 12, 2022

I think we should offer this options later on after this feature is working. And if we do that we should do in the preference. On preferences there could be section for cover art fetcher.

Yup. Btw I think we should try to split the Library page.

@Swiftb0y
Copy link
Member

We could also make the size of the cover art to be depended on the filesize. Then HQ flacs get HQ cover arts but LQ mp3s get low res cover arts. I think someone with a flac collection cares more about quality in general. Also since the size increase is relative, it wouldn't be that noticeable.

@ronso0
Copy link
Member

ronso0 commented Jul 12, 2022

If we store it in a separate folder, the user can delete the big once if he is running out of HDD. Would that be worth to consider?

I wouldn't make it that complicated. A pref option [small | medium | large] as well as "Store in folder or add to file" should suffice.
For "medium" something between 800 and 1200 is okay, no? choose depending on what's available (if that info is available prior to fetching the image).

@ronso0
Copy link
Member

ronso0 commented Jul 12, 2022

Then HQ flacs get HQ cover arts but LQ mp3s get low res cover arts. I think someone with a flac collection cares more about quality in general.

I do care about audio quality but I don't need 4k cover art : ) Thus I'd prefer a pref option.

@fatihemreyildiz
Copy link
Contributor Author

Yup. Btw I think we should try to split the Library page.

I haven't understand what do you mean by split the library page, sorry. Could you please explain it more?

@Swiftb0y
Copy link
Member

I do care about audio quality but I don't need 4k cover art : ) Thus I'd prefer a pref option.

Right... still I think it would be a shame if one was not able to leverage HQ cover arts when they are available. Since we already have a pref options for image size in pixels and in file size, a third option for specifying a ratio wouldn't be that much more complicated.

@ronso0
Copy link
Member

ronso0 commented Jul 12, 2022

I haven't understand what do you mean by split the library page, sorry. Could you please explain it more?

Just a side note, actually irrelevant here. I mean splitting up Preferences > Library into at least two sub-pages, because we added more options, thus the Library page gets longer and longer and needs to be scrolled and it becomes harder to find certain options.

@fatihemreyildiz
Copy link
Contributor Author

So, I have updated the GUI as I mentioned on this comment. I have left few comments in the new files/classes just for this commit to tell what I wanted to.

I think that on the Tag Fetcher menu it shouldn't be right clicked (cover art menu shouldn't be poped up). In order to do that, I needed another class. But the classes were almost the same, so in order to not duplicate the code, I have made a base class called "CoverArtLabel" (it is on the tag fetcher) and the widget one is derived from the base class. On my test it was working fine. But I'm not sure if I did something good.

I might also need that for dlgcoverartfullsize, because it can also trigger the cover art menu, but in the future this menu can be made specialized to cover art fetcher such as, set this cover art, download this cover art and etc.

Also about the GUI, since I am not an expert, when I resize it, it breaks.

I think that these three parts need a bit refactor before moving to the technical part of this PR.

Thanks in advance for the comments and feedback.


} // anonymous namespace

CoverArtLabel::CoverArtLabel(QWidget* parent)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is still a widget, I had expect to find this is the widget folder.
We two names, one for the base without edit menu and one for the parent class with edit menu.

How about
WCoverArtLabelBase and WCoverArtLabelWithEditMenu

Or

WCoverArtDisplay and WCoverArtDisplayEditable

or such

@ronso0 what do yo think

Copy link
Member

@ronso0 ronso0 Jul 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should allow disabling the menu on demand by adding bool enableMenu (defaults to true) argument to the constructor src/widget/wcoverart.cpp and make some parts of that class conditional via if (m_pMenu) or if (m_menuEnabled), instead of duplicating the entire class.
(if the menu is the only difference, I didn't take a closer look)

Though, as I said this is not high prio IMO and can be adressed later on.

Copy link
Contributor Author

@fatihemreyildiz fatihemreyildiz Jul 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I didn't want to duplicate the whole class, I just took some part of it and made it base class, the other part (wcoverartmenu) is in the derived class.

For now only the menu is the difference, but I can not be sure about the future steps. Maybe later, just for "Fetched Cover Art" there could be a menu such as "apply this cover" or "find the album" etc.

So about making a bool enableMenu or another base class which can derive different classes for different menus. What would be the final version of these? Or should we decide it at the last step for now, it can be like this, or it can be like in the first place.

What do you think generally?

Though, as I said this is not high prio IMO and can be adressed later on.

👍

<item row="4" column="1">
<widget class="QPushButton" name="btnCover">
<property name="text">
<string>&amp;Set Cover Art</string>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a use case to use a cover form a different release than the actual release of the metadata?
If yes, that should be probably rare. So I think I would conciser to just go with a single "Apply" button that applies all metadata at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some use cases which metadata can be applied from results 1 and cover art applied from release 2. But as you mentioned, this is pretty rare. I was thinking about the same, just a single button would look better :)

@fatihemreyildiz
Copy link
Contributor Author

fatihemreyildiz commented Jul 14, 2022

As we mentioned that, GUI can be addressed later on. So I wanted to get your ideas about the displaying fetched cover arts.

What I want to do is add another feature on the existing TagFetcher. So before I ask my questions, I want to basically tell how Tag Fetcher works. This is directly related for the future steps.

As mentioned On the comments in tagfetcher.h There are 3 stages. When user clicks on "Import Metadata from Musicbrainz" these happens:

  1. Chromaprint creates AcoustID fingerprint.
  2. AcoustIDtask returns MusicBrainz UUID's so they can be used to get additional info about the track on Musicbrainz.
  3. MusicBrainzTask gets related metadata of the track with the given UUID from AcoustIDTask.

This system works perfectly, even there is no related info about the track, user is warned. Sometimes it returns just a result or up to 30 results according to my test tracks (Most result I had for a track was In The End by Linkin Park). This is an important information about fetching the cover arts. This is also a fast process comparing to the coverartarchive.

How do I get cover arts of the tracks? By using cover art archive. How this works? It only accepts release id.

  1. I will send request to CoverArtArchive with the existing id of the results, that returns all the links as a j son response. There is a interesting block for me, It might be failed because some ID's returning 404 because they don't have a cover art. But also we need an another step for to retrieve 'actual' cover art.
  2. I will send another request to coverartarchive with the URL of the Image itself. That will return me Byte Array of the image. I think that I can use this to display the cover art on the second fetched cover art cover art label. This task never fails on my tracks. It always returns an image, i think that because some ID's failed at first stage.

The ideas are:

  • The first idea is to fetch cover arts at first time and display right after the menu is opened. This can be achieved by adding these 2 steps after the musicbrainztask succeeded. This has advantages and disadvantages.
    As an advantage, users can see the cover arts without waiting one by one.
    As a disadvantage, this can make the time longer for to get results of the metadata. For the popular songs (as i mentioned earlier In the End) to get metadatas it takes noticeably longer already. If we fetch all the cover arts in the first place I'm afraid this will make to get results longer. Also, coverartarchive is a slower process compared to others.
    Fetching all the cover arts (I assume in the first place we wanted to get small ones) it is 25-30 kbs each, if we have a lot of results would it be a lot to fetch all of them at once?

  • Second idea is, when a result selected via mouse click or hovered by keyboard events, we can start the coverartarchive process. This will not make this 3 stage task longer so menu will be displayed faster, the tasks will be separated. But also, user can sent a lot of request without noticing, for now when I sent few requests one after another, Mixxx crashes. I can fix this by adding aborted I guess. Also if we choose this way, I think we need to have a dummy cover art that can notice users, that fetching on progress, and also another dummy cover art that fetching didn't have a result (this no result also a must for first idea too)

  • I know that we wanted to make this menu without buttons (just apply) but. The last idea about it, (it is really similar to the second one). We add a button for to fetch cover arts, and if the result found user can apply it. It has also a good use-case for the users who doesn't want to get cover arts IMHO.

This became a huge update-question. Sorry for that 😅

What do you think about these ideas?

src/library/dlgtagfetcher.cpp Outdated Show resolved Hide resolved
Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more findings while I was working on DlgCoverArtFullSize UX improvements.

src/library/dlgtagfetcher.cpp Outdated Show resolved Hide resolved
src/library/dlgtagfetcher.cpp Outdated Show resolved Hide resolved
@ronso0 ronso0 added coverart and removed build labels Dec 4, 2022
@ronso0
Copy link
Member

ronso0 commented Dec 4, 2022

@daschuer Could you take another look?
Besides my recent reviews and some manual testing (went well, no issues), I didn't look into CoverArtArchiveLinkTask or CoverArtCopyWorker.

@ronso0 ronso0 mentioned this pull request Dec 5, 2022
1 task
@daschuer
Copy link
Member

daschuer commented Feb 4, 2023

Sorry for the looong delay. I have now find time for a sorrowfully test and find no more issues. This is a good base for more.
Thank you very much for that nice and useful feature.

@daschuer daschuer changed the base branch from main to 2.4 February 4, 2023 22:55
@daschuer
Copy link
Member

daschuer commented Feb 4, 2023

@ronso0: Is your review also addressed?

@ronso0
Copy link
Member

ronso0 commented Feb 6, 2023

Yup, these issues are fixed, ready for merge in that regard.
(note I don't feel confident to Merge since I touched only a small part of this branch)

@daschuer
Copy link
Member

daschuer commented Feb 7, 2023

Thank you for confirming. And thank you Emre for this nice and helpful feature.

@daschuer daschuer merged commit 93e7089 into mixxxdj:2.4 Feb 7, 2023
@fatihemreyildiz
Copy link
Contributor Author

You are welcome Daniel! I'm glad that Mixxx has the cover art fetcher feature now. I hope everyone like this feature.

I would like to also thank to everyone who reviewed the code and gave me feedback along the way. :octocat:

@ronso0 ronso0 added the changelog This PR should be included in the changelog label Feb 8, 2023
@ronso0
Copy link
Member

ronso0 commented Feb 8, 2023

🎉

Do we need some explanation in the manual, somewhere in Library > Edit metadata of audio files?

@daschuer
Copy link
Member

daschuer commented Feb 8, 2023

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This PR should be included in the changelog coverart library ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants