[vlc-devel] [PATCH] add ARM/NEON conversions for audio_filter/channel_mixer/simple

[Rémi Denis-Courmont]

Le mercredi 4 avril 2012 16:03:45 David Geldreich, vous avez écrit :
> Le 4 avr. 2012 à 14:36, Rémi Denis-Courmont a écrit :
> > We have a separate directory for NEON acceleration plugins, aptly named
> > arm_neon. Please stick the code there in a dedicated plugin.
> 
> As j-b mentioned, this is a tradeoff between duplicating the code or
> "polluting" the original file. Also, I am not expert with the VLC code
> architecture so all advices are welcome.

What do you think will happen when someone adds another series of 
optimizations? The plugin code will become unreadable. We've been there and 
done that with the deinterlacer already. No thanks.

Also if someone adds another unaccelerated case, you cannot expect him/her to 
know NEON and be able to write the corresponding assembler.

> >> inline assembly is in separate functions for clarity and will be inlined
> >> by the compiler
> > 
> > Yes but inlined assembler is harder to read and it cannot selected at
> > run-time. The overhead of a function is neglible here. Inlining assembler
> > makes sense if you want to mix C code, especially for branching. But you
> > have already implemented branching in assembler anyway. So you might as
> > well use a dedicated assembler source file, then.
> 
> There is some branching in the DoWork function to switch between all the
> input/output combinations

In other C functions, so totally irrelevant and orthogonal to my point.

Besides, all those if's really should be done once during plugin instantiation 
rather than at every filtering iteration. I do not expect you to clean up the 
existing code, but please don't reproduce the same mistakes again.

-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis