After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 620136 - Orc integration
Orc integration
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 0.10.30
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-05-31 03:49 UTC by David Schleef
Modified: 2010-06-09 19:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Main patch (61.29 KB, patch)
2010-05-31 03:49 UTC, David Schleef
needs-work Details | Review
autogenerated code (94.80 KB, patch)
2010-05-31 03:50 UTC, David Schleef
none Details | Review
Orc integration documentation (6.55 KB, text/plain)
2010-05-31 03:51 UTC, David Schleef
  Details
updated patch, all merged together (159.06 KB, patch)
2010-06-07 02:37 UTC, David Schleef
none Details | Review

Description David Schleef 2010-05-31 03:49:50 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?
Comment 1 David Schleef 2010-05-31 03:50:31 UTC
Created attachment 162357 [details] [review]
autogenerated code
Comment 2 David Schleef 2010-05-31 03:51:24 UTC
Created attachment 162358 [details]
Orc integration documentation
Comment 3 Sebastian Dröge (slomo) 2010-05-31 09:23:44 UTC
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)?
Comment 4 Sebastian Dröge (slomo) 2010-05-31 09:24:21 UTC
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 ;)
Comment 5 Tim-Philipp Müller 2010-05-31 17:10:36 UTC
Just FYI, next core/base/good freeze is planned for around June 11-13 (please don't commit this one day before that ;))
Comment 6 David Schleef 2010-05-31 18:45:40 UTC
Also at git://code.entropywave.com/git/gstreamer/gst-plugins-base.git in the orc branch.
Comment 7 David Schleef 2010-05-31 19:42:52 UTC
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
Comment 8 David Schleef 2010-06-02 01:41:27 UTC
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
Comment 9 David Schleef 2010-06-02 07:18:38 UTC
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 10 Stefan Sauer (gstreamer, gtkdoc dev) 2010-06-05 21:04:58 UTC
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.
Comment 11 David Schleef 2010-06-07 02:37:16 UTC
Created attachment 162899 [details] [review]
updated patch, all merged together

Updated patch. Also pushed to the above repository.
Comment 12 David Schleef 2010-06-07 07:26:19 UTC
Updated branch again.  Split up into multiple patches.  Pretty close to what I'll push.
Comment 13 Sebastian Dröge (slomo) 2010-06-07 09:53:28 UTC
Sounds good :) But please make a new orc release before pushing your changes
Comment 14 David Schleef 2010-06-08 07:07:57 UTC
Orc-0.4.5 is out, orc branch pushed.  Please reopen if you notice any buggies.
Comment 15 Tim-Philipp Müller 2010-06-08 08:07:44 UTC
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?
Comment 16 Tim-Philipp Müller 2010-06-08 08:19:23 UTC
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? ;-)).
Comment 17 Tim-Philipp Müller 2010-06-08 12:37:22 UTC
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.
Comment 18 Stefan Sauer (gstreamer, gtkdoc dev) 2010-06-08 13:45:51 UTC
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)
Comment 19 Christian Fredrik Kalager Schaller 2010-06-09 09:43:16 UTC
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.
Comment 20 Tim-Philipp Müller 2010-06-09 10:28:11 UTC
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)"
Comment 21 Benjamin Otte (Company) 2010-06-09 13:01:37 UTC
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.
Comment 22 David Schleef 2010-06-09 19:43:57 UTC
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.