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

[GetMetaTag] Added the Mix_GetMetaTag function. #36

Closed
wants to merge 2 commits into from

Conversation

SNSTRUTHERS
Copy link

A quick-and-dirty way to get meta tags from a Mix_Music pointer. I added it for a project I'm working on.

A quick-and-dirty way to get meta tags from a Mix_Music pointer. Added
it for a project I'm working on.
@Wohlstand
Copy link
Member

Wohlstand commented Jul 21, 2020

@sezero , do you think yourself is this a good thing?

@Wohlstand
Copy link
Member

A think looks interesting 🦊
However, please remove "snstruthers" note and leave comments just "Mixer-X" 🦊

@SNSTRUTHERS
Copy link
Author

Yeah yeah. I wasn't thinking of doing a pull request at first, hence the named comments.

I'm away from my computer at the moment (am typing on phone), so I'll have to do that later.

src/music.h Outdated
@@ -23,6 +23,9 @@
#ifndef MUSIC_H_
#define MUSIC_H_

#define LOOP_DEBUG_PRINTOUT 1
/*#define LOOP_DEBUG_PRINTOUT 0*/
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to keep this here. Please make the CMake side option to turn this on/off 👀

Copy link
Author

Choose a reason for hiding this comment

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

👍

@Wohlstand
Copy link
Member

Yeah yeah. I wasn't thinking of doing a pull request at first, hence the named comments.

I'm away from my computer at the moment (am typing on phone), so I'll have to do that later.

No worry, make any necessary changes once you can 😉

@SNSTRUTHERS
Copy link
Author

No worry, make any necessary changes once you can 😉

Thanks :^)

@@ -547,6 +547,7 @@ Mix_MusicInterface Mix_MusicInterface_GME =
NULL, /* LoopEnd [MIXER-X]*/
NULL, /* LoopLength [MIXER-X]*/
GME_GetMetaTag,/* GetMetaTag [MIXER-X]*/
NULL, /* GetUserTa [MIXER-X-snstruthers]*/
Copy link
Member

Choose a reason for hiding this comment

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

A typo: GetUserTa. Did you meant the GetUserTag?

Copy link
Author

Choose a reason for hiding this comment

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

LOL

@sezero
Copy link
Collaborator

sezero commented Jul 21, 2020

@sezero , do you think it's good yourself?

The debug print stuff pollutes the whole thing badly.

Apart from that, I think it needs explanation as to how it differs from other metadata functionality, whether it can be merged with the existing metatags stuff etc.

@SNSTRUTHERS
Copy link
Author

The debug print stuff pollutes the whole thing badly.

Apart from that, I think it needs explanation as to how it differs from other metadata functionality, whether it can be merged with the existing metatags stuff etc.

That was me forgetting to turn that flag off. I'll change it to a CMAKE flag later.

@Wohlstand
Copy link
Member

@sezero , do you think it's good yourself?

The debug print stuff pollutes the whole thing badly.

Apart from that, I think it needs explanation as to how it differs from other metadata functionality, whether it can be merged with the existing metatags stuff etc.

That would require some sort of associative array at the to keep all meta-tags at the Mix_MusicMetaTags for a quick access without of necessary to make one more extra call entry inside of Mix_MusicInterface. That should make change being majorely smaller and be more universal. I.e. While loading song, put all tags into Mix_MusicMetaTags structure's field (all unhandled tags should use a generic assicoative key-value array), and when requesting an every tag, just access the structure instance instead of calling API of a codec.

@Wohlstand Wohlstand requested a review from sezero July 21, 2020 07:50
@SNSTRUTHERS
Copy link
Author

That would require some sort of associative array at the to keep all meta-tags at the Mix_MusicMetaTags for a quick access without of necessary to make one more extra call entry inside of Mix_MusicInterface. That should make change being majorely smaller and be more universal. I.e. While loading song, put all tags into Mix_MusicMetaTags structure's field (all unhandled tags should use a generic assicoative key-value array), and when requesting an every tag, just access the structure instance instead of calling API of a codec.

I was thinking of doing that, but

  1. I don't know of the intricacies regarding most of these file formats and how they store metadata.
  2. I didn't want to add a hashmap library to the codebase.
  3. The thing I'm working on that makes use of this extra feature only really needs to read the tag once, and that's it. Wasn't really worried about efficiency; just getting something that worked done quick.

@Wohlstand
Copy link
Member

  1. Don't worry on that, I think, I'll make the support for other formats myself. Except of MP3 tags are complex and messy (for them I developed the most basic system only which is needed for important well-known tags).
  2. You don't need that, just make a simple fixed-size array (for example, with 200 entries), and store inside the char *key and char*value fields. If structure is null, it's an end of array. When searching an entry, just fetch from begin up to first null key and compare it. When it matches (case-insensitive), return a value, otherwise, return null when nothing was found.
  3. In 2 I had to explain that it's fine. The main point of me and @sezero is that it's possible to make the code itself being more compact.

@SNSTRUTHERS
Copy link
Author

Well at least I gave you guys an idea or starting off point lol

Copy link
Collaborator

@sezero sezero left a comment

Choose a reason for hiding this comment

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

I already mentioned my views on this PR, so I don't have anything further. (As per reviewing the suggested code line by line: sorry, not interested in the topic much myself.)

Also removed my name from the source/header files
@Wohlstand
Copy link
Member

I do think, the idea is good, but it should be implemented differently:

  • make the cache of all meta-tags on the loading stage
  • access every meta-tag from the cache directly, don't request tags from the library every time, that is a waste of computing time

I'll close this as it became stale, but I will make the task issue to implement the ability once upon.

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

Successfully merging this pull request may close these issues.

None yet

3 participants