[MPlayer-dev-eng] [PATCH] Optimized libmpeg2 routines for ARM/Xscale

[Guillaume Poirier]

Hi,

Siarhei Siamashka wrote:
> On Tuesday 08 May 2007 00:54, Guillaume POIRIER wrote:
> 
> 
>>>In addition, there is a problem with PLD (cache prefetch) instruction, it
>>>is only available on ARMv5TE, but this code is also getting compiled in
>>>when configured for ARMv4. The same problem exists in ffmpeg as well, and
>>>it was raised on ffmpeg-devel mailing list recently.
>>
>>Fine, but you already have it on your fork!
> 
> 
> What fork? ;-) It's more like a branch. I'm subscribed to both mplayer-dev-eng
> and ffmpeg-developers mailing lists since the very beginning and tracking
> what's happening here related to ARM support. All public MPlayer releases
> get imported into maemo garage SVN and ARM/maemo related improvements 
> are initially developed there. When I consider that something is ready for
> inclusion into the official tree, it gets posted here or in the
> ffmpeg-developers mailing list as a patch.
> 
> Branching only public releases is important to save time and be sure that 
> all the regressions introduced in the sequential maemo builds are only my 
> fault. Everything would become much harder if constantly keeping code in 
> sync with official MPlayer SVN trunk. I'm interested not only in bleeding edge
> features and optimizations (which can be always verified and backported), but
> in having reliable and end user ready packages as well.
> 
> Can you suggest a better development model considering that I usually have
> only a handful hours each week (mostly on weekends) to work on this free
> project?

Well, no. ARM is, among architectures that are still in production
(PowerPC, Sparc, x86, IA64) one of the ones (if not THE one) that has
the fewest number of users reporting bugs, and developers maintaining
the code.
So if your development model works for you, and you don't heavily
hack-patch MPlayer, I have no lecture to give you.
I'd however suggest reading part I and II of LWN's article "The
embedded Linux nightmare"
http://lwn.net/Articles/230244/ (I)
http://lwn.net/Articles/232379/ (II)

To put it in a nutshell, it discusses the benefits for people who work
on the embedded Linux world to follow all latest Linux Kernel developers.
I'm not saying I agree with this article, but I think it's a good read.


>>https://garage.maemo.org/plugins/scmsvn/viewcvs.php/trunk/libmpeg2/motion_c
>>omp_arm_s.S?root=mplayer&rev=82&view=markup
> 
> 
> They were kept there as both Nokia 770 and N800 support ARMv5TE 
> instructions and I did not have enough spare time to clean up this code, 
> especially considering the issues with real upstream activity.

Ok. I'd be ok with just disabling that code on ARMv4: we don't know if
there are people who use MPlayer on ARMv4, and if they do, and are
interested in tuning the code, the required modifications are quite small.
BTW, are there still new PDA-like devices that feature ARMv4 in them?


>>I'm open to all suggestions as to what is better to fix this.
> 
> 
> There are 3 options:
> 1. Keep it as is and wait for someone interested in proper armv4 support to
> propose a fix :-)

I'm ok with that. I haven't seen a bug report from an actual ARMv4
user yet.


> 2. Just remove these PLD instructions (but first benchmarking the code to
> estimate how useful they are)

That requires to have access to the hardware. I don't :-(


> 3. Fix this code to conditionally include PLD instructions only when 
> HAVE_ARMV5TE is defined

That's the best solution. I however just don't know GAS enough to do.


>>>Other than that, I don't have any ARM device with IWMMXT support to test
>>>(only have Nokia 770 and N800 here), so can't provide further feedback
>>>regarding IWMMXT code.
>>
>>Neither do I. I'm in contact with a company which seem to be willing
>>to give an xscale-equiped PDA, but I haven't heard from them for a few
>>days...
>>I just figure that if these somewhat clean optimizations don't make it
>>to mainline, there's little chance that people benefit from them,
>>there are chances that people will duplicate this work elsewhere if
>>they don't have it right under their noze (i.e. under MPlayer source
>>tree).
>>
>>Also, since I know little about ARM, trying to get these patches
>>merged is a really good learning experience! :-)
> 
> 
> Having more people with ARM devices to test mplayer/ffmpeg code is always 
> a good news :-)

:-)


>>>Would you like to have ARMv5 optimizations [1] merged in MPlayer as well?
>>>I was not sure if it would be a good idea to submit them to MPlayer
>>>instead of upstream libmpeg2, so did not try that earlier.
>>>
>>>1.
>>>https://garage.maemo.org/plugins/scmsvn/viewcvs.php/trunk/libmpeg2/?root=
>>>mplayer
>>
>>libmpeg2 project doesn't seem too reactive these days. Feel free to
>>send patches to improve libmpeg2 ARM support on MPlayer's fork.
>>I'll do my best to review them and merge them.
> 
> 
> The patch is attached. This code is used in maemo builds since revision 28:
> https://garage.maemo.org/plugins/scmsvn/viewcvs.php?root=mplayer&view=rev&rev=28
> 
> It passed my '-vo md5sum' tests. Also it reduces decoding time to ~105 seconds
> for the video file that was used for older benchmarks:
> http://article.gmane.org/gmane.comp.video.mplayer.devel/43038

This patch is again, very clean and very readable.
It's amazing how with a few macro wizzardness, you can embed
HW-specific optimizations while still having a plain C-like code.

I'd like to check it in in a few days, unless someone disagrees.

Guillaume