dhowden / tag

ID3, MP4 and OGG/FLAC metadata parsing in Go
BSD 2-Clause "Simplified" License
568 stars 77 forks source link

added dsf support, reusing the existing ID3 parser #43

Closed ilkomiliev closed 6 years ago

ilkomiliev commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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.

ilkomiliev commented 6 years ago

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

jooola commented 6 years ago

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

Could you please try it ?

ilkomiliev commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Hi,

can this be merged or something is still missing?

dhowden commented 6 years ago

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.

jooola commented 6 years ago

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

ilkomiliev commented 6 years ago

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

ilkomiliev commented 6 years ago

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

ilkomiliev commented 6 years ago

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.