implement reading of rating tag
ClosedPublic

Authored by astippich on Apr 12 2018, 5:57 PM.

Details

Summary

Allow to read the rating stored in the metadata for idv2, vorbis and ape tags. Since the value ranges all differ, translate them to a range from 0-10 like we use for xattr ratings

Diff Detail

Repository
R286 KFileMetaData
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
astippich created this revision.Apr 12 2018, 5:57 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptApr 12 2018, 5:57 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
astippich requested review of this revision.Apr 12 2018, 5:57 PM
astippich edited the summary of this revision. (Show Details)Apr 12 2018, 6:13 PM

Thanks.
Fix one issue and we should be good to go.
Another nice thing is that we should have a portable way to have ratings in Elisa that could be read or write somewhere else.

src/extractors/taglibextractor.cpp
221–233

You can and should be using all intermediate values. If I remember correctly, in dolphin, you can set all values between 0 and 10. This is done by half blue stars.
Can you use modulo instead of embedded if ?

mgallien requested changes to this revision.Apr 18 2018, 8:15 PM
This revision now requires changes to proceed.Apr 18 2018, 8:15 PM

Thanks.
Fix one issue and we should be good to go.
Another nice thing is that we should have a portable way to have ratings in Elisa that could be read or write somewhere else.

Writing the tags must be implemented first for that :)

src/extractors/taglibextractor.cpp
221–233

The thing here is that the most common implementation don't allow half star ratings. Only Mediamonkey does it and that diverges from the rest. Also, the values are not distributed equidistantly, but I'll look into a more flexible solution.

astippich updated this revision to Diff 32587.Apr 19 2018, 5:55 PM
  • implement reading of half star ratings for mp3 and add tests
bruns added a subscriber: bruns.Apr 19 2018, 5:56 PM
bruns added inline comments.
src/extractors/taglibextractor.cpp
221–233

There is a "half interval" at the bottom and the top, every other interval is 64.

rating = temp * 10 / 255 + 0.5;

should work reasonably well, or, if you prefer a smaller 0 interval:

rating = temp * 9.5 / 255 + 0.99;

code didn't get prettier, but that's the de-facto standard

astippich updated this revision to Diff 32591.Apr 19 2018, 6:28 PM
  • simplify

Those formulas didn't work with newly added tests with values from Winamp, Windows Media Player and MediaMonkey. I calculated a linear regression line and this one seems to work with some special case handling

It's for elisa I guess, could you please elaborate how POPM/RATING is going to be used and why xattr are not applicable?

It's for elisa I guess, could you please elaborate how POPM/RATING is going to be used and why xattr are not applicable?

It will be used as a fallback when there is no xattr rating set or available (e.g. on Windows) so that users who have rated their music with other players can still see their previous ratings.
It will also useful for managing ratings on other platforms when writing support is added.

michaelh added a comment.EditedApr 19 2018, 6:53 PM

It's for elisa I guess, could you please elaborate how POPM/RATING is going to be used and why xattr are not applicable?

It will be used as a fallback when there is no xattr rating set or available (e.g. on Windows) so that users who have rated their music with other players can still see their previous ratings.
It will also useful for managing ratings on other platforms when writing support is added.

I that case I would suggest to use xattr only on systems that support it, and use POPM/RATING on those that don't. I afraid users will find it confusing to have two ways of rating a file. Or is rating via dolphin/baloo-widgets not planned?
Because extractors are called in sequence it would also be possible to create a dedicate taglibRatingExtractor (much easier to apply build conditionals).

It's for elisa I guess, could you please elaborate how POPM/RATING is going to be used and why xattr are not applicable?

It will be used as a fallback when there is no xattr rating set or available (e.g. on Windows) so that users who have rated their music with other players can still see their previous ratings.
It will also useful for managing ratings on other platforms when writing support is added.

I that case I would suggest to use xattr only on systems that support it, and use POPM/RATING on those that don't. I afraid users will find it confusing to have two ways of rating a file. Or is rating via dolphin/baloo-widgets not planned?
Because extractors are called in sequence it would also be possible to create a dedicate taglibRatingExtractor (much easier to apply build conditionals).

We still need the ability to read the tag, since users expect to see the rating in music players.
For baloo-widgets I created D12157

mgallien requested changes to this revision.Apr 23 2018, 9:00 PM

It's for elisa I guess, could you please elaborate how POPM/RATING is going to be used and why xattr are not applicable?

It will be used as a fallback when there is no xattr rating set or available (e.g. on Windows) so that users who have rated their music with other players can still see their previous ratings.
It will also useful for managing ratings on other platforms when writing support is added.

I that case I would suggest to use xattr only on systems that support it, and use POPM/RATING on those that don't. I afraid users will find it confusing to have two ways of rating a file. Or is rating via dolphin/baloo-widgets not planned?
Because extractors are called in sequence it would also be possible to create a dedicate taglibRatingExtractor (much easier to apply build conditionals).

