search

dhowden / tag

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

Add Ogg Opus metadata support #69

Closed wader closed 2 years ago

wader commented 4 years ago

Refactor into a ogg demuxer that returns slice of packets and then look for first vorbis comment or opus tags packet.

Fixes #64

wader commented 4 years ago

@dhowden Sorry for messy diff, it got messy to add support so decided to refactor a bit. Also haven't tried it with many ogg files but it seems to work, please how a go if you have time.

deluan commented 4 years ago

Hey guys, any blockers for this to be merged?

wader commented 4 years ago

I guess testing is what is missing. Do you have lots of oggs with metadata to test on?

deluan commented 4 years ago

Not that many, maybe a dozen or two, but will try with what I have this weekend.

dhowden commented 4 years ago

@dhowden Sorry for messy diff, it got messy to add support so decided to refactor a bit. Also haven't tried it with many ogg files but it seems to work, please how a go if you have time.

No worries. I remember scanning through this and planning to come back to it: wasn't sure why you didn't use the CRC32 stuff from the stdlib.

wader commented 4 years ago

@dhowden Sorry maybe should add a comment about that. I tried to use stdlib crc32 but didn't mange to get it working. Did some digging in the ogg reference implementation and it seems both table generator and update function is a bit different from the go crc32 https://github.com/xiph/ogg/blob/6e9f7cc2f64c9e659c6ca7cde6737f5d1d564b5e/src/framing.c#L112 or do you have any idea if it possible?

deluan commented 4 years ago

Hey, a user of my project was able to test it compiled against this PR and it successfully extracted cover arts from 4K+ files without errors... 👍

wader commented 4 years ago

@deluan ah nice, thanks for testing! any idea what mix or ogg vorbis/opus it was?

deluan commented 4 years ago

It was tested by a user, @andre15silva, maybe he can give some more info about his library.

Also, seems that there are still some edge cases where the library returns an invalid image, I'm trying to get a sample file and attach it here.

wader commented 4 years ago

Ah that would be great

andre15silva commented 4 years ago

@wader, I encoded all those files using opusenc, so they are all ogg opus. What other information is useful to you?

wader commented 4 years ago

@andre15silva ok! i think ogg files produced by different encoders would be useful

mipimipi commented 3 years ago

@all: Just tested that as well - no problems. Can this be merged? Thanks.

wader commented 3 years ago

@mipimipi 👍 @dhowden let me know if there is something i should change or clarify in the PR

dhowden commented 2 years ago

@dhowden Sorry maybe should add a comment about that. I tried to use stdlib crc32 but didn't mange to get it working. Did some digging in the ogg reference implementation and it seems both table generator and update function is a bit different from the go crc32 https://github.com/xiph/ogg/blob/6e9f7cc2f64c9e659c6ca7cde6737f5d1d564b5e/src/framing.c#L112 or do you have any idea if it possible?

Yeah, without examples it's hard to look at. I suspect they aren't using the same form of CRC32 that the stdlib does, which is annoying. I wonder if it's adaptable? Not sure it's worth it though. https://github.com/Michaelangel007/crc32

wader commented 2 years ago

Interesting CRC article, probably need some time to really understand that :) do you feel awkward merging as it is now? would it be enough with a comment about where the CRC stuff comes from and that it is confusing?

Alternatively just skip checking CRC?

idcmp commented 2 years ago

Any thoughts on adding Opus support?

bertvandepoel commented 2 years ago

Is there anything I can do to help advance this PR?

wader commented 2 years ago

I'm happy to help fixing whatever needs to be done

bertvandepoel commented 2 years ago

@wader I see, so you're just waiting for @dhowden to give feedback concerning the CRC issue?

wader commented 2 years ago

@bertvandepoel Yes, i guess accept current change, skip CRC check or somehow rewrite or clarify the CRC thing, but i'm not a CRC expert. If i remember correct i could not figure how to use https://pkg.go.dev/hash/crc32#MakeTable to make it do the same thing, maybe something it could be done by transforming the 0x04c11db7 polynom? so i ended up portort some C version etc of it instead

dhowden commented 2 years ago

Yeah, sorry. I started looking at removing the CRC stuff (a long time ago now), and then didn't finish it. Ideally we would ditch the custom CRC implementation, but without a good solution happy to keep it in for now.

wader commented 2 years ago

@dhowden Yes that would be nice, but don't think i will spend more time on it either. Thanks for reviewing and creating this package!