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

Compilation with MSVC #11

Closed
wants to merge 6 commits into from
Closed

Conversation

0815employee
Copy link

We have recently ported the complete source code of codec2 for MSVC, for our needs.

This includes many changes - in almost all files:

  • replaced all VLA's with a macro, which replaces them with a dynamic allocation if VLA's are not supported
  • replaced all uses of native C-type "complex float"

Other changes

  • fixed an issue with the version.h header install directory
  • added windows ddlimport/dllexport
  • some changes in cmake
  • added missing modes in the "raw" receive api functions

Fixes

  • fixed a memory leak, wrong free order - maybe not an memory leak when using the stack/VLAs only
  • there was an segfault within the ldpc decoder when using the 2020 modes. The output buffer was too small (uint8_t out_char [?] in freedv_2020.c). Since the variable names regarding the codeword lengths were not clear to me, I allocated a greater than necessary length for this. Should be looked at again.

It would be great if these changes (or parts of them) could be merged.

@tmiw
Copy link
Collaborator

tmiw commented Sep 22, 2023

Thanks for this PR. I do have concerns as to how it'll affect our embedded projects (the SM1000 and ezDV). I'm also not sure about how much effort would be needed to keep VS support working in the future. I'm going to forward this along to the rest of the project team for further discussion and will update when we have more.

@drowe67
Copy link
Owner

drowe67 commented Sep 25, 2023

Hi @0815employee - that's quite an impressive project. As per our README we have settled on gcc, and perhaps more generally standards based compilers that support C99. We are content to compile for Windows using gcc based tools, and like features like VLAs and complex float.

We are an open source project so are not motivated to make extensive modifications to support proprietary, non standard compilers such as MSVC.

However it's open source, so please feel free to work on your port as a fork, and even publish it as a separate project if you like. We'd be interested to hear more about the "fixes" if you can be more explicit and/or reproduce the problem using codec2/main on a Linux machine.

Can you please tell us a little about your project? For example what are you using libcodec2 for, and why is MSVC support a requirement?

@0815employee
Copy link
Author

I work for a company and we integrate codec2 in one of our products which we offer for Linux and Windows. So we had to make the decision whether to use mingw or integrate codec2 into our Windows toolchain.

I understand the reasons why you don't want to support MSVC. But I do not like VLAs or complex float either.

  • VLAs allocate memory on the stack, which sometimes hides problems with segfaults. For example, the bug in the LDPC decoder causes a crash when working with memory on the heap. Valgrind helped to find the exact location, which would not be possible with VLA. And it is an optional C-feature. There are several sources on the web why VLAs are problematic.
  • In my experience with std::complex, the built-in complex type is often a bit slower than self-defined complex data types (probably not a problem here). The COMP type already existed in the project. Why two types for the same purpose?

Replacing all VLA with the macro is of course also not optimal and creates ugly code sometimes. At least it is possible to switch now easily between VLA and calloc/free.
IMHO: A clean solution would be not to use VLAs and to allocate memory on the heap once that the module needs in sufficient size.

fixes

  • 68041b9
    freedv_close(freedv) is called before reliable_text_destroy(...). But reliable_text_obj has a pointer to freedv to unlink both objects when calling reliable_text_destroy. reliable_text_destroy accesses members of freedv.

  • LDPC segfault: This fix is buried in b11698f. Sorry for that.
    This is where memory is allocated for the decoded infoword:

    uint8_t out_char[coded_bits_per_frame];

    But more is needed in ldpc_decode_frame

    codec2/src/interldpc.c

    Lines 151 to 152 in e14d690

    void ldpc_decode_frame(struct LDPC *ldpc, int *parityCheckCount, int *iter,
    uint8_t out_char[], float llr[]) {
    for the 2020(B) mode. For reproduction you only have to log access to out_char, maybe print the access index or use a debugger.

@drowe67
Copy link
Owner

drowe67 commented Sep 26, 2023

  • In my experience with std::complex, the built-in complex type is often a bit slower than self-defined complex data types (probably not a problem here). The COMP type already existed in the project. Why two types for the same purpose?

Historical reasons - the OFDM modem was written last and the primary author chose to use complex float, I didn't actually know complex float was available when I wrote the earlier modems that use the macros.

Thanks for the tips on the bugs - we'll check them out. 2020B is likely to be removed soon as it doesn't offer any real advantage.

It's likely the earlier FDMDV/COHPSK modems will be removed soon, along with many FreeDV modes. We're working on a more generic, single FreeDV mode that can handle many channels.

Did you port all the ctests to MSVC and do they all pass? That's perhaps a far more important issue than coding and compiler preferences.

@0815employee
Copy link
Author

Did you port all the ctests to MSVC and do they all pass? That's perhaps a far more important issue than coding and compiler preferences.

Not complete. The source code itself is ported, but the tests and the executables do not build, as there are still some GCC-specific things here.

As soon as I have time I will have another look.

@drowe67 drowe67 closed this Oct 2, 2023
@0815employee
Copy link
Author

0815employee commented Oct 26, 2023

I have now ported many of the unit tests, see fork. Some unfortunately do not work for various reasons.

  • ldpc_dec and lpcd_enc do not compile - not C99 compliant
  • problems with Octave and these mex files - more work needed
  • they need to be looked at individually - more work needed

These tests are disabled.

@GASitton
Copy link

My name is Gary Sitton and I'm very interested in a port from C99 gcc to ANSI C in MSVS C. I have done ports like this before into Thetis which I am working on now. I have a complete Macro library for the complex data type which works very well in place of a native complex type in C99. I am very interested in porting Codec2 into Thetis and have extensive experience in LPC and other DSP technologies. I would like to get in contact with you. My personal e-mail is [email protected]. Thank you.

@drowe67
Copy link
Owner

drowe67 commented Feb 16, 2024

Hi @GASitton - I emailed you yesterday, hope it made it through!

As @tmiw has suggested in the email thread, our recommended path for use with MSVC is to use DLLs cross compiled from the C99 codebase. This will save you a lot of work, and you well get well tested and maintained code. A source level port is a very big project, and would require you to port many ctests and provide ongoing maintenance as Codec 2 evolves.

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.

4 participants