threadsafe basop32 and enh40 files, with control over global variables and exit/abort

[signalogic]

basop32_threadsafe.c, enh40_threadsafe.c, basop32_threadsafe.h, and enh40_threadsafe.h are modified basop and enh40 files for use with ITU and ETSI codecs. The modifications make some functions static inline, allow disabling terminations not suitable for Linux libs such as exit() and replace with alternate error handling and message logging, and allow removal or alternative usage of global variables Overflow and Carry. For example, sub_ovf() is identical to sub() but includes a stack based Overflow param. Definitions such as USE_BASOPS_INLINE, EXCLUDE_BASOPS_NOT_USED, USE_BASOPS_OVERFLOW_GLOBAL_VAR, USE_BASOPS_CARRY_GLOBAL_VAR, and USE_BASOPS_EXIT allow fine-grain control of modification behavior. Also:

-2017 STL format & indentation is maintained -modifications include detailed comments -for testing with standard codec builds, modify your Makefile to temporarily rename basop32_threadsafe.h to basop32.h and enh40_threadsafe.h to enh40.h -so far bit-exact tested with some codecs, but no separate functional test is provided for basop

[ErikNorvell-Ericsson]

Many thanks for providing this update!

We have also come across the issue with the global variables in STL and have patched it locally in our code, similar to the solution you propose. We still have some comments that we think may be considered:

• When using the operators without _ovf, it might be better to halt the execution via e.g. an assert. The global flags should preferably be removed to avoid any usage of these. This means that the updated STL cannot be used directly with old implementations, but in our view it would still be a better solution for the continued use of STL.
• The current code has a few preprocessor macros related to EVS, and also to enable the new functionality. It would be preferable to have the new functionality added without preprocessor macros – if you need an older variant you will just have to download that one instead.
• In the file basop32_threadsafe.c there is a note that it was tested with EVS and AMR-WB. How was this testing done? I guess the thread safe functions were not part of that test, since it would require an update of the EVS and AMR-WB basop code.

Thanks and best regards, Erik

[signalogic]

Hi Erik, thanks very much for your feedback ! Some comments and notes:

1) Asserting upon overflow inside non _ovf operators sounds good. If we make that behavior a build-time option then it should still be possible for old implementations to use update STL source. Does that sound ok ?

2) Re. EVS #defines, can you clarify "without preprocessor macros" - do you mean without any preprocessor macros or just the EVS ones ?

3) Testing was done with Signalogic implementations of EVS and AMR-WB codecs, enabling testing of concurrent / multithreaded operation.

-Jeff

[ErikNorvell-Ericsson]

Hi Jeff,

Thanks for your reply. First, I would like to point to #173 provided by our colleagues at Fraunhofer. I guess this a clearer picture of what we had in mind.

1. I suppose a build-time option could be ok, but then I think the default should be to use the new behavior. So you could e.g. activate the old solution using global variables using a preprocessor macro. Maybe the user can be made aware the tool is used in a non-recommended way.
2. I was thinking about the EVS macro in basop_threadsafe.c. Perhaps this was just there to mark the source of these changes and was meant to go away?
3. Thanks for the explanation. If I understood you right, the EVS and AMR-WB implementations would need to be updated to use the new operators. If we would take the reference basop code for these codecs and combine with the updated STL package, they would just use the old operators. To give some more explanation: in our work with IVAS we introduced these changes as outlined in #173. We needed to patch the EVS code to use the introduced operators and I was simply wondering if you needed to do something similar.

[simaocampos]

A quick comment - I think 1) moving away from global variables is a good idea moving forward. It fits the general philosophy of the library for modularity (that gave me a lot of work, especially when I was incorporating modules such as G.726 and G.722 into the STL a few decades ago... not a new problem!) 2) I support the proposed approach to have a global compile time switch for backward compatibility, there is a lot of code that uses end expect that behavior. However, we need to make sure that as future versions of the module are developed, that the backward option will continue to work. That means, it needs to be carefully included in regression / portability testing.

Cheers, Simão