We still need the ability to read the tag, since users expect to see the rating in music players.
For baloo-widgets I created D12157

If I have correctly understood the ideas behind the conception of Baloo, we should probably prefer to store the rating with a "native" solution instead of the xattr one that is not portable outside of software using KFileMetaData.

We have to stay compatible with older versions of KFileMetaData. So we should push the same rating to both properties and try to read it from both properties in the cases where only one exists.

Could you try to modify your patch to do that ?

This revision now requires changes to proceed.Apr 23 2018, 9:00 PM

It's for elisa I guess, could you please elaborate how POPM/RATING is going to be used and why xattr are not applicable?

It will be used as a fallback when there is no xattr rating set or available (e.g. on Windows) so that users who have rated their music with other players can still see their previous ratings.
It will also useful for managing ratings on other platforms when writing support is added.

I that case I would suggest to use xattr only on systems that support it, and use POPM/RATING on those that don't. I afraid users will find it confusing to have two ways of rating a file. Or is rating via dolphin/baloo-widgets not planned?
Because extractors are called in sequence it would also be possible to create a dedicate taglibRatingExtractor (much easier to apply build conditionals).

We still need the ability to read the tag, since users expect to see the rating in music players.
For baloo-widgets I created D12157

If I have correctly understood the ideas behind the conception of Baloo, we should probably prefer to store the rating with a "native" solution instead of the xattr one that is not portable outside of software using KFileMetaData.

We have to stay compatible with older versions of KFileMetaData. So we should push the same rating to both properties and try to read it from both properties in the cases where only one exists.

Could you try to modify your patch to do that ?

Your interpretation is correct, Baloo works as a cache, storing data in a normalized form, to speed up lookup. It is not a permanent data store.

Storing/updating metadata inside the file has the benefit it works independent of the filesystem capabities. On the other hand, one risks damaging the file, and synchronization between devices may become more difficult.

If I have correctly understood the ideas behind the conception of Baloo, we should probably prefer to store the rating with a "native" solution instead of the xattr one that is not portable outside of software using KFileMetaData.

We have to stay compatible with older versions of KFileMetaData. So we should push the same rating to both properties and try to read it from both properties in the cases where only one exists.

Could you try to modify your patch to do that ?

I don't understand. Which patch to you want me to modify? This one here only adds the ability to read the rating embedded in the tags in addition to the xattr rating. It's up to the application to decide what to do with the information.
The patch for baloo-widgets just hides this new rating tag to avoid duplicate entries in e.g. dolphin, and actually preserves the current behavior this way. Baloo caches the embedded rating anyways.
When KFileMetaData gains the ability to write the tags, one should write to both properties, but that is currently not possible.

bruns added a comment.May 3 2018, 9:01 PM

If I have correctly understood the ideas behind the conception of Baloo, we should probably prefer to store the rating with a "native" solution instead of the xattr one that is not portable outside of software using KFileMetaData.

We have to stay compatible with older versions of KFileMetaData. So we should push the same rating to both properties and try to read it from both properties in the cases where only one exists.

Could you try to modify your patch to do that ?

I don't understand. Which patch to you want me to modify? This one here only adds the ability to read the rating embedded in the tags in addition to the xattr rating. It's up to the application to decide what to do with the information.
The patch for baloo-widgets just hides this new rating tag to avoid duplicate entries in e.g. dolphin, and actually preserves the current behavior this way. Baloo caches the embedded rating anyways.
When KFileMetaData gains the ability to write the tags, one should write to both properties, but that is currently not possible.

I see three problems here:

  • The current "Rating" is handled specially, you can e.g. select Rating: 4 or more in Dolphins "Find ..." dialog. If I have a file with an embeddedrating of 4.5 (normalized), I would expect it to show up. Probably the right thing would be to teach Baloo to treat embeddedrating the same way as a rating stored in an xattr, although I am not sure.
  • Inconsistent data - the xattr rating may differ from the embedded rating. The one from the xattr should likely be preferred, but what is exposed in the file information widget?
  • Writing/updating the rating - we should remove any xattr rating, to avoid any ambiguities which one is the current one.

The XAttr rating is retrieved with the basicindexingjob, and there is currently no possibility to "merge" data from the extractors in a different way than just creating the union. So in the database we have either

  • Two, probably different, "rating" tags. Searching would match if any matches, and the widget would show an arbitrary one
  • Two independent tags. Every search for a rating becomes if ("rating" == r OR "embeddedrating" == r)

If I have correctly understood the ideas behind the conception of Baloo, we should probably prefer to store the rating with a "native" solution instead of the xattr one that is not portable outside of software using KFileMetaData.

We have to stay compatible with older versions of KFileMetaData. So we should push the same rating to both properties and try to read it from both properties in the cases where only one exists.

