ralfbiedert / openh264-rs

Idiomatic Rust wrappers around OpenH264.
66 stars 32 forks source link

Remove profile options override in openh264-sys2 #11

Closed jelmansouri closed 2 years ago

jelmansouri commented 2 years ago

I find it les confusing to override profile configuration using cargo, for example:

[profile.dev.package.openh264-sys2]
opt-level = 3

Currently this setting doesn't work.

jelmansouri commented 2 years ago

If it's acceptable can a new patch version be released?

ralfbiedert commented 2 years ago

Have to look into this tomorrow, but some questions:

ralfbiedert commented 2 years ago

Also, I wouldn't call it an override, its more In was trying to make it actually follow the profle settings but your right the code right now is too simplistic. Wouldn't a better option be trying to read the effective opt and debug setting of the used profile and use them?

About your other question, yes once resolved can do new release right away.

jelmansouri commented 2 years ago

Thanks a lot for looking into it tomorrow:

doesn't this disable C (openh264) optimizations even on release builds?

To get the opt-level before compiling the cc crate calls this:

fn get_opt_level(&self) -> Result<String, Error> {
        match self.opt_level.as_ref().cloned() {
            Some(ol) => Ok(ol),
            None => Ok(self.getenv_unwrap("OPT_LEVEL")?),
        }
    }

and OPT_LEVEL is set accordingly to the target profile being compiled. For example when calling cargo build -vv --release:

[openh264-sys2 0.2.9] running: "cl.exe" "-nologo" "-MD" "-O2" "-Brepro" "-I" "upstream/codec/api/svc/" "-I" "upstream/codec/common/inc/" "-I" "upstream/codec/encoder/core/inc/" "-I" "upstream/codec/encoder/plus/inc/" "-I" "upstream/codec/processing/interface/" "-Fo...\\openh264-rust\\target\\release\\build\\openh264-sys2-91fb7b911fedd4ea\\out\\upstream/codec/encoder\\core\\src\\svc_set_mb_syn_cabac.o" "-c" "upstream/codec/encoder\\core\\src\\svc_set_mb_syn_cabac.cpp"

how would you then enable/disable debug info for the C parts?

debug uses the DEBUG environment variable when not set explicitly. Haven't done tests but I expect it to work in the same way!

Hope this helps!! Thanks,

jelmansouri commented 2 years ago

So I can confirm for debug so, dev by default adds -Z7 on the command line, as you can see in the previous comment -Z7 is not present in release and if you want debug symbols in release you can override it :

[profile.release.package.openh264-sys2]
debug = true

I only tested on windows, but from looking at the code the same behavior is available on linux.

jelmansouri commented 2 years ago

I think the major difference from the original code is that on linux, opt-level was forced to -O3 on gcc/clang in anything other than the dev profile, the default being -O2. Haven't benchmarked the delta yet.

Our monorepo has:

[profile.release]
opt-level = 3
debug = true
debug-assertions = false
panic = 'abort'

[profile.bench]
opt-level = 3
debug = true
panic = 'abort'
ralfbiedert commented 2 years ago

Just looked at this again, the original code was doubly wrong. Not only should it have read OPT_LEVEL and debug, but also cc is already doing this for us:

image

ralfbiedert commented 2 years ago

Published 0.2.11. Thanks for the PR!