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

A lingering TagLib problem #1

Closed
fictionic opened this issue Aug 16, 2017 · 13 comments
Closed

A lingering TagLib problem #1

fictionic opened this issue Aug 16, 2017 · 13 comments

Comments

@fictionic
Copy link

Hi again! It's dylandforbes from Bitbucket.

I'm finally getting around to following up on this issue. While Demlo does a decent job at deciding when to use TagLib and when to use FFmpeg, it's not perfect; there is one crucial missing case involving date. It took some investigating, but I figured out that the problem is that TagLib doesn't support setting the date tag to an arbitrary string, only to an integer. As I like to store dates in ISO 8601 format, this is crucial for me. Unfortunately, Demlo does not take this into account when deciding between TagLib and FFmpeg.

Thus the fix is to force use of FFmpeg when type(o.date) == 'string'. Should be simple enough.

Thanks!

@fictionic fictionic changed the title Some lingering TagLib problems A lingering TagLib problem Aug 16, 2017
@Ambrevar
Copy link
Owner

Good catch! e0449bd should address this issue. Let me know.

Off-topic: Why did you submit here and not on Bitbucket?
That was a lucky hunch I guess, as I was planning to move to GitHub :)

@fictionic
Copy link
Author

It works!

I submitted here because I noticed a while ago that you had put Demlo up here on GitHub, and I figured you were making the transition. On that topic, is go get bitbucket.org/ambrevar/demlo still the correct way to download the source? If I do go get github.com/ambrevar/demlo it also works but it downloads the Bitbucket copy as well...

@Ambrevar
Copy link
Owner

Ambrevar commented Sep 2, 2017

Great!

I'll finish the GitHub transition when I'll release the next version.
Demlo has a few sub-packages that are pointed to by their Bitbucket URI, which is why you get both versions when you try to go get the GitHub version. I'll change those URI with the GitHub release.

@Ambrevar Ambrevar closed this as completed Sep 2, 2017
@fictionic
Copy link
Author

Following up again! This issue also manifests itself for the disc and track tags. When editing those tags in-place, they always end up as non-zero-padded ints, or empty strings, if they cannot be cast to integers. People certainly often want track numbers to be zero-padded, and I see no reason why disc number should be the exception: for all three tags that TagLib wants to be integers (date, track, and disc), Demlo should check to see if the user wants to set it to a string, and bypass TagLib if so.

@Ambrevar
Copy link
Owner

I'm not sure about this one.

Zero-padding is useful for filenames, but tags need not be zero-padded for filenames to be zero-padded.
Demlo can insert padding in filenames with the path.lua script.
Same goes for discs.

If your concern is for display in music players, I believe the job of padding zeros belongs to the music player display. To be honest, I do not know why you'd want to pad zeros in a music player display.

Tags should store information, not appearance, i.e. they should store the track actual number, not some formatting of it.
This way, Demlo abides by its own goals: clean up tags and ensure consistency. All tracks will be numbered the same way.

Another use for track numbers / disc numbers is to store the total number, e.g. "1/10" if it's the first track out of ten.
In terms of data structure, this is terrible though: the total number of tracks should be stored in a separate tag.
It would be much more flexible and easy to manipulate. Music players can then easily adapt the display and demlo can easily generate file names without having to parse the track number field.
I do not know if there is a standard field for this though.

@fictionic
Copy link
Author

fictionic commented Nov 21, 2017

Zero-padding is useful for filenames, but tags need not be zero-padded for filenames to be zero-padded. Demlo can insert padding in filenames with the path.lua script. Same goes for discs.

This is true...

If your concern is for display in music players, I believe the job of padding zeros belongs to the music player display. ... Tags should store information, not appearance, i.e. they should store the track actual number, not some formatting of it.

...However, the problem is that not all music players do that. The only way to guarantee that it'll show up consistently is to have the data be stored consistently. In an ideal world you'd be right.

Another use for track numbers / disc numbers is to store the total number, e.g. "1/10" if it's the first track out of ten. In terms of data structure, this is terrible though: the total number of tracks should be stored in a separate tag. It would be much more flexible and easy to manipulate. Music players can then easily adapt the display and demlo can easily generate file names without having to parse the track number field.

I've always gotten rid of tracktotal tags, since I didn't see the point of them—I knew how many tracks there were without being told! But now that I think about it, this would be the best way to do it. In an ideal world, all music files would have a non-zero-padded track tag and a tracktotal tag (or some standard name, see below), and similar for disc and disctotal, and the music player would insert the appropriate amount of zero-padding. But alas that's not how things work. Many players will show the track as <track>/<tracktotal>, which I find revolting.

I do not know if there is a standard field for this though.

