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

Retrieve tags from Bass tracks when available #5627

Closed
wants to merge 6 commits into from
Closed

Conversation

naoey
Copy link
Contributor

@naoey naoey commented Jan 16, 2023

For use in ppy/osu#21189. Only retrieves a subset of all the tags read by Bass that seemed relevant to osu!.

Comment on lines +49 to +52
/// <summary>
/// The ID3 tags contained in this track's metadata.
/// </summary>
Tags? Tags { get; }
Copy link
Collaborator

@bdach bdach Jan 16, 2023

Choose a reason for hiding this comment

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

I'm really not sure if we want this on Track. Especially so if it's going to not be implemented for anything but TrackBass.

Alternative would be something like

public interface ITaggedTrack
{
    Tags Tags { get; }
}

which could only be applied to TrackBass, but that likely causes trouble with DrawableTrack (the inner track is private, so you wouldn't be able to access the tags otherwise).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made sense in my head as tags can be present on the track regardless of implementation (Bass or otherwise).

DrawableTrack's inner track could probably be public since it's already readonly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, but layering wise I'm not sure it fits smack dab in the middle of the rest of the audio subsystem.

I'm just mostly pondering. Need more opinions on this for sure before proceeding in any particular direction.

Copy link
Member

@peppy peppy Jan 17, 2023

Choose a reason for hiding this comment

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

I remember there being an overhead with tag retrieval, in stable it’s only requested in the editor to avoid this.
Will need to check if this is still the case.

On actually inspecting the code, it looks to be on-demand so this shouldn't be an issue.

osu.Framework/Audio/Track/Track.cs Outdated Show resolved Hide resolved
if (string.IsNullOrEmpty(parsed.Artist))
parsed.Artist = tags.AlbumArtist;

if (Int32.TryParse(tags.BPM, out int bpm))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (Int32.TryParse(tags.BPM, out int bpm))
if (int.TryParse(tags.BPM, out int bpm))

Also what's the guarantee that this is gonna parse as int? What about fractional BPM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hadn't considered that, on second thought not even sure this is worth having because it doesn't seem to be populated very commonly.

@bdach
Copy link
Collaborator

bdach commented Jan 16, 2023

Also please check code style.

@@ -23,6 +23,8 @@ public abstract class Track : AdjustableAudioComponent, ITrack, IAudioChannel

public virtual bool Looping { get; set; }

public Tags? Tags => GetTags();
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to go as far as having a property, it should probably cache so they are only retrieved once.

bdach added a commit to bdach/osu that referenced this pull request Dec 27, 2024
…editor

- Closes ppy#21189
- Supersedes / closes ppy/osu-framework#5627
- Supersedes / closes ppy#22235

The reason why I opted for a complete rewrite rather than a revival of
that aforementioned pull series is that it always felt quite gross to me
to be pulling framework's audio subsystem into the task of reading ID3
tags, and I also partially don't believe that BASS is *good* at reading
ID3 tags. Meanwhile, we already have another library pulled in that is
*explicitly* intended for reading multimedia metadata, and using it
does not require framework changes. (And it was pulled in explicitly for
use in the editor verify tab as well.)

The hard and dumb part of this diff is hacking the gibson such that
the metadata section on setup screen actually *updates itself*
after the resources section is done doing its thing. After significant
gnashing of teeth I just did the bare minimum to make work by caching
a common parent and exposing an `Action?` on it. If anyone has better
ideas, I'm all ears.
TheDark98 pushed a commit to TheDark98/osu that referenced this pull request Jan 14, 2025
…editor

- Closes ppy#21189
- Supersedes / closes ppy/osu-framework#5627
- Supersedes / closes ppy#22235

The reason why I opted for a complete rewrite rather than a revival of
that aforementioned pull series is that it always felt quite gross to me
to be pulling framework's audio subsystem into the task of reading ID3
tags, and I also partially don't believe that BASS is *good* at reading
ID3 tags. Meanwhile, we already have another library pulled in that is
*explicitly* intended for reading multimedia metadata, and using it
does not require framework changes. (And it was pulled in explicitly for
use in the editor verify tab as well.)

The hard and dumb part of this diff is hacking the gibson such that
the metadata section on setup screen actually *updates itself*
after the resources section is done doing its thing. After significant
gnashing of teeth I just did the bare minimum to make work by caching
a common parent and exposing an `Action?` on it. If anyone has better
ideas, I'm all ears.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants