TeamPyOgg / PyOgg

Simple OGG Vorbis, Opus and FLAC bindings for Python
The Unlicense
63 stars 27 forks source link

Multiple definitions of library functions #55

Closed mattgwwalker closed 3 years ago

mattgwwalker commented 3 years ago

@Zuzu-Typ mypy popped up with an error around the definition of oggpack_writeinit() in ogg.py. It seems as though we might have a copy-paste error perhaps?

Would you mind having a look at it? Specifically, the issues are around lines 471-490.

To me it seems as though the function has been defined four times, and that possibly it just requires that the python function names are corrected and their calls to the ogg library corrected too so that they match the argument and return type definitions above them. Do you agree? Or is there some magic going on here that I don't understand?

Cheers,

Matthew

mattgwwalker commented 3 years ago

If I understand correctly, I think the code should look like this:

    libogg.ogg_sync_init.restype = c_int
    libogg.ogg_sync_init.argtypes = [oy_p]

    def ogg_sync_init(oy):
        return libogg.ogg_sync_init(oy)

    libogg.ogg_sync_clear.restype = c_int
    libogg.ogg_sync_clear.argtypes = [oy_p]

    def ogg_sync_clear(oy):
        return libogg.ogg_sync_clear(oy)

    libogg.ogg_sync_reset.restype = c_int
    libogg.ogg_sync_reset.argtypes = [oy_p]

    def ogg_sync_reset(oy):
        return libogg.ogg_sync_reset(oy)

    libogg.ogg_sync_destroy.restype = c_int
    libogg.ogg_sync_destroy.argtypes = [oy_p]

    def ogg_sync_destroy(oy):
        return libogg.ogg_sync_destroy(oy)

    try:
        libogg.ogg_sync_check.restype = c_int
        libogg.ogg_sync_check.argtypes = [oy_p]
        def ogg_sync_check(oy):
            return libogg.ogg_sync_check(oy)
    except:
        pass

If you agree, then I'll include this with my changes.

mattgwwalker commented 3 years ago

Ah, it appears we may have other similar problems elsewhere: it seems vorbis_info_init() was also redefined.

Did you write this code by hand?!? I had assumed it was automatically generated. If you did it by hand I've no idea how you kept your sanity!

Zuzu-Typ commented 3 years ago

Yeah, those are definitely copy and paste errors. It was a tedious process, but yes, I did them all by hand. That's why there are so many errors 😅 Sorry about that.

ogg and vorbis were the first ones I made, so they are probably also the worst. Hopefully I didn't make many mistakes in opus and flac's files.

mattgwwalker commented 3 years ago

That you did them by hand is amazing. I certainly wouldn't have had the patience! Thanks for all that work :o)

So because I'm going through the issues raised by mypy, did you go through the include files for each library? If I have questions, is that the best place to double-check?

Zuzu-Typ commented 3 years ago

Yes, I used the include files from each library as a basis. In case you don't want to look them up yourself: For ogg.py: https://github.com/xiph/ogg/tree/master/include/ogg For vorbis.py: https://github.com/xiph/vorbis/tree/master/include/vorbis For opus.py: https://github.com/xiph/opus/tree/master/include , https://github.com/xiph/opusfile/tree/master/include and https://github.com/xiph/libopusenc/tree/master/include And for flac.py: https://github.com/xiph/flac/tree/master/include/FLAC

For opus and flac I tried to label each section with the respective include-file I used. For example, this section was ectracted from metadata.h.

In case your wondering about the random try and except definitions, I had to add them because for some reason, they aren't always present in all libraries (especially on Linux if I recall correctly). Though I remember commenting out quite a bunch of them as sections, without trying each one individually, which isn't necessarily a good thing.

It may also be worth considering using @-decorators to generate the wrapper functions (or remove the wrappers completely, as they are rarely necessary).

mattgwwalker commented 3 years ago

These issues should now be resolved (see pull request #58).

Mypy no longer detects any repeated definitions.

Closing.