From: Erik Norvell @.> Sent: Friday, 22 September, 2023 09:21 To: openitu/STL @.> Cc: Subscribed @.***> Subject: Re: [openitu/STL] threadsafe basop32 and enh40 files, with control over global variables and exit/abort (PR #171)

Hi Jeff,

Thanks for your reply. First, I would like to point to #173https://github.com/openitu/STL/pull/173 provided by our colleagues at Fraunhofer. I guess this a clearer picture of what we had in mind.

1. I suppose a build-time option could be ok, but then I think the default should be to use the new behavior. So you could e.g. activate the old solution using global variables using a preprocessor macro. Maybe the user can be made aware the tool is used in a non-recommended way.
2. I was thinking about the EVS macro in basop_threadsafe.c. Perhaps this was just there to mark the source of these changes and was meant to go away?
3. Thanks for the explanation. If I understood you right, the EVS and AMR-WB implementations would need to be updated to use the new operators. If we would take the reference basop code for these codecs and combine with the updated STL package, they would just use the old operators. To give some more explanation: in our work with IVAS we introduced these changes as outlined in #173https://github.com/openitu/STL/pull/173. We needed to patch the EVS code to use the introduced operators and I was simply wondering if you needed to do something similar.

- Reply to this email directly, view it on GitHubhttps://github.com/openitu/STL/pull/171#issuecomment-1730928228, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AEC4YAVIJQQWTTXXVVIQN23X3U34LANCNFSM6AAAAAAY2DYATA. You are receiving this because you are subscribed to this thread.Message ID: @.**@.>>

[sdoehla-iis]

A quick comment - I think 1) moving away from global variables is a good idea moving forward. It fits the general philosophy of the library for modularity (that gave me a lot of work, especially when I was incorporating modules such as G.726 and G.722 into the STL a few decades ago... not a new problem!)

Newer codecs like EVS do not have any global variables other than the ones in the BASOPs and potentially a frame counter (which is convenient in development code but probably even there not justified).

We fully support Jeff's objective that we need to avoid global variables to have threadsafe and reentrant code. As mentioned by Erik, in https://github.com/openitu/STL/pull/173 we have an alternative way how it could be done. We do not insist on our PR to make it, but we do have the objective to have the full set of BASOPs available without global variables so that 3GPP can develop fixed-point code that does not inherit this issue. As a side note, we've developed this with 3GPP IVAS compatibility in mind, while Jeff did so with Signalogic's implementations.

2) I support the proposed approach to have a global compile time switch for backward compatibility, there is a lot of code that uses end expect that behavior. However, we need to make sure that as future versions of the module are developed, that the backward option will continue to work. That means, it needs to be carefully included in regression / portability testing. Cheers, Simão

Yes, this is the main pain point of such changes. There is potentially quite some code around that uses the old operators. In our experience, the saturating behavior of the operators or usage of the Overflow / Carry flags is not extensively used. It is thus a realistic undertaking to modify existing code. Modified code would then benefit from explicit marking where Overflows happen, which would even permit faster implementations of the operators in the cases where there is no Overflow. This is of course outside the scope of STL but may be relevant for implementers who replace the BASOPs by their platform-specific intrinsics.

The G.722 has been patched in https://github.com/openitu/STL/pull/173 - but regression/portability testing was limited to the tests that were part of STL.

On backward compatibility in general, I would opt more for making a cut and making the variant without global variables the default behaviour. Existing code may rely on STL2009, old ETSI BASOPs, STL2023 or even variants that have been modified inside a codec (EVS is an example that used STL2009 plus some operators that only made it into STL2015). The asserts in our approach help to identify where to modify code and if e.g. a codec wants to move to STL2025 (or whatever the next version is) then it would have to be mildly changed - as we did in G.722 or the EVS code portions inside IVAS.

Leaving a variant with global variables in to activate by a preprocessor switch - just to allow using latest STL with old code - may be an option, but I don't think it would be needed.

One other aspect on global variables: the BASOP counters are still global variables. We did not want to touch this in https://github.com/openitu/STL/pull/173 as they would only be used for complexity simulation and disabled in production code - and because carrying them around would require API changes to almost all operators.

[signalogic]

Hi Erik, let me reply to your comments first, then Stefan's.