Yeah that is a problem. I think ID3 has a standard one, but there are probably plenty of Vorbis Comment names in use: tracktotal, track_total, totaltracks, total_tracks, etc. I consider this the job of a Demlo script to standardize :).

(Continued below)

@fictionic
Copy link
Author

This way, Demlo abides by its own goals: clean up tags and ensure consistency. All tracks will be numbered the same way.

That is a good argument. Consider this for a counter, however.

The behavior of Demlo in this matter is already inconsistent, simply because of differences between TagLib and FFmpeg. If Demlo uses TagLib to edit tags, then the tracknumber will be a non-padded integer; but if it uses FFmpeg (which is sometimes unavoidable), then the tracknumber will be set to whatever the user asks for, assuming the output format allows it—that is, there is another layer of inconsistency, because the MP4 container does not allow arbitrary tag contents for all tags (probably what motivated TagLib's restriction(?)).

My proposal is to

  1. Try to do what the user asks as much as possible—by checking output.tags to see if ffmpeg is required, and
  2. Be as informative as possible to the user about what is going on—by letting them know (a) when FFmpeg is being used instead of TagLib because of their computed tags (Demlo already does this (per this very issue), though it doesn't mention the reason for its behavior), and (b) when their desired tags cannot be stored verbatim into the output file in the desired format at all, because of restrictions imposed by the format.

I see two variants of the latter situation, for each problematic tag: (i) When the tag can be coerced into the format (by removing leading 0s)—here I think Demlo should proceed; and (ii) when the tag cannot be so coerced, in which case I think Demlo should exit... Or maybe it should set an empty tag in the output (which is what most other software does). Either way it should be verbose (and not require -debug). Maybe it should be another setting? You probably wouldn't want that... "-forceverbatim" maybe?


This strikes me as the most consistent route to take that also gives users what they want. Anything would be giving special treatment to track and disc over date.

Oh, and as for why someone might want non-integers in tracknumbers: A1, A2, .... Yeah, it's not for me, but I've seen it (and ncmpcpp didn't like it).

@Ambrevar
Copy link
Owner

Yeah that is a problem. I think ID3 has a standard one, but there are probably plenty of Vorbis Comment names in use: tracktotal, track_total, totaltracks, total_tracks, etc. I consider this the job of a Demlo script to standardize :).

Do you mean Demlo should impose its standard by setting some Vorbis comment such as "tracktotal" when a specific tag is set, for instance o.tracktotal?
Considering how marginal this is at the moment (neither you nor I use this feature), I don't think I'll go that way.

The resulting output of FFmpeg or TagLib should be the same.
The fact that it is not is a bug. I'll fix this.

FFmpeg and TagLib are merely implementation details and the end user need not know about it beyond -debug.
I think adding some other -verbose options would add unnecessary complexity.

Regarding the argument of players with bad interfaces: well, it's not Demlo's job to fix them all.

Considering there seems to be no good solution, I do understand the need for more flexible disc/track tags.
I'll turn them to strings then.

Let me know what you think and if you have no further comment, I'll push a fix.

@fictionic
Copy link
Author

Do you mean Demlo should impose its standard by setting some Vorbis comment such as "tracktotal" when a specific tag is set, for instance o.tracktotal?
Considering how marginal this is at the moment (neither you nor I use this feature), I don't think I'll go that way.

No, I mean it's the job of a Demlo user to customizer their Demlo user scripts to pick one tag and use it. Which is what I currently do.

Considering there seems to be no good solution, I do understand the need for more flexible disc/track tags.
I'll turn them to strings then.

Awesome!

There is one other perhaps-related bug involving those tags that I've been meaning to submit. For some reason, the only way to clear the disc and track tags is to set them to 0—setting them to nil does not work, and neither does setting them to an empty string. This shows up in the preview output. It's also potentially problematic if the user desires the content of those tags to be '0'. I believe it is a TagLib issue, but the fact that it messes up the preview output is confusing. Basically, FFmpeg should be used whenever the disc and track tags cannot be parsed as positive integers. And setting them to nil (and to "") should clear them, just like the other tags.

@Ambrevar Ambrevar reopened this Nov 25, 2017
@Ambrevar
Copy link
Owner

Sorry for the delay, I'll come back to this as soon as possible.

As a side note, you mentioned the disc field in this issue, but as far as Demlo is concerned, disc is not handled by TagLib and thus should not suffer from the integer conversion issue. Am I wrong here?

@fictionic
Copy link
Author

Hm. You're right. Not sure how I missed that in my testing.

@Ambrevar
Copy link
Owner

Done, let me know it that works as you expected.

And sorry for the long delay.

@fictionic
Copy link
Author

It all works great! Thanks!

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

No branches or pull requests

2 participants