Closed Bug 832390 Opened 11 years ago Closed 10 years ago

Enable arm neon for libpng

Categories

(Core :: Graphics: ImageLib, defect)

Product:
Core
Component:
Graphics: ImageLib
Platform:
ARM
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29

People

(Reporter: romaxa, Assigned: glennrp+bmo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 58 obsolete files)

v59-832390-enable-arm.diff
22.02 KB, patch
glandium
: review+
glandium
: feedback+
Details | Diff | Splinter Review 
freemangordon from maemo community noticed that libpng library does not have arm neon optimizations enabled...
http://mxr.mozilla.org/mozilla-central/source/media/libpng/pngrutil.c#3841 - has some arm neon define
which supposed to use png_read_filter_row_sub3_neon
But I don't see that in tree, seems .S files are missing and possible something else
The .S file is in the libpng/arm directory, which has not been checked in to mozilla.  It is available in the libpng distributions from libpng.sourceforge.net.  Grab it from the libpng GIT repo at SourceForge, libpng15 branch, if that's easier.
actual question is why that is not in mozilla enabled state... was there are any problems with that? or just lack of time?
if no problems then it make sense to push it, enable it and get perf improvement for Android/B2G...
I could push the stuff, but someone else will have to do the testing.
The ARM support in libpng-1.5.14 will be slightly different; libpng-1.5.14
will be out in about a week.  I'd prefer to wait for that, unless you don't
mind if I patch in the "rc" version for testing. (taking bug.)
Assignee: nobody → glennrp+bmo
Status: NEW → ASSIGNED
Blocks: 832487
No longer blocks: 832487
Depends on: 832487
I have uploaded a patch to bug #832487 (update libpng to 1.5.14) which includes the ARM stuff.
Assignee: glennrp+bmo → romaxa

        Attachment #706841 -
        Flags: review?(glennrp+bmo)

    
    

    
  
Comment on attachment 706841 [details] [diff] [review]
Enable arm neon png implementation

Review of attachment 706841 [details] [diff] [review]:
-----------------------------------------------------------------

It looks OK to me (just reading the source) but I'm unable to test and don't feel qualified to review it.  I assume you have had to manually change the png_ prefixes to MOZ_PNG because the compiler failed to do C preprocessing.

    
  
Blocks: 462796

    
    

    
  
Is there a plan to use ARM NEON code with zlib also?  I know that a patch for zlib exists, but I don't see anything in our MXR about that. Oleg, do you want to open a separate bug for that?

    
    

    
  
I discussed this with John Bowler and we believe that instead of
manually changing the function name prefixes from "png_" to "MOZ_PNG_"
we can do this:

insert near the top of arm/filter_neon.S, at line 23:

 #define PNG_VERSION_INFO_ONLY
 #include "png.h"

Also put the same two lines at the top of arm/arm_init.c, replacing line 12
that includes ../pngpriv.h.

Oleg, please give that a try.  If it works, we'll do that upstream in
libpng-1.5.15 and later.

    
    

    
  
My mistake.  Don't change arm/arm_init.c; only add the two lines
to arm/filter_neon.S

    
    

    
  


  
Since the filter*neon functions aren't exported, the simplest solution is just to not rename them in mozpngconf.h.  In fact there are a lot of other things renamed in mozpngconf.h that don't need to be, but that's for another bug, or maybe just wait for libpng-1.5.15 to fix.

        Attachment #706841 -
        Attachment is obsolete: true

        Attachment #706841 -
        Flags: review?(glennrp+bmo)

    
  

        Attachment #707092 -
        Flags: review?(joe)

    
    

    
  
Comment on attachment 707092 [details] [diff] [review]
v01 enable ARM without modifying arm/filter_neon.S

Review of attachment 707092 [details] [diff] [review]:
-----------------------------------------------------------------

I see no issues with this, but I also don't know whether changing Makefile.in like this will cause issues - so off to gps.

        Attachment #707092 -
        Flags: review?(joe)

        Attachment #707092 -
        Flags: review?(gps)

        Attachment #707092 -
        Flags: review+

    
    

    
  
Comment on attachment 707092 [details] [diff] [review]
v01 enable ARM without modifying arm/filter_neon.S

Review of attachment 707092 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libpng/Makefile.in
@@ +26,5 @@
> +# This may be fixable if clang's ARM/iOS assembler improves into a
> +# viable solution in the future.
> +ifneq (Darwin,$(OS_ARCH))
> +ifdef HAVE_ARM_NEON
> +USE_ARM_NEON_GCC=1

This seems like the kind of thing we should have in configure. What are your thoughts on that?

    
    

    
  
Here's the sad thing: the libpng code detect neon support with /proc/self/auxv, and /proc/self/auxv is not readable on most android devices.

    
    

    
  
Does Android set __linux__?

That's what we check, the BSDs don't set it (and they don't have /proc).

    
    

    
  
Android *is* a Linux system, so yes, __linux__ is set. And without runtime detection, we can't enable neon on Android builds, because not all ARMv7 devices have neon (Tegra 2, i'm looking at you).

    
    

    
  
Hum...  I found this web page:

http://www.kandroid.org/ndk/docs/CPU-ARM-NEON.html

I admit this is a major ARM Ltd fuckup, and multiple other easily googleable pages say the same thing in different ways.  It clearly needs to be upstreamed to ARM Ltd.

So far as libpng is concerned we're not ARM gurus, we're not Android gurus and we're not Linux gurus; we're libpng gurus and, maybe, PNG gurus.  Which means we simply don't have test systems to engineer or even validate fixes for problems like this.  We have to take them on trust.

An Android specific patch, along the lines of the current supposedly __linux__ specific patch, would be welcome.

John Bowler <jbowler@acm.org>

    
    

    
  
