GNOME Bugzilla – Bug 620136
Orc integration
Last modified: 2010-06-09 19:43:57 UTC
Created attachment 162356 [details] [review] Main patch Hey, I'd like to start integrating Orc usage soon. I'm pretty confident in Orc's abilities and stability recently, and I'm planning to dedicate a bunch of time in the next few months to get it nicely integrated everywhere, fix bugs, and maybe even write some new features. I've already pushed orc.mk to common, it is used to handle the HAVE_ORC/not HAVE_ORC case. It's automake-foo, so it's clumsy and somewhat gross, but it works. Attached are two patches and a documentation file (also in the first patch). The first patch converts a bunch of liboil-using plugins to Orc. The second patch is autogenerated code, which you can ignore. (The autogenerated code is checked into git so it can be used when Orc is not available at compile time.) Please read the attached doc, and if anything is unclear, please ask. (Oh yeah, if anyone has better names for the autogenerated files, please suggest.) Aside from the patch review, what additional criteria should be necessary before going ahead? Other comments?
Created attachment 162357 [details] [review] autogenerated code
Created attachment 162358 [details] Orc integration documentation
Will orc contain functions to get the CPU features, i.e. something like oil_cpu_get_flags()? I know of a few plugins that can't be converted to orc (without larger orc changes) but they currently use the CPU flags to enable usage of assembly. If not, what's your plan on this? Shall we add this feature somewhere in GStreamer? Apart from that I like the patch and would like to see it committed. Did you measure the performance impact of the orcification (both when using SIMD features and the fallback C code)?
David said on IRC that orc has somthing like oil_cpu_get_flags() in the unstable API but that it should be moved to the stable API. So everything should be fine ;)
Just FYI, next core/base/good freeze is planned for around June 11-13 (please don't commit this one day before that ;))
Also at git://code.entropywave.com/git/gstreamer/gst-plugins-base.git in the orc branch.
slomo: The replacement for oil_cpu_get_flags() is orc_target_get_default_flags(orc_target_get_by_name("sse")); and testing against ORC_TARGET_SSE_SSE2, etc. MMX is handled separately. Just added a quick hack to orcc to push out performance data as part of the test, so you can run tests/orc/volume and get this: orc_process_int16: cycles (backup): 2.83178 0.0460488 cycles (compiled): 0.938086 0.00306276 orc_process_int16_clamp: cycles (backup): 2.03946 0.00371835 cycles (compiled): 0.774511 0.00130922 orc_process_int8: cycles (backup): 2.04956 0.00011869 cycles (compiled): 0.712586 0.0015266 orc_process_int8_clamp: cycles (backup): 3.99119 0.00220664 cycles (compiled): 0.609633 0.00128364
I've decided on a new naming scheme. For volume, ORC_SOURCE=gstvolumeorc gstvolumeorc.orc Original .orc source file gstvolumeorc.h Generated header tmp-orc.c Generated source gstvolumeorc-dist.[ch] Disted source/header files
cog in -bad now implements all this stuff. I moved the tests from tests/orc into tests/check. I updated the orc branch. It's still all piled into one commit. Unless someone objects, I'd like to split this into one commit per plugin and push in the next few days. This stuff still requires Orc from git to reproduce cleanly. I'll be doing an Orc release in the next day or two, before pushing these changes.
Comment on attachment 162356 [details] [review] Main patch The patch needs to be updated. Some hunks don't apply. In Makefile.am it needs s/orc.mk/orc.mak. Finally it fails to build for me: make -C adder make[3]: Entering directory `/home/ensonic/projects/gstreamer/gst-plugins-base/gst/adder' make[3]: *** No rule to make target `.orc', needed by `tmp-orc.c'. Stop.
Created attachment 162899 [details] [review] updated patch, all merged together Updated patch. Also pushed to the above repository.
Updated branch again. Split up into multiple patches. Pretty close to what I'll push.
Sounds good :) But please make a new orc release before pushing your changes
Orc-0.4.5 is out, orc branch pushed. Please reopen if you notice any buggies.
So currently it just errors out if you don't have recent-enough orc installed. Is that intentional (instead of just falling back to disabling orc)? To make people aware that they're missing out on optimisations or so?
The build fails in audioresample (orc/* includes) with --disable-orc and no orc installed (did I ever mention my suggestion to keep a copy of the latest orc in core as gst-orc? ;-)).
commit 164a91d10d7b8b4f35e9efd5cb1c4db9620509cb Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Tue Jun 8 13:26:53 2010 +0100 Fix build if orc is not installed Orc is not a hard requirement. Things should still compile and work without orc, but slow fallback code may be used in this case. Fix up configure to not error out if orc is not installed and wrap use of orc profiling in audioresample in #ifdefs. Fixes #620136 some more.
The plugins seem to work fine. Also as I learned from IRC I checked that orc is actualy used: ORC: INFO: orcdebug.c(66): _orc_debug_init(): orc-0.4.5 debug init ORC: INFO: orccompiler.c(147): orc_program_compile_full(): initializing compiler for program "add_float32" ORC: INFO: orccompiler.c(211): orc_program_compile_full(): allocating code memory ORC: INFO: orccompiler.c(214): orc_program_compile_full(): compiling for target ORC: INFO: orccompiler.c(226): orc_program_compile_full(): finished compiling (success) Seems that for adder orc got used. I was expecting it to be used for volume/audioconvert too - will check. Anyway no more crashers, audio seems to be okay and performance is the same as without it :/ (both on Intel Core Duo and Amd64)
Tim, regarding your question in 15, I think erroring out if --disable-orc is not specified is the correct behaviour. If we just silently fall back to unoptimized code then a lot of people will not notice that they are not using Orc, and thus we will get a shit storm of comments on twitter and blogs about how slow GStreamer is. So forcing people to actively turning of optimisation support by using --disable-orc is a good thing IMO as at least they should have an idea why GStreamer is 'slow' then.
I'm not opposed to changing it back if that's the consensus. The error message should say a bit more than just 'orc is required' then though. Alternatively, we could add some kind of 'configuration summary' blurb at the end of configure, similar to what we have in core, which would then say something like: "Optimisations: disabled (needs orc > 0.4.5)"
I agree with Christian that disabling orc without being explicitly told is a bad idea, because it keeps working fine and is just slow. People will never think it's their fault but always blame GStreamer. That said, it'd also be bad for packages. If we up the orc requirement to something that's not in Fedora and I build new packages, the packages will build fine and I won't look at the logs to notice that stuff is suddenly slow.
I have no opinion on the default behavior if Orc is missing. My goal is to allow graceful failure when using Orc, but that doesn't mean that feature has to be used here. Moving the Orc check into a macro ("ORC_CHECK()") in common/m4/orc.m4. Also adding a macro ORC_NOTICE that prints out this at the end of a configure run: configure: *** Orc acceleration disabled. Requires Orc >= 0.4.5, which was not found. Slower code paths will be used. Or other messages as appropriate. The behavior during the Orc check now depends on the --enable-orc flag: If you specify --enable-orc, configure will fail if the required orc is not found. No flag: automatic detection. --disable-orc continues to have the expected result.