1. My view of default behavior for global variable usage would be to maintain compatibility with legacy codec source code Makefiles (i.e. without a #define set, global variables are active). For updated codec source codes, Makefiles would set the correct #define(s) and global variables would be not be used. In this way users don't have to "think about it", it just works. Older versions that must be maintained due to customer requirements would be wholly unaffected if they choose to incorporate new STL. For IVAS, for which there is no issue with legacy versions already in production, as Stefan mentions the default behavior would no global variables. Re. your suggestion "Maybe the user can be made aware the tool is used in a non-recommended way", I think this is excellent -- any warnings or notices that can be given would be helpful. As Simao points out, messing with global variables can be a lot of work and saving users that work is desirable.

2. Re. the _EVS_ macro in basop32_threadsafe.c, we have it in there to allow compatibility with basop32.c in EVS source code. Evidently EVS authors added a few things, mostly BASOP_CHECKs, along with a reference to MSCVER (defined in a Visual Studio build environment). I don't know if there is an equivalent STLxxxx version or not, the only place I've seen it is in EVS source. Our thinking was that EVS source code users could set _EVS\ to obtain full legacy behavior. Possibly we could use the __hasinclude(header-name) preprocessor macro to define _EVS\ so it would be automatic, and let EVS users know that's been done at build-time, along with an option to disable that.

3. Re. your comment "If I understood you right, the EVS and AMR-WB implementations would need to be updated to use the new operators" -- yes, any updated versions would set #defines as appropriate in their Makefiles, per my comments in 1. Current and older sources (which I expect there are many in production) would not need to do anything. Re. your comment "If we would take the reference basop code for these codecs and combine with the updated STL package, they would just use the old operators" if you by "reference basop code" you mean codes that make use of basop operators, yes. Re. your comment "We needed to patch the EVS code to use the introduced operators and I was simply wondering if you needed to do something similar" yes we had to do the same.

[signalogic]

Hi Stefan-

I agree with most of your comments. In particular, your philosophy on explicitly specifying "no global variables" (i.e. the BASOP_NOGLOB #define) I think is correct, and the polarity of the preprocessor macros in our pull-request should be reversed (for example USE_BASOPS_OVERFLOW_GLOBAL_VAR should be changed to NO_BASOPS_OVERFLOW_GLOBAL_VAR). However, we have a key difference on the value of using an updated STL with existing codec sources (re. your comment "Leaving a variant with global variables in to activate by a preprocessor switch - just to allow using latest STL with old code - may be an option, but I don't think it would be needed").

The main point of Signalogic's pull-request is this. We have a number of large corporate customers who are deeply concerned about using a modified STL because it would constitute an ITU license violation (per the prohibition against source code modifications in the "ITU STL General Public License at https://github.com/openitu/STL/blob/dev/LICENSE.md). In fact, in a few situations this has dominated discussion as they are concerned not only about thread-safe operation but also unforeseen and untrackable aborts and exits, of which even the remotest possibility must be avoided. To address these concerns -- and avoid any touching whatsoever of current STL source -- Signalogic has come up with a method to intercept STL exit() and abort() calls, read the stack trace to know which codec source made the STL operator call, issue a warning/error message (and also log to a file if specified by user options), perform some type of "continuation behavior" (for example setting the result to maximum possible N-bit value in the case of division by zero), and return to the calling code. Of course we want to avoid this complexity; this is why our STL pull-request provides a "NO_BASOPS_EXIT" preprocessor macro. Possibly this could be expanded to include #defines for fine-grained control of error/warning messages, logging to file, and continuation behavior. Using this approach we can serve corporate customers who wish to use their current version of codec source with an updated STL that both provides thread-safe operation and eliminates the possibility of a total application exit.

[signalogic]

Signalogic has published a BASOP upgrade proposal. As a non-member we can't upload the doc to the ITU site so we have posted it on our STL Github fork:

https://github.com/signalogic/STL/blob/dev/doc/Signalogic_ITU_Non-Member_SG12_BASOPS_Upgrade.pdf

Also we posted a 3-slide overview PPT:

https://github.com/signalogic/STL/blob/dev/doc/Signalogic_ITU_BASOP_Upgrade_3Slides_Overview.pdf

[signalogic]

Continued upgraded basop development:

1) Updated proposal document, including more explanation of the (i) ITU GPL license issue and (ii) rationale for backward compatibility support (Signalogic_ITU_Non-Member_SG12_BASOPS_Upgrade_v2.pdf)

2) Reversed preprocessor macro polarity to meet backwards compatibility requirement. For example, USE_BASOPS_OVERFLOW_GLOBAL_VAR is now NO_BASOPS_OVERFLOW_GLOBAL_VAR. This matches BASOP_NOGLOB as described in PR173

3) Added thread-safe overflow functions add_ovf(), L_add_c_ovf(), L_sub_c_ovf(), and L_sat_ovf()

4) Added basop_platform.h to allow codec specific defines (would not be under the STL GPL License)