Add Ogg Opus metadata support #69
Conversation
Refactor into a ogg demuxer that returns slice of packets and then look for first vorbis comment or opus tags packet. Fixes dhowden#64
|
@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. |
|
Hey guys, any blockers for this to be merged? |
|
I guess testing is what is missing. Do you have lots of oggs with metadata to test on? |
|
Not that many, maybe a dozen or two, but will try with what I have this weekend. |
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. |
|
@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? |
|
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... 👍 |
|
@deluan ah nice, thanks for testing! any idea what mix or ogg vorbis/opus it was? |
|
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. |
|
Ah that would be great |
|
@wader, I encoded all those files using opusenc, so they are all ogg opus. What other information is useful to you? |
|
@andre15silva ok! i think ogg files produced by different encoders would be useful |
|
@ALL: Just tested that as well - no problems. Can this be merged? Thanks. |
we check isValidExt to determine if the file extension should be scanned, currently tag does not support opus files unless dhowden/tag#69 is merged
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 |
|
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? |
|
Any thoughts on adding Opus support? |
|
Is there anything I can do to help advance this PR? |
|
I'm happy to help fixing whatever needs to be done |
|
@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 |
|
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. |
|
@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! |
Refactor into a ogg demuxer that returns slice of packets
and then look for first vorbis comment or opus tags packet.
Fixes #64