(In reply to jbowler@acm.org from comment #16)
> An Android specific patch, along the lines of the current supposedly
> __linux__ specific patch, would be welcome.

You can just replace the __linux__ specific code with something that reads /proc/cpuinfo instead of /proc/self/auxv. You can check the pixman code, for example, that is what it does.

    
    

    
  
(In reply to Gregory Szorc [:gps] from comment #12)
> Comment on attachment 707092 [details] [diff] [review]
> v01 enable ARM without modifying arm/filter_neon.S
> 
> Review of attachment 707092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libpng/Makefile.in
> @@ +26,5 @@
> > +# This may be fixable if clang's ARM/iOS assembler improves into a
> > +# viable solution in the future.
> > +ifneq (Darwin,$(OS_ARCH))
> > +ifdef HAVE_ARM_NEON
> > +USE_ARM_NEON_GCC=1
> 
> This seems like the kind of thing we should have in configure. What are your
> thoughts on that?

Not even touching the subject of whether this is food for configure.in, it's wrong to use a platform check for what is intended to be a compiler/toolchain check. And I do realize this basically comes from the pixman makefile, which is thus wrong too.

    
    

    
  
Mike Hommey; you are missing the essence of what I was trying to say:

>You can just replace the __linux__ specific code with 

No; we cannot.  The people you are talking to (Glenn and me) really don't have the faintest idea what you are talking about.

We are not gurus in the area in which you require a guru.

A guru in the appropriate area needs to submit a patch that works.  That patch will be reviewed with regard to compatibility with all the other systems that we can test on (which, basically, means the entire universe except Windows RT) and then added to the source.

It's a real bug, it needs a patch.

    
    

    
  
(In reply to jbowler@acm.org from comment #19)
> Mike Hommey; you are missing the essence of what I was trying to say:
> 
> >You can just replace the __linux__ specific code with 
> 
> No; we cannot.  The people you are talking to (Glenn and me) really don't
> have the faintest idea what you are talking about.
> 
> We are not gurus in the area in which you require a guru.

You don't need a guru. You need someone able to write code that can search for a string in a file (essentially, check that "neon" can be found in the line starting with "Features" in /proc/cpuinfo)

    
    

    
  
https://bugzilla.mozilla.org/show_bug.cgi?id=583958

Unfortunately it's GPLed, and libpng isn't.  So it's necessary to find the documentation somewhere so that it can be written from scratch, or, better, for someone who is the author of one of those pieces of code (and can therefore find a copy from before it got GPLed) to submit a patch.

Code reviewing the first patch it seems to have major flaws; specifically it will crash if /proc/cpuinfo has 1024 or more characters in it.  (On my machine /proc/cpuinfo has 3824 characters.)

Code reviewing the alternative patch it's much better written; it checks for Features at the start of line, rather than just relying on the appearance of "neon" in the first 1024 characters.

Bear in mind that this code gets executed once every time for every time a PNG is read, including cases where the PNG data is cached in memory.  That means ever time a PNG is decompressed for display a OS level file open happens, a stdio(3) file buffer (typically 4096 bytes) is allocated, an OS level read of either the full file or the buffer happens, the buffer contents are parsed looking for '\n' (fgets) and the result copied to a separate stack buffer (up to 511 byes) which is then searched for the relevant strings and finally the malloc stdio buffer is freed and the OS file is closed.

Of course the same applies to the current code, so I don't think it is any more of a problem, but I find it difficult to believe that this can result in a performance improvement when reading a typical PNG file (around 64x64 bytes.)

    
    

    
  
(In reply to jbowler@acm.org from comment #21)
> https://bugzilla.mozilla.org/show_bug.cgi?id=583958
> 
> Unfortunately it's GPLed, and libpng isn't.  So it's necessary to find the
> documentation somewhere so that it can be written from scratch, or, better,
> for someone who is the author of one of those pieces of code (and can
> therefore find a copy from before it got GPLed) to submit a patch.
> 
> Code reviewing the first patch it seems to have major flaws; specifically it
> will crash if /proc/cpuinfo has 1024 or more characters in it.  (On my
> machine /proc/cpuinfo has 3824 characters.)

It's much smaller on arm machines, fwiw.

> Code reviewing the alternative patch it's much better written; it checks for
> Features at the start of line, rather than just relying on the appearance of
> "neon" in the first 1024 characters.
> 
> Bear in mind that this code gets executed once every time for every time a
> PNG is read, including cases where the PNG data is cached in memory.  That
> means ever time a PNG is decompressed for display a OS level file open
> happens, a stdio(3) file buffer (typically 4096 bytes) is allocated, an OS
> level read of either the full file or the buffer happens, the buffer
> contents are parsed looking for '\n' (fgets) and the result copied to a
> separate stack buffer (up to 511 byes) which is then searched for the
> relevant strings and finally the malloc stdio buffer is freed and the OS
> file is closed.
> 
> Of course the same applies to the current code, so I don't think it is any
> more of a problem, but I find it difficult to believe that this can result
> in a performance improvement when reading a typical PNG file (around 64x64
> bytes.)

It doesn't have to run every time. In fact, the code in gecko just runs once and stores the value in a variable.

    
    

    
  


  
http://www.kandroid.org/ndk/docs/CPU-ARM-NEON.html

I've got no way to even compile this, so there may be typos.  The patch applies to the libpng15 GIT head, libpng 1.5.15beta04, though since arm/arm_init.c hasn't changed much recently that shouldn't matter.

    
    

    
  
Comment on attachment 707092 [details] [diff] [review]
v01 enable ARM without modifying arm/filter_neon.S

I'm deferring to Mike since he had opinions expressed in the comments.

        Attachment #707092 -
        Flags: review?(gps) → review?(mh+mozilla)

    
    

    
  
Comment on attachment 707092 [details] [diff] [review]
v01 enable ARM without modifying arm/filter_neon.S

Review of attachment 707092 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the fpu flag figured out. Please file a separate bug for the /proc/self/auxv issue.

::: media/libpng/Makefile.in
@@ +26,5 @@
> +# This may be fixable if clang's ARM/iOS assembler improves into a
> +# viable solution in the future.
> +ifneq (Darwin,$(OS_ARCH))
> +ifdef HAVE_ARM_NEON
> +USE_ARM_NEON_GCC=1

What Gregory mentions should be addressed, but that can be done in a followup (considering this was just stolen from libpixman).

@@ +59,5 @@
>  
> +ifdef USE_ARM_NEON_GCC
> +CSRCS += arm_init.c
> +SSRCS += filter_neon.S
> +ARM_NEON_CFLAGS = -mfpu=neon

This in itself won't do what you want. You need something like the following instead:
filter_neon.$(OBJ_SUFFIX): ASFLAGS += -mfpu=neon

        Attachment #707092 -
        Flags: review?(mh+mozilla) → review+

    
    

    
  
(In reply to Mike Hommey [:glandium] from comment #25)

> Please file a separate bug for the
> /proc/self/auxv issue.

Libpng is working on this issue upstream, so there's probably no need for a separate bug.  We expect to have this worked out in libpng-1.5.15 and 1.6.1.  Both will be released in a few weeks.

    
  

        Attachment #714424 -
        Attachment is obsolete: true

    
  
Depends on: 858478

    
  
Depends on: 858578
No longer depends on: 858478

    
    

    
  


  
Revised patch to work with libpng-1.5.15 and later.  Untested.

        Attachment #707092 -
        Attachment is obsolete: true

    
  
Depends on: 873001

    
    

    
  
This may need revisions due to the ongoing moz.build conversion work.

    
    

    
  
Right, and it's also possible that there is some bit rot due to checkin of libpng-1.5.16 tonight.

    
    

    
  
It appears that the v02 patch won't work because it does not #define PNG_ALIGNED_MEMORY_SUPPORTED nor PNG_ARM_NEON_SUPPORTED.  Other #defines
that might be needed are PNG_ARM_NEON_CHECK_SUPPORTED and/or PNG_ARM_NEON_API_SUPPORTED, if we want to have run-time detection of ARM/NEON support.

    
    

    
  


  
Revised patch to work with libpng-1.5.16 and moz.build

        Attachment #737926 -
        Attachment is obsolete: true

    
  
OS: Windows 8 → All
Hardware: x86 → All

    
    

    
  
Need tryserver run
Flags: needinfo?(ryanvm)

    
    

    
  
With arm_init.c in the right place in moz.build.
https://tbpl.mozilla.org/?tree=Try&rev=f3c50bc2904b

Also, the commit message is pointing to the wrong bug #.

    
    

    
  
And one more using ASFILES instead of SSRCS.
https://tbpl.mozilla.org/?tree=Try&rev=fab2a826f67b

    
    

    
  


  
Fixed moz.build to look in the arm subdirectory for the arm sources

        Attachment #759565 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
make[5]: Entering directory `/builds/slave/try-ics_a7_g-00000000000000000/build/obj-b2g/media/libpng'
/builds/slave/try-ics_a7_g-00000000000000000/build/config/rules.mk:1123: target `filter_neon.S' doesn't match the target pattern
mkdir -p ".deps/"
/builds/slave/try-ics_a7_g-00000000000000000/build/gonk-toolchain/prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-gcc -o filter_neon.S -march=armv7-a -mthumb -mfpu=vfp -mfloat-abi=softfp -fPIC -Wa,--noexecstack -include ../../mozilla-config.h -DMOZILLA_CLIENT  -g -c /builds/slave/try-ics_a7_g-00000000000000000/build/obj-b2g/media/libpng/backend.mk
arm-linux-androideabi-gcc: /builds/slave/try-ics_a7_g-00000000000000000/build/obj-b2g/media/libpng/backend.mk: linker input file unused because linking not done
png.c
/bu
Flags: needinfo?(ryanvm)

    
    

    
  
https://tbpl.mozilla.org/?tree=Try&rev=6a8d657e4403

With bug # fixed and arm_init.c moved up in the moz.build list again.

    
    

    
  
qref would help, though.
https://hg.mozilla.org/try/rev/a4ad45ec4f2f

(Please fix these locally...)

    
    

    
  


  

        Attachment #759725 -
        Attachment is obsolete: true

    
    

    
  


  
CSRCS in alphabetical order, bug number fixed

        Attachment #759730 -
        Attachment is obsolete: true

    
    

    
  
Busted. Presumably you need a moz.build file in the arm directory including the sources rather than the root directory. Also, I'm assuming this'll need appropriate ifdeffing so we don't try to build this on !arm?

    
    

    
  


  
Account for MOZ_PNG prefix in function name, in mozpngconf.h

        Attachment #759733 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Ryan VanderMeulen [:RyanVM] from comment #43)
> Busted. Presumably you need a moz.build file in the arm directory including
> the sources rather than the root directory.

OK

> Also, I'm assuming this'll need
> appropriate ifdeffing so we don't try to build this on !arm?

OK, but there are ifdef's inside the two files that don't leave much in !arm.
From media/libjpeg/moz.build, it looks like it would be some kind of

   if[config] LIBPNG_ARM_ASM [...]

rather than ifdef, though, and mimic in
configure.in the stuff that defines LIBJPEG_TURBO_ARM_ASM

    
    

    
  
arm/* build fine on x86 - that's why the 'mark stack as non-executable' .section is outside the #ifdef PNG_ARM_NEON_SUPPORTED, nevertheless it's better not to build them if possible.

    
    

    
  


  
Made a separate arm/moz.build file with DIRS='arm' defined in libpng top level moz.build

        Attachment #759738 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
You need to hg add arm/moz.build. Fixed locally for the Try push.

https://tbpl.mozilla.org/?tree=Try&rev=78f2de4cbae8
Flags: needinfo?(ryanvm)

    
    

    
  
make[6]: Entering directory `/builds/slave/try-ics_a7_g-00000000000000000/build/obj-b2g/media/libpng/arm'
/builds/slave/try-ics_a7_g-00000000000000000/build/config/rules.mk:1123: target `filter_neon.S' doesn't match the target pattern
make[6]: Nothing to be done for `libs'.
make[6]: Leaving directory `/builds/slave/try-ics_a7_g-00000000000000000/build/obj-b2g/media/libpng/arm'

    
    

    
  
I don't know what that means.  Is it looking for filter_neon.asm perhaps?
Flags: needinfo?

    
    

    
  
Looks to me like it should be in SSRCS not ASFILES. Ryan can you do a quick replace and resubmit?
Flags: needinfo? → needinfo?(ryanvm)

    
    

    
  
SSRCS hasn't been ported to moz.build yet. We're going to need that in a makefile :(
Flags: needinfo?(ryanvm)

    
    

    
  


  
Add SSRCS to Makefile.in, commented out from arm/moz.build

        Attachment #759786 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
Is that going to work? I would think SSRCS needs to be defined in the arm directory.
Flags: needinfo?(ryanvm)

    
    

    
  
I thought VPATH would take care of that.

    
    

    
  
Now that the first patch from bug 880773 has landed, we should be able to use SSRCS inside moz.build files.

    
    

    
  
(In reply to Glenn Randers-Pehrson from comment #55)
> I thought VPATH would take care of that.

Please don't rely on VPATH. We are trying to move away from it. See bug 875013.

    
  
Depends on: 886499

    
    

    
  
libpng-1.5.17 is out and a patch is in bug #866499.  The two functions in the "arm" subdirectory now look for PNG_ARM_NEON_OPT > 0 instead of PNG_ARM_NEON_SUPPORTED for conditional compilation, so the v10 patch will need to be revised once libpng-1.5.17 is checked in.

    
    

    
  


  
Libpng-1.5.17 has been checked in.  It accomplishes most of what the v10 patch did; however, it does not enable run-time checking for ARM_NEON support.  Also, libpng depends upon "__ARM_NEON__" being defined, while our moz.build depends upon a configuration variable that depends upon "$CPU_ARCH" = "arm".  This "v12" patch makes the bundled libpng depend upon the same configuration variable, MOZ_PNG_ARM_NEON. I don't know if this will actually make a difference on any platform, but it seems more consistent.

        Attachment #759847 -
        Attachment is obsolete: true

    
    

    
  
Should I have added this to moz.build?

    CFLAGS += [
       '-mfpu=neon',
       '-march=armv7-a',
    ]

The equivalent appears elsewhere in configure.in

    
    

    
  
You don't want these flags on all files, do you?

    
    

    
  
No, but if I put them in libpng/arm/moz.build it'll only affect the two files in that subdirectory, right?

    
    

    
  
It you say -mfpu=neon then __ARM_NEON__ is set.  If you build with this you need to be absolutely certain every machine that runs the code will have a NEON capable FPU.

I don't think the CPU architecture is relevant.  So far as I could determine (sample of 1) setting an arch appropriate to a CPU which *does* have a NEON capable FPU does not cause GCC to generate NEON instructions (well, to set __ARM_NEON__.)

It's probably moot because it's not a library issue; if the build is targeting systems that *all* have NEON then the build should be setting -mfpu=neon.  If not then it is impossible to determine whether NEON is there at runtime without special casing each OS *and* sub-OS (Android != Linux for example.)

John Bowler

    
    

    
  
Appears that sandbox_symbols.py doesn't know about CFLAGS (or any sort of compiler FLAGS), so my suggestion in comment #60 probably won't work.  CFLAGS will have to be defined in a Makefile.in instead.

    
    

    
  
(In reply to Glenn Randers-Pehrson from comment #64)
> Appears that sandbox_symbols.py doesn't know about CFLAGS (or any sort of
> compiler FLAGS), so my suggestion in comment #60 probably won't work. 
> CFLAGS will have to be defined in a Makefile.in instead.

It soon will, FWIW.

(In reply to Glenn Randers-Pehrson from comment #62)
> No, but if I put them in libpng/arm/moz.build it'll only affect the two
> files in that subdirectory, right?

Yes, if you do mean for all these files to be built with those flags.

    
    

    
  


  
Attempt to build in the arm subdirectory using arm/Makefile.in and arm/moz.build
This does not yet address setting the -march and -mfpu compiler flags.  Someone please submit to the try server.

        Attachment #778224 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)
Flags: needinfo?(joe)

    
    

    
  
The "if CONFIG[]" tests in arm/moz.build of patch v13 are redundant.  That doesn't matter for the purpose of testing but I'll remove them from the next patch, after seeing the try server results.

    
    

    
  
(In reply to jbowler@acm.org from comment #63)
> It you say -mfpu=neon then __ARM_NEON__ is set.  If you build with this you
> need to be absolutely certain every machine that runs the code will have a
> NEON capable FPU.

This is 100% not the case. Which makes me wonder...

(In reply to Glenn Randers-Pehrson from comment #59)
> Libpng-1.5.17 has been checked in.  It accomplishes most of what the v10
> patch did; however, it does not enable run-time checking for ARM_NEON
> support.

Does this imply that we'll SIGILL on ARM CPUs without NEON?

    
    

    
  
(In reply to jbowler@acm.org from comment #63)
> It you say -mfpu=neon then __ARM_NEON__ is set.  If you build with this you
> need to be absolutely certain every machine that runs the code will have a
> NEON capable FPU.

Patch v13 has this in mozpngconf.h:
#ifdef MOZ_PNG_HAVE_ARM_NEON
#  define PNG_ARM_NEON_OPT 2
#  define PNG_ARM_NEON_CHECK_SUPPORTED
#  define PNG_ALIGNED_MEMORY_SUPPORTED
#endif

So if the CPU is not neon-capable, then the runtime png_have_neon(),
which is enabled by PNG_ARM_NEON_CHECK_SUPPORTED in arm/arm_init.c will
find that out, won't it?

This does raise a problem if mozilla's "configure.in" does not
set CPU_ARCH=="arm" but __ARM_NEON__ is defined; in this case we'll
fall through to the test in pngpriv.h and maybe get a different
answer.

I'll add

    #else
    #  define PNG_NEON_OPT 0

to prevent that.

    
    

    
  
(In reply to Joe Drew (:JOEDREW! \o/) from comment #68)
>
> Does this imply that we'll SIGILL on ARM CPUs without NEON?

Yes; you just told the compiler it has a NEON FPU!  If the code does any floating point or (because the FPU also supports integer SIMD instructions) anything that the compiler can vectorize it may generate instructions that don't exist on a non-NEON system.

libpng has a lot of floating point (unless you build -DPNG_NO_FLOATING_POINT) and a lot of very very vectorizable stuff.

John Bowler
Flags: needinfo?(joe)

    
    

    
  


  
Revised patch per comment #67 and comment #69

        Attachment #778475 -
        Attachment is obsolete: true

    
    

    
  
(In reply to jbowler@acm.org from comment #70)

> libpng has a lot of floating point (unless you build
> -DPNG_NO_FLOATING_POINT) and a lot of very very vectorizable stuff.

We don't build with PNG_NO_FLOATING_POINT.  In mozpngconf.h
we support both fixed and floating point:

    #define PNG_FIXED_POINT_SUPPORTED
    #define PNG_FLOATING_ARITHMETIC_SUPPORTED
    #define PNG_FLOATING_POINT_SUPPORTED

    
    

    
  
(In reply to Glenn Randers-Pehrson from comment #69)
> Patch v13 has this in mozpngconf.h:
> #ifdef MOZ_PNG_HAVE_ARM_NEON
> #  define PNG_ARM_NEON_OPT 2

That says "I *unconditionally* have NEON capability."  Surely you mean "1"?  (See the comments in scripts/pnglibconf.dfa).  As it says there, it's the same as -mfpu=neon.

> #  define PNG_ARM_NEON_CHECK_SUPPORTED
> #  define PNG_ALIGNED_MEMORY_SUPPORTED

If you then do this you turn on the checks, but you tell the app that NEON is on unconditionally.

mozpngconf.h needs to follow the rules for pnglibconf.h:

0: the libpng builder disabled the NEON assembler, even though -mfpu=neon might be set during a build (e.g. for iOS, with -multi-arch, and the system builder doesn't want to have the overhead of the multi-arch binary in this case.)

1: set if one of the run-time checks (ARM_NEON_API, ARM_NEON_CHECK)is enabled.

2: set in pnglibconf.h if -mfpu=neon was specified during the build (e.g. with -multi-arch - this is the iOS support.)

John Bowler

    
    

    
  
> 2: set in pnglibconf.h if -mfpu=neon was specified during the build (e.g.
____________^^^^^^^^^^^^

should be pngpriv.h

> with -multi-arch - this is the iOS support.)

    
    

    
  


  
#  define PNG_ARM_NEON_OPT 1
(instead of 2, per comment #73)
Please submit v15 to the try server.

        Attachment #778491 -
        Attachment is obsolete: true
Flags: needinfo?(joe)

    
  
Flags: needinfo?(joe)

    
    

    
  
Bear with me; it'll take a while for me to learn that it's my job, not the computer's, to put lists in alphabetical order.

    
    

    
  


  
Alphabetized the DEFINES.

        Attachment #778525 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
Try server says

   in function MOZ_PNG_read_filt_row:../../../gecko/media/libpng/pngrutil.c:3873:
   error: undefined reference to 'MOZ_PNG_init_filt_func_neon'

I saw this before in bug #886499, v03 patch, where the system somehow failed to descend into the arm subdirectory and build the two files there (one of them supplies MOZ_PNG_init_filt_func_neon (which is renamed in mozpngconf.h from png_init_filter_functions_neon).

Here's a hint in the full log:
  ../../../gecko/media/libpng/pngrutil.c:17:
  ../../../gecko/media/libpng/mozpngconf.h:27:1: warning: "PNG_ARM_NEON_OPT" redefined
  In file included from ../../../gecko/media/libpng/pngrutil.c:17: 
  WARNING -  ../../../gecko/media/libpng/pngpriv.h:92:1: warning: this is the location of the previous definition
  INFO -  BUILDSTATUS TIERDIR_FINISH media/libpng

It seems that pngpriv.h is being accessed before mozpngconf.h is, which is
not what I was expecting.  There's no indication in the log that it attempted to compile arm/*.c, *.S.  This must be a situation where the OS is "arm" but __ARM_NEON" is not defined.  The build in question is B2G_try_panda_dep.

I'll update the patch to make sure mozpngconf.h is hit before the ARM stuff in pngpriv.h.

    
    

    
  
PS the Android builds all seem to have succeeded.  There was the same warning about the macro being redefined, and then this:

make -C arm libs
make[6]: Entering directory `/builds/slave/try-and-a6-0000000000000000000/build/obj-firefox/media/libpng/arm'
make[6]: Nothing to be done for `libs'.
make[6]: Leaving directory `/builds/slave/try-and-a6-0000000000000000000/build/obj-firefox/media/libpng/arm'
make[5]: Leaving directory `/builds/slave/try-and-a6-0000000000000000000/build/obj-firefox/media/libpng'

I can't tell from that whether anything was built in the arm subdirectory but it does not look that way.

    
    

    
  


  
Relocated "#include mozpngconf.h" ahead of ARM_NEON code in pngpriv.h

        Attachment #778683 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
https://tbpl.mozilla.org/?tree=Try&rev=c5d75bef1cd8
Assignee: romaxa → glennrp+bmo
Flags: needinfo?(ryanvm)

    
    

    
  
The v17 patch did not work.

In file included from ../../../image/src/RasterImage.cpp:29:0:
../../../image/decoders/nsPNGDecoder.h: In member function 'bool mozilla::image::nsPNGDecoder::IsValidICO() const':
../../../image/decoders/nsPNGDecoder.h:47:17: error: 'LIBPNG_WAS_COMPILED_WITH__PNG_NO_SETJMP' was not declared in this scope
../../../image/decoders/nsPNGDecoder.h:47:57: error: 'setjmp' was not declared in this scope
make[6]: *** [RasterImage.o] Error 1

I'll revert v17 and try a different approach.

    
    

    
  


  
Put #include mozpngconf.h back where it was, and added some #undef directives to mozpngconf.h to avoid the redefinition warning.

        Attachment #778736 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
We seem to be back to the situation where nothing gets compiled in the "arm" subdirectory (as in comment #80), but now without the warnings about macros
being redefined:

 pngrutil.c:3873: error: undefined reference to 'MOZ_PNG_init_filt_func_neon',
but not every time:

 Android 2.2 x86 opt B [green]
 Android 2.2 Armv6 opt B [red]
 Android 2.2 NoIon opt B [red]

    
    

    
  


  
One last try: combine libpng/moz.build with libpng/arm/moz.build, as in bug #886499.

        Attachment #778769 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
Three issues:

1) You must *NOT* define __ARM_NEON__: unless you have patched pngpriv.h you can't get to line 92 unless it is defined.

2) For Android builds to work you *must* link to the cpufeatures library if the NEON checks are enabled.

3) libpng15 is broken - compare with libpng16.  The "Compile time options" section in pngpriv.h needs to be after the #include (direct or indirect) of pnglibconf.h.  This was fixed in libpng16 to support the official version of symbol prefixing (not supported in libpng15).

What (3) means is that ARM NEON isn't going to work in libpng15 (but then it never did) - the order of include mess needs to be fixed.  This was done in libpng16 by causing pngpriv.h to pre-include pnglibconf.h.

John Bowler

    
    

    
  


  
Added link to Android cpu-features

        Attachment #778783 -
        Attachment is obsolete: true

    
    

    
  
ping: still needing a try run of the v20 patch

    
  
Blocks: 841734

    
    

    
  
Crap, sorry about that :(
https://tbpl.mozilla.org/?tree=Try&rev=d57caf4f3a13
Flags: needinfo?(ryanvm)

    
    

    
  
It got a little further but still busted on the Android platforms.

    
    

    
  


  
Removed renaming macros for NEON functions from mozlibpng.h

        Attachment #778869 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  


  
Overwrite neon filter functions from pngpriv.h, use those in mozpngconf.h instead.

    
  

        Attachment #781270 -
        Attachment is obsolete: true

    
    

    
  


  
v22 is busted on the ARM platforms.  v23 eliminates the renaming of neon functions in mozpngconf.h but I don't have really high hopes for that.  It may be that this just cannot be made to work as stated in comment # 89.

        Attachment #782164 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  


  
Compile entire png.h with arm/filter_neon.S to get the exported file prototypes

    
    

    
  
Comment on attachment 783874 [details] [diff] [review]
v24 Enable ARM optimization with libpng-1.5.17 and later

Please only submit the v23 patch to Try.  After discussion with John Bowler it seems the v24 patch (marking it obsolete) is unlikely to work.

        Attachment #783874 -
        Attachment is obsolete: true

    
    

    
  
These DEFINES from moz.build are passed to the compiler for arm/arm_init.c
but not to the assembler for arm/filter_neon.S:

-DMOZ_PNG_READ -DMOZ_PNG_WRITE -DMOZ_PNG_HAVE_ARM_NEON

Because of -DMOZ_PNG_HAVE_ARM_NEON being missing, the neon functions are
not assembled and loaded into the library.

I guess this is some sort of shortcoming in the SSRCS processing of moz.build.

    
    

    
  
(In reply to Glenn Randers-Pehrson from comment #101)
> These DEFINES from moz.build are passed to the compiler for arm/arm_init.c
> but not to the assembler for arm/filter_neon.S:
> 
> -DMOZ_PNG_READ -DMOZ_PNG_WRITE -DMOZ_PNG_HAVE_ARM_NEON
> 
> Because of -DMOZ_PNG_HAVE_ARM_NEON being missing, the neon functions are
> not assembled and loaded into the library.
> 
> I guess this is some sort of shortcoming in the SSRCS processing of
> moz.build.

gps, any ideas?
Flags: needinfo?(gps)

    
  
Depends on: 874266

    
    

    
  
(In reply to Glenn Randers-Pehrson from comment #101)
> These DEFINES from moz.build are passed to the compiler for arm/arm_init.c
> but not to the assembler for arm/filter_neon.S:
> 
> -DMOZ_PNG_READ -DMOZ_PNG_WRITE -DMOZ_PNG_HAVE_ARM_NEON
> 
> Because of -DMOZ_PNG_HAVE_ARM_NEON being missing, the neon functions are
> not assembled and loaded into the library.
> 
> I guess this is some sort of shortcoming in the SSRCS processing of
> moz.build.

I don't think this has to do with moz.build, but the SSRCS rules in config/rules.mk, which is:

$(SOBJS):
        $(AS) -o $@ $(ASFLAGS) $(LOCAL_INCLUDES) $(TARGET_LOCAL_INCLUDES) -c $<

None of these variables contain DEFINES, so they won't show up in the command-line. I don't know if it makes sense to add DEFINES to this rule, or to add the -D flags to ASFLAGS (ASFLAGS isn't in moz.build yet, by the way).

    
    

    
  


  
temporarily moved SSRCS and associated DEFINES back into Makefile.in, until ASFLAGS becomes available.
Flags: needinfo?(ryanvm)

    
    

    
  


  
Restored NEON function renaming in mozpngconf.h; that was not the cause of problems.

        Attachment #785202 -
        Attachment is obsolete: true

    
    

    
  


  
Made PNG ARM NEON support configurable with
--enable-png-arm-neon-support=off, check (default), or nocheck
"nocheck" is faster and has a smaller footprint but the builder must
guarantee that the target CPU has the required ARM support.

        Attachment #785403 -
        Attachment is obsolete: true

    
    

    
  
Oops, in comment#106 the option is "no", not "off", to disable ARM support even if the target CPU has it.

    
  
No longer depends on: 832487, 858578, 873001

    
  

        Attachment #783422 -
        Attachment is obsolete: true

    
    

    
  
What mshal said. This has nothing to do with moz.build and everything to do with rules.mk not passing DEFINES into the assembler.

We should either add DEFINES into the assembler invocation or create a new variable (say AS_DEFINES) that collects just the assembler defines. The former is certainly easier. But I'm not sure if there will be any undesired side-effects from passing defines into the assembler. I want to say no.

Long term this should be fixable with bug 879837.
Flags: needinfo?(gps)

    
    

    
  
From the libpng perspective it doesn't matter much whether it's DEFINES or AS_DEFINES (there would be fewer of the latter because we only need one of them).  Also we could work around the status quo if need be. I'll go ahead and generate a patch to test that assertion.

    
    

    
  


  
Removed test on PNG_ARM_NEON_OPT from
arm/filter_neon.S because moz.build does not
pass DEFINES to the assembler (bug #832390)

Depend on mozilla's "configure" instead of this
#define to avoid assembling the code when it's
unwanted.

Please submit to the Try server.

        Attachment #785440 -
        Attachment is obsolete: true

    
    

    
  


  
Fixed typo introduced in the v28 patch, in configure.in

        Attachment #785914 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
The v29 patch worked on Fedora, Ubuntu, and Android platforms.  However, the three B2G platforms tested failed to find the cpufeatures program needed for runtime identification of the ARM_NEON capability.  How can we detect ARM_NEON capability on those B2G platforms?
Flags: needinfo?

    
    

    
  


  
Disable ARM detection and use of ARM optimization on B2G for now.

        Attachment #786348 -
        Attachment is obsolete: true
Flags: needinfo?

    
  
Flags: needinfo?(ryanvm)

    
    

    
  
needinfo? gps on comment 114
Flags: needinfo?(ryanvm) → needinfo?(gps)

    
    

    
  
I'm not sure what the "cpufeatures program" is, but the references in the tree I found are related to WebRTC (https://mxr.mozilla.org/mozilla-central/search?string=cpufeatures). If this is the "cpufeatures program," B2G not having WebRTC would seem like the issue with those platforms. If "cpufeatures" is coming from elsewhere, you may need to talk to RelEng about installing a package on the B2G builders.

That being said, "cpufeatures" sounds like it is querying capabilities of the currently-used CPU. Obviously the CPU on the building machine might be different from the run-time machine. In these cases, all CPU decisions should be performed in configure and should respect the build configuration and not optimize for the building machine.
Flags: needinfo?(gps)

    
    

    
  
cpufeatures is part of the Android NDK.  B2G claims to be Android but seemingly does not provide the cpufeatures library.  Yes, its purpose is to query the CPU at runtime.  On real Android platforms, the source code for the cpufeatures library is at $(ANDROID_NDK)/sources/android/cpufeatures/cpu-features.c

So that leaves us a choice at configure time, to assume that the ARM optimization code is present, or that it's not.  The latest patch here (v30) assumes that it is not.  In the libpng-1.6.3 upgrade v00 patch (bug #841734), we leave it as a configure option, to use it unconditionally or not, but we can't do the run-time test.

    
  
No longer blocks: 841734
Depends on: 841734

    
    

    
  


  
Updated patch to libpng-1.6.6 which has now been checked in.
Untested; need try run (only on Android and B2G platforms for now)

        Attachment #786451 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
Looks like I missed a recent change in moz.build syntax:

    The error was triggered on line 11 of this file:    '-DMOZ_PNG_WRITE',

I believe I'm just supposed to remove the "-D", right?

    
  
Flags: needinfo?(gps)

    
    

    
  
Obviously from the sandbox example my comment #121 is wrong.  I'll try this:

DEFINES['MOZ_PNG_READ'] = True
DEFINES['MOZ_PNG_WRITE'] = True

if CONFIG['MOZ_PNG_ARM_NEON']:
    DEFINES['MOZ_PNG_HAVE_ARM_NEON'] = True

if CONFIG['MOZ_PNG_ARM_NEON_CHECK']:
    DEFINES['MOZ_PNG_ARM_NEON_CHECK'] = True

and remove the DEFINES from Makefile.in

    
    

    
  


  
Revised moz.build (DEFINES syntax)

        Attachment #811451 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
https://tbpl.mozilla.org/?tree=Try&rev=835727f91231
Flags: needinfo?(ryanvm)
Flags: needinfo?(gps)

    
    

    
  
The v32 patch gave a lot of errors like this while compiling image/decoders/png
and media/libpng/*:

nsPNGDecoder.cpp:373:43: error: 'MOZ_PNG_set_gamma' was not declared in this scope

That is consistent with the DEFINES['MOZ_PNG_READ'] not working as I expected.

In fact, MOZ_PNG_READ and MOZ_PNG_WRITE can be eliminated (see bug #922471).
Depends on: 922471

    
    

    
  
I had prematurely removed the MOZ_PNG_READ define from image/decoders/png/Makefile.in
in the v32 patch.  I'll come back to this bug after completing bug #922471.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v33 enable-arm in libpng (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #812617 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v34 enable-arm in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Fixed typo (MOZ_PNG_ARM_NEON_CHECK should be MOZ_PNG_HAVE_ARM_NEON_CHECK) in moz.build

        Attachment #814204 -
        Attachment is obsolete: true

    
    

    
  
Try for v34 is mostly green but I'm not sure it actually tested ARM nor did it actually test our media/libpng on an ARM platform.  The log for b2g_unagi (which failed to link) includes this:

11:01:23     INFO -  target asm: libpng <= external/libpng/contrib/pngneon/png_read_filter_row_neon.s
11:01:23     INFO -  target thumb C: libpng <= external/libpng/png.c
11:01:24     INFO -  target thumb C: libpng <= external/libpng/pngerror.c
11:01:24     INFO -  target thumb C: libpng <= external/libpng/pnggccrd.c

contrib/pngneon directory doesn't exist in our media/libpng, nor does pnggccrd.c.  The latter is from libpng-1.2.x.

I'm not sure that any of the other tests were running on a real armv7/neon CPU.  I don't see any evidence in the other logs that there was any attempt to assemble arm/filter_neon.S.

    
    

    
  
mwu, is the in-tree libpng used for B2G builds? ISTR another copy being a factor for the clang removal work on the JB builds?
Flags: needinfo?(mwu)

    
    

    
  
B2G gecko uses the in tree libpng. It's necessary for apng support. We have another copy of libpng, but that's only for other system binaries/libraries to use.
Flags: needinfo?(mwu)

    
    

    
  
Thanks.  It does appear that in the b2g_unagi trial, media/libpng was also compiled, including media/libpng/arm/arm_init.c and media/libpng/arm/filter_neon.S was assembled, but apparently
cpufeatures.c was not compiled.  Makefile.in will not compile cpufeatures.c unless OS_TARGET
is Android and MOZ_WEBRTC is not defined.  I don't see "OS_TARGET" or "MOZ_WEBRTC" anywhere
(but plenty of "webrtc") in the full log so that may be the problem. Also, there is a warning
about PNG_ARM_OPT being redefined in mozpngconf.h.  I'm not sure that would cause the failure
but I'll go ahead and fix that.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v35 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Removed extra #define of PNG_ARM_NEON_OPT from mozpngconf.h

        Attachment #814688 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
It does appear that the Android 2.2 builds did indeed link with cpufeatures found somewhere in the tree,
and therefore did perform the ARM test.

    
    

    
  
Result with v35 is same as v34 except that the warnings about multiple definition of PNG_ARM_NEON_OPT is gone.  Again, functions in cpufeatures were not found by the linker.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v36 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Disable use of Android cpufeatures on B2G (MOZ_WIDGET_GONK) platforms.

        Attachment #816292 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
The v36 patch is bit-rotten due to change of CSRCS to SOURCES in moz.build.
Testing v37 locally now.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v37 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Fix bit rot due to recent change to moz.build (CSRCS -> SOURCES)

        Attachment #819521 -
        Attachment is obsolete: true

    
    

    
  
More bit-rot has occurred.  V37 fails to patch media/libpng/Makefile.in.
Look for a V38 patch later on, with the stuff in V37's Makefile.in
moved to moz.build.
Flags: needinfo?(ryanvm)

    
    

    
  
Comment on attachment 822819 [details] [diff] [review]
v37 enable ARM in libpng

Marking patch v37 obsolete due to bit rot.

        Attachment #822819 -
        Attachment is obsolete: true

    
  
See Also:  → 920992

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v38-832390-enable-arm.diff (obsolete)
        — 
          — Splinter Review
      

    


  
Moved ARM support stuff from media/libpng/Makefile.in to media/libpng/moz.build
Builds OK on Ubuntu, not tested on ARM.
Flags: needinfo?(ryanvm)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v38 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Added ".align 2" to arm/filter_neon.S as suggested in bug #920992

        Attachment #824738 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v40 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Removed extra "/cpufeatures" from a line in moz.build that caused ARM builds to fail.

        Attachment #824806 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v41 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Moved ARM support from media/libpng/moz.build to media/libpng/arm/moz.build

        Attachment #824891 -
        Attachment is obsolete: true

    
    

    
  
There's a stray single-quotation mark in arm/moz.build

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v42 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Removed stray quotation mark from arm/moz.build

        Attachment #825086 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
V42 try was all greeen except for B2G-Unagi:

Linux Opt
Linux x64 Opt
OS X 10.7 Opt
   OK, no attempt to build libpng/arm/*

Android 2.2 Armv6 Opt
Android 2.2 Opt
   OK, built libpng/arm/* and cpufeatures

Android 4.2 x86 Opt
B2G ICS Emulator Opt
B2G JB Emulator Opt
   OK, no attempt to build libpng/arm/*

B2G Device Image Opt
   Failed.  Attempted to build libpng/arm/*
   but could not find cpu-features.h, which
   configure.in had already determined is present,
   while compiling libpng/arm/arm_init.c

    
    

    
  
I would think we'd want it enabled on the B2G emulator builds.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v43 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Added some debugging output to tell why libpng/arm/* was not built on the platform.

        Attachment #825333 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v44 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
The two B2G emulators use libpng-1.2.19 (released May 2007), not the bundled libpng-1.6.6.

The B2G Unagi build failed because it skips the library tests in configure.in and therefore did not set some configuration variables needed by libpng/moz.build

The v44 patch fixes the Unagi problem.

        Attachment #825613 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
Actually, "works around" the Unagi problem not "fixes" it.  The Unagi build will be without ARM support.

    
    

    
  
(In reply to Glenn Randers-Pehrson from comment #156)
> The two B2G emulators use libpng-1.2.19 (released May 2007), not the bundled
> libpng-1.6.6.

That doesn't seem to jive with comment 132, does it? Also, killing ARM support on B2G seems a tad suboptimal since it's probably more important there than Android given the hardware it's meant to run on.
Flags: needinfo?(ryanvm) → needinfo?(mwu)

    
    

    
  
The libpng used by the emulator shouldn't matter. It's used by some libs, but as mentioned in comment 132, we use our internal copy/fork of libpng.

We can hardcode NEON support if that helps. Everything we're running on and planning to run on that involves ARM will have NEON.
Flags: needinfo?(mwu)

    
    

    
  
I'll note that for libraries like libvpx, libjpeg-turbo, etc, I believe hardcoding is what we already do.

    
    

    
  
Based on comment #159, then, we don't need run-time checking (via calls to cpufeatures) for NEON support when MOZ_WIDGET_GONK is defined, right?  That would simplify matters.  We might have to deal with ARMv8 though -- libpng-1.6.7 will be out in a couple of weeks and will offer ARMv8 support via C intrinsics calls instead of assembler code.
Flags: needinfo?(mwu)

    
    

    
  
Yeah. I'm assuming you also check that you're on ARM first since we do support MOZ_WIDGET_GONK on x86, but checking MOZ_WIDGET_GONK is one way.

We should be able to detect ARMv8 at compile time, right? On such a device, we would compile everything as ARMv8.
Flags: needinfo?(mwu)

    
    

    
  
Yes, we check for ARM support before checking MOZ_WIDGET_GONK.

In fact, libpng will use the C intrinsics whenever possible, and that includes ARMv7 support, too.
We don't need to distinguish between ARMv7 and ARMv8; we simply depend on __ARM_NEON__ which
gets defined as a consequence of passing "-mfpu=neon" to the compiler.

Do you know why configure.in does SKIP_LIBRARY_CHECKS on the B2G-unagi platform?  I had to move a little bit of ARM-detection code outside of the LIBRARY_CHECKS block.
Flags: needinfo?(mwu)

    
    

    
  
No idea - I don't remember touching SKIP_LIBRARY_CHECKS. (not to say I didn't - just don't remember anything about it)
Flags: needinfo?(mwu)

    
    

    
  
SKIP_LIBRARY_CHECKS is turned off if COMPILE_ENVIRONMENT is not "1" (due to the use of the directive
--disable-compile-environment) or if _WIN32_MSVC is defined to be a non-empty string.  I don't think the latter is the case, and I hesitate to tinker with COMPILE_ENVIRONMENT without knowing the intention.  I don't see any mention of SKIP_LIBRARY_CHECKS, WIN32, or COMPILE_ENVIRONMENT in the full log for b2g-unagi.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v45 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Fixed some minor bit rot in configure.in and improved one of the error messages.  Still need a try run.

        Attachment #826562 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v46 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Try was all green but ARM support was not built on B2G platforms due to a missing AC_SUBST() directive.  The v46 patch restores that directive and fixes new bit rot due to checkin of bug #936924 (SOURCES -> UNIFIED_SOURCES).

        Attachment #829769 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
The v46 patch produced the same result as v42, namely no ARM support and no libpng-1.6.6 built on the B2G platforms.  Despite the assertions in comment #132 and comment #159, there is nothing in the full logs to indicate that there was any attempt to build libpng-1.6.6.  Only libpng-1.2.19 seems to be built.  It appears that ARM support was successfully included in the Android 2.2 builds.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v47 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
The v47 patch should build cpufeatures on Non-android ARM platforms.  Hopefully this will turn the b2g-Unagi trial green.

        Attachment #830456 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
https://tbpl.mozilla.org/?tree=Try&rev=5d2911764bca

(In reply to Glenn Randers-Pehrson from comment #170)
> The v46 patch produced the same result as v42, namely no ARM support and no
> libpng-1.6.6 built on the B2G platforms.  Despite the assertions in comment
> #132 and comment #159, there is nothing in the full logs to indicate that
> there was any attempt to build libpng-1.6.6.  Only libpng-1.2.19 seems to be
> built.  It appears that ARM support was successfully included in the Android
> 2.2 builds.

This really has me nervous.
Flags: needinfo?(ryanvm) → needinfo?(mwu)

    
    

    
  
It seem weird that building libpng/arm/arm_init.c failed on B2G-Unagi
but worked on Android 2.2.

On B2G-Unagi,
LOCAL_INCLUDES became
"-I/builds/slave/b2g_try_unagi_dep-000000000000/build/gecko/builds/slave/b2g_try_unagi_dep-000000000000/build/ndk/sources/android/cpufeatures"
while SOURCES became
"../../../../ndk/sources/android/cpufeatures/cpu-features.c

Both cpu-features.h and cpu-features.c were found by configure to
be in ...ndk/sources/android/cpufeatures/

so the SOURCES conversion is apparently ok but the LOCAL_INCLUDES didn't work.

On Android 2.2 opt, the LOCAL_INCLUDES became
-I/builds/slave/try-and-0000000000000000000000/build/builds/slave/try-and-0000000000000000000000/build/android-ndk/sources/android/cpufeatures

and SOURCES became
../../../../android-ndk/sources/android/cpufeatures/cpu-features.c

I'm not sure whether the "-I ..." worked on Android 2.2 or if cpu-features.h
was found somewhere else in the include path.

libvpx uses a Makefile.in to define LOCAL_INCLUDES, and it expands
to something much shorter on Android 2.2
-I/builds/slave/try-and-0000000000000000000000/build/android-ndk/sources/android/cpufeatures
On B2G-Unagi I could not find the expansion of the cpufeatures directory from LOCAL_INCLUDES.

I'm thinking there might be a mistake in the expansion of LOCAL_INCLUDES
in moz.build, compared to what happens when LOCAL_INCLUDES is conveyed by
Makefile.in.

I'm not sure where to go from here except maybe to revert from moz.build to Makefile.in
just to see if it works (obviously this is not the right way to go in the long run!)

    
    

    
  
(In reply to Glenn Randers-Pehrson from comment #173)
> I'm thinking there might be a mistake in the expansion of LOCAL_INCLUDES
> in moz.build, compared to what happens when LOCAL_INCLUDES is conveyed by
> Makefile.in.

moz.build LOCAL_INCLUDES only supports topsrcdir-relative paths (indicated by an absolute path in LOCAL_INCLUDES) or srcdir-relative paths (indicated by non-absolute paths in LOCAL_INCLUDES).  There's no way to get an absolute path directory outside of topsrcdir into LOCAL_INCLUDES from moz.build files.  And if I understand your patch, that's what's happening.

The LOCAL_INCLUDES for some of the cairo bits used absolute paths, but I don't recall whether those were converted to moz.build or not.

    
    

    
  
OK, so we have to migrate back to Makefile.in.  #:-(

Before doing that I'd like to see some actual evidence that this is worth while.  I have only seen tests reported for four large PNG files, claiming a 15 percent improvement (the PNG files weren't provided so I have no way to verify that.) Just from looking at code, I think there will be *NO* benefit when decoding typical "filter-type none" PNG files  I'd expect a good result with large filtered PNGs, which should probably be JPEGs anyhow.  Since Android 2.2 seems to be working (in spite of the moz.build versus Makefile.in problem with LOCAL_INCLUDES), someone who has such a development platform could compare with and without the patch, for a reasonable set of pages containing PNG images.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v48 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Configure with a topdir-relative path to cpu-features.c, .h in LOCAL_INCLUDES

        Attachment #830625 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v49 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Fixed misplaced "/" in v48 patch that caused ARM support to be omitted.

        Attachment #831006 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
FWIW, I don't think it's possible to successfully build without building our own libpng on B2G device builds. See http://hg.mozilla.org/mozilla-central/file/7b014f0f3b03/b2g/app/Makefile.in#l16 which explicitly links the main binary against libpng.
Flags: needinfo?(mwu)

    
    

    
  
OK, I've been poking around with things and think I understand what's going on. Thanks to bug 936924 (building libpng in unified mode), the in-tree compilation of libpng isn't as obvious. It shows up under "Unified_c_media_libpng0.c" in the build. The intentionally-broken Try push below shows that indeed we do build the in-tree libpng on all platforms:
https://tbpl.mozilla.org/?tree=Try&rev=dd5192e01c78

So looking at the Try push from comment 179, Unified_c_media_libpng is indeed present in all logs. What I don't know is what this means for filter_neon.S. I don't see that in any logs, including Android.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v50 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Use $_topsrcdir instead of $DEPTH in tests in configure.in
Move arm_init.c from SOURCES to UNIFIED_SOURCES
Add debugging to find out why embedded libpng is not used in B2G emulators

        Attachment #831255 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
Re comment # 181: right, I had missed this line (was searching for the wrong string in the log)
18:27:37     INFO -  Unified_c_media_libpng0.o

I don't think filter_neon.S can be included in the UNIFIED_SOURCES so the v50 patch leaves it
alone in SOURCES.  It will only show up in the logs where it was selected for assembly by configure.in,
as seen in the v47 result for "Android 2.2 ARMv6":
https://tbpl.mozilla.org/php/getParsedLog.php?id=30467741&tree=Try&full=1
It doesn't show up in v48 or v49 because of my errors in finding the cpufeatures directory, hopefully fixed in v50.

    
    

    
  
The v50 patch isn't quite right:
line 15748: _topsrcdir: command not found

Testing a v51 patch to fix this now.

    
    

    
  
(In reply to Glenn Randers-Pehrson from comment #184)
> Re comment # 181: right, I had missed this line (was searching for the wrong
> string in the log)
> 18:27:37     INFO -  Unified_c_media_libpng0.o
> 
> I don't think filter_neon.S can be included in the UNIFIED_SOURCES so the
> v50 patch leaves it
> alone in SOURCES. 

UNIFIED_SOURCES barfs if anything is left. But it will create a separate Unified file for assembly. (Or at least, it should, maybe that's the problem). That, however might not work if you need to pass extra arguments to that file.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v51 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Use ${_topsrcdir} instead of $_topsrcdir in libpng ARM tests in configure.in

        Attachment #831659 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
(In reply to Mike Hommey [:glandium] from comment #186)

> UNIFIED_SOURCES barfs if anything is left. But it will create a separate
> Unified file for assembly. (Or at least, it should, maybe that's the
> problem). That, however might not work if you need to pass extra arguments
> to that file.

The v47 patch did not barf; however that patch compiled arm_init.c and filter_neon.S in a
subdirectory with its own moz.build.  If v51 fails I'll put it back there.

There's nothing in
hg.mozilla.org/mozilla-central/file/default/python/mozbuild/mozbuild/frontend/sandbox_symbols.py that says that SOURCES and UNIFIED_SOURCES cannot coexist, so if v51 fails for this reason then I'd recommend that a comment be added there (or to say that they *can* coexist if that's true).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v52 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
The v51 patch failed on Android and B2G ARM platforms with "Reference to a file that doesn't exist in UNIFIED_SOURCES (MOZ_PNG_ARM_CPUFEATURES_LIB/cpu-features.c)"; apparently the expansion of the MOZ_PNG... environmental variable did not happen, probably due to a missing "$" in configure.in within a block of code that I cannot test locally.

        Attachment #831777 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
V52 patch resulted in all green for non-ARM platforms and all red for ARM platforms, due to not being able to find cpu-features.c or maybe cpu-features.h, both of which had been found by configure.in.

Will have to try another approach.  I'm reluctant to go back to Makefile.in, VPATH, etc.

    
  

        Attachment #832459 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v53 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
The v53 patch takes a different approach, making a local copy of cpu-features.c,
which shouldn't increase the size of libxul.so at all because we were already making a local copy through the UNIFIED_SOURCES mechanism anyhow.
Flags: needinfo?(ryanvm)

    
    

    
  
unable to find 'media/libpng/arm/MOZ_PNG_cpu-features.c' for patching
patching file media/libpng/arm/MOZ_PNG_cpu-features.c
1 out of 1 hunks FAILED -- saving rejects to file media/libpng/arm/MOZ_PNG_cpu-features.c.rej
unable to find 'media/libpng/arm/MOZ_PNG_cpu-features.h' for patching
patching file media/libpng/arm/MOZ_PNG_cpu-features.h
1 out of 1 hunks FAILED -- saving rejects to file media/libpng/arm/MOZ_PNG_cpu-features.h.rej
Flags: needinfo?(ryanvm)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v54 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Added "new file" directives that were missing from the v53 patch.

        Attachment #8334997 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
https://tbpl.mozilla.org/?tree=Try&rev=14763d9fe7ab

I'm still trying to understand why we need to jump through all these hoops for libpng but not for the other libraries with Neon support (like libjpeg-turbo, libvpx, etc).
Flags: needinfo?(ryanvm)

    
    

    
  
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #196)

> I'm still trying to understand why we need to jump through all these hoops

libvpx is still using Makefile.in with VPATH; I was asked not to do that
in comment #57.

libjpeg-turbo seems to not check cpu-features but just assume that if
the operating system is *arm*, neon features are present (libpng can be
made to behave that way too with --enable-png-arm-neon-support=nocheck),
but that seems risky.

    
    

    
  
I thought we decided in comment 161 that we didn't need runtime detection? Or do we need it for armv8 support?

    
    

    
  
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #198)
> I thought we decided in comment 161 that we didn't need runtime detection?
> Or do we need it for armv8 support?

That takes care of B2G (GONK) but not Android.  And anyhow, as you say, we'll
have to address ARMv8 as well and I'll work on that after libpng-1.6.7 lands.

    
    

    
  
How are we not breaking Android users with unconditional support in the other libraries then?

    
    

    
  
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #200)
> How are we not breaking Android users with unconditional support in the
> other libraries then?

libjpeg-turbo has a built-in check for cpufeatures, in simd/jsimd_arm.c
(I was mistaken in comment #197; I had missed the built-in check).
That's more-or-less equivalent to what we're trying to do here.

libvpx is using the system cpu-features, which it finds via Makefile.in

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v55 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
Include the local "MOZ_PNG_cpu-features.h" instead of <cpu-features.h> in arm_init.c.  The v54 patch which includes <cpu-features.h> worked on Android platforms and on the B2G emulators, but not on B2G Unagi.

        Attachment #8335335 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v56 enable ARM in libpng (obsolete)
        — 
          — Splinter Review
      

    


  
V55 patch was missing the update to arm_init.c

        Attachment #8335430 -
        Attachment is obsolete: true

    
  
Depends on: 938740

    
    

    
  
Try v56 is all green except for one B2G emulator that failed for an unrelated reason.
Not asking for review at this time because I expect libpng-1.6.7 to land shortly and
there will therefore be some bit rot and new ARMv8 support.

    
    

    
  


  
Patch revised to work with libpng-1.6.7, and to use the intrinsic C code on ARM platforms instead of assembler code whenever possible.  Now supports ARMv8.

        Attachment #8335602 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
Looks like you forgot to hg add the new files.

patching file media/libpng/arm/MOZ_PNG_cpu-features.c
1 out of 1 hunks FAILED -- saving rejects to file media/libpng/arm/MOZ_PNG_cpu-features.c.rej
unable to find 'media/libpng/arm/MOZ_PNG_cpu-features.h' for patching
patching file media/libpng/arm/MOZ_PNG_cpu-features.h
1 out of 1 hunks FAILED -- saving rejects to file media/libpng/arm/MOZ_PNG_cpu-features.h.rej
Flags: needinfo?(ryanvm)

    
    

    
  


  
Added two "new file" directives to the patch.

        Attachment #8343170 -
        Attachment is obsolete: true
Flags: needinfo?(ryanvm)

    
    

    
  
Try of the v58 patch shows green on all platforms.  Logs indicate that the optimized ARM code was used when appropriate.

    
  

        Attachment #8343746 -
        Flags: review?(jmuizelaar)

    
  
Hardware: All → ARM

    
    

    
  
Comment on attachment 8343746 [details] [diff] [review]
v58 enable ARM in libpng-1.6.7 and later

This is mostly a build patch, so I'd rather build people review it. I'm fine with the concept in principle.

        Attachment #8343746 -
        Flags: review?(jmuizelaar) → review?(mh+mozilla)

    
    

    
  
Comment on attachment 8343746 [details] [diff] [review]
v58 enable ARM in libpng-1.6.7 and later

Review of attachment 8343746 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +3891,5 @@
> +else
> +    MOZ_PNG_ARM_DEBUG='CPU_ARCH_IS_NOT_ARM'
> +fi
> +
> +AC_SUBST(MOZ_PNG_ARM_NEON)

There's already an AC_SUBST for MOZ_PNG_ARM_NEON below.

@@ +3898,3 @@
>  fi # SKIP_LIBRARY_CHECKS
>  
> +AC_MSG_RESULT([MOZ_PNG_ARM_DEBUG=$MOZ_PNG_ARM_DEBUG])

MOZ_PNG_ARM_DEBUG doesn't seem very useful.

::: media/libpng/arm/MOZ_PNG_cpu-features.c
@@ +10,5 @@
> +
> +/*  This is a copy of webrtc/system_wrappers/source/android/cpu-features.c
> + *  with name changed to MOZ_PNG_cpu-features.c, exported symbols prefixed
> + *  with "MOZ_PNG_", and cpufeatures.h changed to a local file with MOZ_PNG
> + *  prefix.

If this is mozilla specific (as opposed to something you land in upstream png), why need a copy at all?

::: media/libpng/moz.build
@@ +40,5 @@
>  
> +    if CONFIG['MOZ_PNG_ARM_NEON_CHECK']:
> +        DEFINES['MOZ_PNG_HAVE_ARM_NEON_CHECK'] = True
> +        UNIFIED_SOURCES += [
> +            'arm/MOZ_PNG_cpu-features.c'

Note this file is android specific, but nothing makes it android specific in moz.build. This means this likely breaks linux arm builds.

        Attachment #8343746 -
        Flags: review?(mh+mozilla) → feedback-

    
    

    
  


  
Removed redundant AC_SUBST().

Removed MOZ_PNG_ARM_DEBUG debugging info.

Added #ifdef __ANDROID__/#endif to remove the Android cpu-features.c code from the unified source (it's not called on non-Android platforms).

Copying the webrtc code into the libpng directory was necessary because the webrtc directory couldn't be reliably found on all platforms.  In the long run we should use a different approach but that's for another bug. Note that libjpeg, libyuv, libtheora, vpxports, SkUtils, and pixman all have their own slightly different copies (search MXR for "proc/cpuinfo").  I think all of them should be using a shared cpu-detection function (probably the one in mozglue/build/arm.cpp, "supports_arm").  I'm willing to try using that in libpng as a test case, but it'll likely delay landing libpng ARM support for a while.

        Attachment #8343746 -
        Attachment is obsolete: true

        Attachment #8346071 -
        Flags: feedback?(mh+mozilla)

    
    

    
  
I said, Added #ifdef __ANDROID__/#endif to remove the Android cpu-features.c code from the unified source

Just to clarify, of course it doesn't actually remove the lines from the unified source.  It merely prevents the lines from being compiled.

        Attachment #8346071 -
        Flags: feedback?(mh+mozilla) → feedback+

    
    

    
  
Try run for v59 patch needed.
Flags: needinfo?(ryanvm)

    
    

    
  
Comment on attachment 8346071 [details] [diff] [review]
v59-832390-enable-arm.diff

r?  Try is all green except for two Mac tests that are pending.  The full logs show that the ARM code in libpng is being built, but I don't know how to tell if it's actually being used (there seem to be at least three different instances of libpng in the B2G Unagi build).

        Attachment #8346071 -
        Flags: review?(mh+mozilla)

    
    

    
  
(In reply to Glenn Randers-Pehrson from comment #218)
> I don't know how
> to tell if it's actually being used (there seem to be at least three
> different instances of libpng in the B2G Unagi build).

Actually I do know how but don't have a platform to test.  Just try to display
the PNG that is attached to bug #945912.  If it crashes in MOZ_PNG_do_expand_plte
at media/libpng/pngrtran.c:4675, then media/libpng is being used.  If it does not
crash, then one of the older libpng instances is probably being used.

    
    

    
  
(In reply to Glenn Randers-Pehrson from comment #197)
> libjpeg-turbo seems to not check cpu-features but just assume that if
> the operating system is *arm*, neon features are present (libpng can be
> made to behave that way too with --enable-png-arm-neon-support=nocheck),
> but that seems risky.

The upstream libjpeg-turbo has had arm neon runtime detection since ages ago and it just works (causes no known problems in linux/android/ios and needs no pointless refactoring). Mozilla tends to patch and unify the runtime detection code in libraries, but presumably there are no problems with libjpeg-turbo in Mozilla either.

        Attachment #8346071 -
        Flags: review?(mh+mozilla) → review+

    
  
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/ba55f07dbdc7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

    
  
Blocks: 952505

    
  
Summary: Investigate possibility to enable arm neon for libpng → Enable arm neon for libpng

    
  
Blocks: 1296946

    
  
No longer blocks: 1296946

          You need to log in
          before you can comment on or make changes to this bug.