Could you try to modify your patch to do that ?

I don't understand. Which patch to you want me to modify? This one here only adds the ability to read the rating embedded in the tags in addition to the xattr rating. It's up to the application to decide what to do with the information.
The patch for baloo-widgets just hides this new rating tag to avoid duplicate entries in e.g. dolphin, and actually preserves the current behavior this way. Baloo caches the embedded rating anyways.
When KFileMetaData gains the ability to write the tags, one should write to both properties, but that is currently not possible.

I would like to have only one rating in the KFileMetaData API that would transparently use the most appropriate way to store and read the rating.
That would be:

  • music audio file with a rating tag = we read the tag and write both tag and xattr attribute to maximize compatibility ;
  • music audio file with only xattr rating attribute = we read the xattr attribute and write both tag and xattr attribute to maximize compatibility ;
  • any file with only xattr rating attribute = we read the xattr attribute and write the xattr attribute ;

Does this sound reasonable and feasible ?

I agree, there are several problems coming from the fact that we basically have two sources for the same property, which can potentially conflict. Btw, we already have the same issue with the comment property.

I see three problems here:

  • The current "Rating" is handled specially, you can e.g. select Rating: 4 or more in Dolphins "Find ..." dialog. If I have a file with an embeddedrating of 4.5 (normalized), I would expect it to show up. Probably the right thing would be to teach Baloo to treat embeddedrating the same way as a rating stored in an xattr, although I am not sure.
  • Inconsistent data - the xattr rating may differ from the embedded rating. The one from the xattr should likely be preferred, but what is exposed in the file information widget?

I want to leave that decision up to the application, and this patch is actually the first step towards that.

  • Writing/updating the rating - we should remove any xattr rating, to avoid any ambiguities which one is the current one.

I think one should write to both properties, so that existing applications relying on xattr continue to work. Also keep in mind that (at least with this patch) there are only rating tags for audio files, I don't know if such things exist for other documents.

The XAttr rating is retrieved with the basicindexingjob, and there is currently no possibility to "merge" data from the extractors in a different way than just creating the union. So in the database we have either

  • Two, probably different, "rating" tags. Searching would match if any matches, and the widget would show an arbitrary one
  • Two independent tags. Every search for a rating becomes if ("rating" == r OR "embeddedrating" == r)

Hmm, haven't really thought about searching in baloo. But imho we should keep both ratings, since we don't know which is the "right" or more important one.

I would like to have only one rating in the KFileMetaData API that would transparently use the most appropriate way to store and read the rating.

xattr tags have a completely different code path than the extractors. One could wire that up, but this would need to adapt all extractors and to teach the applications using KFileMetaData not to query the xattr separately anymore.

That would be:

  • music audio file with a rating tag = we read the tag and write both tag and xattr attribute to maximize compatibility ;
  • music audio file with only xattr rating attribute = we read the xattr attribute and write both tag and xattr attribute to maximize compatibility ;
  • any file with only xattr rating attribute = we read the xattr attribute and write the xattr attribute ;

    Does this sound reasonable and feasible ?

That was the plan anyway, but to let this be handled in the applications. I think this is especially important if we have conflicting ratings, and the user should be able to decide which one to use. I don't think that a library such as KFileMetaData should do that decision, it should present all available information.

Let me know what you think. I prefer to let KFileMetaData just present all information and let the applications handle the difficulties. If you guys think that this is not the way to go, I can also try to implement this inside KFileMetaData.

astippich updated this revision to Diff 34398.May 17 2018, 8:21 PM
  • rebase on master
  • implement rating tag for m4a
Restricted Application edited subscribers, added: Baloo, kde-frameworks-devel; removed: Frameworks. · View Herald TranscriptMay 17 2018, 8:21 PM

any comments on the previous discussion?

mgallien added a subscriber: cgilles.EditedMay 22 2018, 8:25 PM

@cgilles Given digikam seems using KFileMetaData (seems correct if one looks at the Debian package dependencies) , I have a coupleof questions.
This diff is about adding a way to read ratings from "tags" native to a specific file format.

This would be in addition to the current ratings that are independent from the file types.

In digikam, it is really easy to rate a photograph. I suppose this patch would need to be extended to support (seems like some definition exists in IPTC standard) the rating embedded in the image metadata.

Can you comment on this patch with a digikam point of view ?

I am wondering if we should not find a way to make this more transparent for users of the API (like only one entry point for ratings). This is why I am asking here.

astippich updated this revision to Diff 35242.May 31 2018, 9:52 AM
  • update comments
mgallien accepted this revision.May 31 2018, 3:32 PM

I hope application developers will not complain about the need to support a second api to get ratings. I am still unsure this the best way to add this support.

This revision is now accepted and ready to land.May 31 2018, 3:32 PM
This revision was automatically updated to reflect the committed changes.