Unable to successfully set the number of threads on Linux and Windows

[repnop]

It currently doesn't seem possible to set the number of threads to anything other than the default value of 0, otherwise openh264 returns an error (on Windows) or segfaults (on Linux, and on Windows if Decoder::with_config is modified to only set the number of threads [Linux also segfaults]). Here's some backtraces from GDB that might help with debugging!

With everything as is, as of 0.2.8:

[0] from 0x00007ffff7d48b3d in sem_wait@@GLIBC_2.2.5
[1] from 0x0000555556a06716 in SemWait(SWelsDecSemphore*, int32_t)+77 at upstream/codec/decoder/core/src/wels_decoder_thread.cpp:243
[2] from 0x0000555556a2c17b in WelsDec::CWelsDecoder::CloseDecoderThreads()+109 at upstream/codec/decoder/plus/src/welsDecoderExt.cpp:328
[3] from 0x0000555556a2b97a in WelsDec::CWelsDecoder::~CWelsDecoder()+108 at upstream/codec/decoder/plus/src/welsDecoderExt.cpp:235
[4] from 0x0000555556a2ba45 in WelsDec::CWelsDecoder::~CWelsDecoder()+39 at upstream/codec/decoder/plus/src/welsDecoderExt.cpp:257
[5] from 0x0000555556a30820 in WelsDestroyDecoder(ISVCDecoder*)+58 at upstream/codec/decoder/plus/src/welsDecoderExt.cpp:1502
[6] from 0x00005555569b69fe in openh264::decoder::{impl#0}::drop+14 at /home/repnop/.cargo/registry/src/github.com-1ecc6299db9ec823/openh264-0.2.8/src/decoder.rs:82
[7] from 0x00005555569ba32b in core::ptr::drop_in_place<openh264::decoder::DecoderRawAPI>+11 at /rustc/f1edd0429582dd29cccacaf50fd134b05593bd9c/library/core/src/ptr/mod.rs:188
[8] from 0x00005555569b6d8d in openh264::decoder::Decoder::with_config+669 at /home/repnop/.cargo/registry/src/github.com-1ecc6299db9ec823/openh264-0.2.8/src/decoder.rs:154

If Decoder::with_config is changed to only set the number of threads:

[0] from 0x00007ffff7d48b3d in sem_wait@@GLIBC_2.2.5
[1] from 0x0000555556a06c36 in SemWait(SWelsDecSemphore*, int32_t)+77 at upstream/codec/decoder/core/src/wels_decoder_thread.cpp:243
[2] from 0x0000555556a308d7 in WelsDec::CWelsDecoder::ThreadDecodeFrameInternal(unsigned char const*, int, unsigned char**, TagBufferInfo*)+161 at upstream/codec/decoder/plus/src/welsDecoderExt.cpp:1416
[3] from 0x0000555556a2dbf8 in WelsDec::CWelsDecoder::DecodeFrameNoDelay(unsigned char const*, int, unsigned char**, TagBufferInfo*)+94 at upstream/codec/decoder/plus/src/welsDecoderExt.cpp:698
[4] from 0x00005555569b8c95 in openh264::decoder::DecoderRawAPI::decode_frame_no_delay+37 at /home/repnop/.cargo/registry/src/github.com-1ecc6299db9ec823/openh264-0.2.8/src/decoder.rs:69
[5] from 0x00005555569b72df in openh264::decoder::Decoder::decode+255 at /home/repnop/.cargo/registry/src/github.com-1ecc6299db9ec823/openh264-0.2.8/src/decoder.rs:172

In both cases, it looks like we get a nullptr deref in the semaphor waiting function on Linux, on Windows the segfault also occurs within the decoding functions, but we're not able to quite tell where.

So it may be worth removing the function until its possible to actually use it without segfaulting

[ralfbiedert]

I'm inclined to close this as it's not really a bug on our side, but feel free to change my mind:

Ultimately we want to allow users to do their thing, and ideally want to expose most OpenH264 APIs. Setting threading is an OpenH264 option. They mark it as dangerous themselves, which it why we expose it as unsafe, but in any case it's a option offered.

(On a side note, it's mostly coincidental this is one of the few APIs exposed so far, When playing with API design I just wanted to try a few setting and this just turned out to be one of them. In the long term imagine there are a few dozen options / methods users might call)

Now the code may be broken upstream on some platforms, but if we wanted to curate API availability based on these considerations we'd have to test each API on all platforms and then ... only include it if it works everywhere? Somewhere? The last part is where things get fuzzy for me. What if on RISC-V this actually worked (it doesn't, just for sake of argument)?

[repnop]

I definitely agree with the choice to offer it -- especially as an unsafe method, however if other people experience the same issue, regardless of platform, that we have with trying to set any amount of additional threads, I'm not entirely sure if its a good idea to have it available at all since it'll end up being unusable anyway. I can try to discover whether or not this affects macOS (ideally both architectures), and ARM-based Windows/Linux, but I would think that those would also be pretty niche. I guess to tl;dr my point: it seems broken on every platform in various ways, and they do call it "experimental" in their 2.1.0 release notes:

Experimentally support for multi-thread decoding(default disabled,and may result in random problems if enabled)

Perhaps a middle ground could be additional documentation on the DecoderConfig::num_threads method that multithreaded decoding is considered an experimental feature and is likely to segfault.