-
Notifications
You must be signed in to change notification settings - Fork 425
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
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
a47d4ee
Modify sample-track.mp3 to contain id3 tags
naoey 57f878c
Add a `Tags` field to `Track` & read tags from Bass
naoey 7c236ef
Expose tags via `ITrack`
naoey 181d81d
Add null checks
naoey 3f1cb71
Update osu.Framework/Audio/Track/Track.cs
naoey 3309669
Remove BPM becuase seems to be rarely used
naoey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence. | ||
// See the LICENCE file in the repository root for full licence text. | ||
|
||
namespace osu.Framework.Audio.Track | ||
{ | ||
/// <summary> | ||
/// A container for audio track metadata. | ||
/// </summary> | ||
public class Tags | ||
{ | ||
public string? Title { get; set; } | ||
|
||
public string? Artist { get; set; } | ||
|
||
public string? Album { get; set; } | ||
|
||
public int? Year { get; set; } | ||
|
||
public string? Genre { get; set; } | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,8 @@ public abstract class Track : AdjustableAudioComponent, ITrack, IAudioChannel | |
|
||
public virtual bool Looping { get; set; } | ||
|
||
public Tags? Tags => GetTags(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
public string Name { get; } | ||
|
||
protected Track(string name) | ||
|
@@ -119,6 +121,8 @@ public virtual double Rate | |
|
||
public virtual ChannelAmplitudes CurrentAmplitudes { get; } = ChannelAmplitudes.Empty; | ||
|
||
protected virtual Tags? GetTags() => null; | ||
|
||
protected override void UpdateState() | ||
{ | ||
FrameStatistics.Increment(StatisticsCounterType.Tracks); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 butTrackBass
.Alternative would be something like
which could only be applied to
TrackBass
, but that likely causes trouble withDrawableTrack
(the inner track is private, so you wouldn't be able to access the tags otherwise).There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.