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

added dsf support, reusing the existing ID3 parser #43

Merged
merged 26 commits into from
Nov 4, 2018

Conversation

ilkomiliev
Copy link
Contributor

please note that this is my very first golang work, so any feedback is appreciated. Still have no clue how to prepare tests, but already questions about this:

  1. are you including the necessary music files also in the repo (in case of dsf the files could be huge - the one I've tested with is 300 mb).

  2. copyrights: I think some dsd files are provided for test purposes on the web, but haven't check.

@dhowden
Copy link
Owner

dhowden commented Sep 21, 2018

Hey, thanks for the contribution!

I'll try to have a look at the code in the next few days.

To answer your questions: The tag data is in the first few KB of the file, so you shouldn't need to include much for a full test. I don't have any DSF files myself, so if you could post a source of test files here that would be great.

@jooola
Copy link
Contributor

jooola commented Sep 21, 2018

@ilkomiliev If you have a sample file, you can add some testing.

Just check how the tag_test.go file works.

I couldn't find any DSF file, that's weird. So you could create on using the testdata/with_tags/sample.flac file.

Copy link
Owner

@dhowden dhowden left a comment

Choose a reason for hiding this comment

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

Hey!

Just had a quick look through and made a few comments.

I also looked at the spec and see that the ID3 tags are supposed to be at the end of the DSF files - so I now see your issue with adding some testdata! Don't worry about adding a file, just attach a few test URLs in the comments here so we can try a few and see the code in action.

Thanks again!

dsf.go Outdated Show resolved Hide resolved
dsf.go Outdated Show resolved Hide resolved
dsf.go Outdated Show resolved Hide resolved
@ilkomiliev
Copy link
Contributor Author

should be ok now. Following site provides a lot of DSF files:
2L
I've tested:
this one

@jooola
Copy link
Contributor

jooola commented Sep 21, 2018

I tried to create a file that we could keep in the repo:
sample.zip

Could you please try it ?

@ilkomiliev
Copy link
Contributor Author

I tried to create a file that we could keep in the repo:
sample.zip

Could you please try it ?

Metadata Format: ID3v2.3
File Type: DSF
Title: Test Title
Album: Test Album
Artist: Test Artist
Composer: Test Composer
Genre: Jazz
Year: 2000
Track: 3 of 0
Disc: 2 of 0
Picture:
Lyrics:

"TYER": "2000"
"TALB": "Test Album"
"TPE1": "Test Artist"
"TPE2": "Test AlbumArtist"
"TPOS": "2"
"TRCK": "3"
"COMM": &tag.Comm{Language:"eng", Description:"", Text:"Test Comment\x00"}
"TCOM": "Test Composer"
"TCON": "Jazz"
"TIT2": "Test Title"

let me check then how to integrate it into the test suite and I'll update the PR with the tests

@ilkomiliev
Copy link
Contributor Author

ok, now looks ok - I have to edit the dsf file to add the ttl tracks. Now the test is passing. Please review it again, thanks.

@ilkomiliev
Copy link
Contributor Author

Hi,

can this be merged or something is still missing?

@dhowden
Copy link
Owner

dhowden commented Oct 2, 2018

Hi there! Apologies, I've been preoccupied with other stuff. As I don't have any DSF files myself - so I could quickly check the implementation - I will just have to read the spec instead (and hence I've been putting it off somewhat!).

If anyone else has DSF files that they can run though that would be great, otherwise I hope to look at this at the weekend.

@ilkomiliev ilkomiliev closed this Nov 4, 2018
@ilkomiliev ilkomiliev deleted the feature/dsf-metadata branch November 4, 2018 16:56
@jooola
Copy link
Contributor

jooola commented Nov 4, 2018

@ilkomiliev Did you just give up ? Sad...

@ilkomiliev ilkomiliev restored the feature/dsf-metadata branch November 4, 2018 17:29
@ilkomiliev
Copy link
Contributor Author

no, I've messed up the branches somehow, trying to clean up :-(
let me check again

@ilkomiliev ilkomiliev reopened this Nov 4, 2018
@ilkomiliev
Copy link
Contributor Author

ok, back again - good that github is so sophisticated!

Copy link
Owner

@dhowden dhowden left a comment

Choose a reason for hiding this comment

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

Sorry, I went silent on this.

Not sure what the motivation is behind the compilation scripts and ignore file.

In general I think that the best way to handle things like that is to make official releases, so I won't merge them in.

If you can remove those two new files, I will merge the rest of the PR now.

cmd/tag/compile.sh Outdated Show resolved Hide resolved
.gitignore Outdated
@@ -0,0 +1,12 @@
# Binaries for programs and plugins
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this file.

cmd/tag/compile.sh Outdated Show resolved Hide resolved
@ilkomiliev
Copy link
Contributor Author

sorry for that - it seems I mess it totally, I hope I've fixed everything now. Quite late already here - if something goes wrong again, I'll check it tomorrow.

@dhowden dhowden merged commit a9f04c2 into dhowden:master Nov 4, 2018
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.

3 participants