mpris:length type in Metadata

[natsukagami]

https://specifications.freedesktop.org/mpris-spec/2.2/Track_List_Interface.html#Mapping:Metadata_Map

According to the specs, the type of mpris:length field should be signed 64-bit integer. However the library cast this into an unsigned integer.

If the length of the track is known, it should be provided in the metadata property with the "mpris:length" key. The length must be given in microseconds, and be represented as a signed 64-bit integer.

I found out that it is Spotify's behaviour, so maybe modify the code to support both?

[natsukagami]

I implemented a simple fix in 088c41b3da16f4e0f21b79a22d4997ee75a43c0a if you're interested to backport to v1.1.0.

[Mange]

Spotify seems to work for me. What exactly is the problem for you? Is it returning a negative number, or is it not loading for you?

I figured that unsigned made more sense (it integrates directly with Duration, and what would a negative length be?) and assumed that "signed" were either because that is more common than unsigned in C/C++-land (some style guides forbids unsigned for some reason), or because they wanted -1 as a sentinel value for "unlimited length" or something like that, which I much rather encode as an enum.

I'm asking you this because I want to understand the problem. Changing this type is a breaking change, so right before 2.0 is finished is an excellent time to look this one over. :)

No matter what, I'm also considering having a method that returns the "raw" value (raw_length?) and one that returns the more high-level representation of it (length_in_microseconds?).

[natsukagami]

The cast function from dbus crate interprets the input type based on the output type. Therefore when you cast the raw length value it tries to interpret the DBus value (which is typed) as a u64 and hence ignore the value if it was an i64.

Instead of introducing a breaking change you can try to interpret both as u64 and i64 like I did. Since the spec required the number to be i64 typed, I suspect there are more implementations that would use i64 instead of u64.

On Thu., Dec. 13, 2018, 2:38 a.m. Magnus Bergmark <notifications@github.com wrote:

Spotify seems to work for me. What exactly is the problem for you? Is it returning a negative number, or is it not loading for you?

I figured that unsigned made more sense (it integrates directly with Duration, and what would a negative length be?) and assumed that "signed" were either because that is more common than unsigned in C/C++-land (some style guides forbids unsigned for some reason), or because they wanted -1 as a sentinel value for "unlimited length" or something like that, which I much rather encode as an enum.

I'm asking you this because I want to understand the problem. Changing this type is a breaking change, so right before 2.0 is finished is an excellent time to look this one over. :)

No matter what, I'm also considering having a method that returns the "raw" value (raw_length?) and one that returns the more high-level representation of it (length_in_microseconds?).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Mange/mpris-rs/issues/40#issuecomment-446871052, or mute the thread https://github.com/notifications/unsubscribe-auth/AIpFaRuV3oSJUX0JgmB3gAtQQgW6qWzkks5u4gPkgaJpZM4ZMZTn .

[Mange]

Thank you. That makes a lot of sense. I'll look your commit over the next time I have some dev time!

Den tor 13 dec. 2018 15:36 skrev Natsu Kagami notifications@github.com:

The cast function from dbus crate interprets the input type based on the output type. Therefore when you cast the raw length value it tries to interpret the DBus value (which is typed) as a u64 and hence ignore the value if it was an i64.

Instead of introducing a breaking change you can try to interpret both as u64 and i64 like I did. Since the spec required the number to be i64 typed, I suspect there are more implementations that would use i64 instead of u64.

On Thu., Dec. 13, 2018, 2:38 a.m. Magnus Bergmark < notifications@github.com wrote:

Spotify seems to work for me. What exactly is the problem for you? Is it returning a negative number, or is it not loading for you?

I figured that unsigned made more sense (it integrates directly with Duration, and what would a negative length be?) and assumed that "signed" were either because that is more common than unsigned in C/C++-land (some style guides forbids unsigned for some reason), or because they wanted -1 as a sentinel value for "unlimited length" or something like that, which I much rather encode as an enum.

I'm asking you this because I want to understand the problem. Changing this type is a breaking change, so right before 2.0 is finished is an excellent time to look this one over. :)

No matter what, I'm also considering having a method that returns the "raw" value (raw_length?) and one that returns the more high-level representation of it (length_in_microseconds?).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Mange/mpris-rs/issues/40#issuecomment-446871052, or mute the thread < https://github.com/notifications/unsubscribe-auth/AIpFaRuV3oSJUX0JgmB3gAtQQgW6qWzkks5u4gPkgaJpZM4ZMZTn

.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/Mange/mpris-rs/issues/40#issuecomment-446989968, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAGP-kjrsHX640nDTzs7TfdaFTDx9djks5u4mXwgaJpZM4ZMZTn .

[Mange]

When using the official Spotify Linux client I only get back u64 values. If I only load i64, then the cast fails. However, I can replicate the problem you are seeing with VLC. Then the length fails to load without looking for an i64.

I'll push a fix to the 1.1.x branch and also apply it to the 2.0.x branch.

[Mange]

I released version 1.1.1 with this fix included, and it's also slated for the 2.0.0 release. Thank you so much for reporting this! :heart: