Enable arm neon for libpng

RESOLVED FIXED in mozilla29

Status

()

Core
ImageLib
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: romaxa, Assigned: Glenn Randers-Pehrson)

Tracking

(Blocks: 1 bug)

unspecified
mozilla29
ARM
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 58 obsolete attachments)

22.02 KB, patch
glandium
: review+
glandium
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
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.
(Reporter)

Comment 2

5 years ago
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...
(Assignee)

Comment 3

5 years ago
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
(Assignee)

Updated

5 years ago
Blocks: 832487
(Assignee)

Updated

5 years ago
No longer blocks: 832487
Depends on: 832487
(Assignee)

Comment 4

5 years ago
I have uploaded a patch to bug #832487 (update libpng to 1.5.14) which includes the ARM stuff.
(Reporter)

Comment 5

5 years ago
Created attachment 706841 [details] [diff] [review]
Enable arm neon png implementation
Assignee: glennrp+bmo → romaxa
Attachment #706841 - Flags: review?(glennrp+bmo)
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 462796
(Assignee)

Comment 7

5 years ago
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?
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
My mistake.  Don't change arm/arm_init.c; only add the two lines
to arm/filter_neon.S
(Assignee)

Comment 10

5 years ago
Created attachment 707092 [details] [diff] [review]
v01 enable ARM without modifying 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)
(Assignee)

Updated

5 years ago
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.

Comment 14

5 years ago
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).

Comment 16

5 years ago
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.

Comment 19

5 years ago
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)

Comment 21

5 years ago
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.

Comment 23

5 years ago
Created attachment 714424 [details] [diff] [review]
Patch to use method supported by Android, make read safe onm EINTR

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+
(Assignee)

Comment 26

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #714424 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 858478
(Assignee)

Updated

5 years ago
Depends on: 858578
No longer depends on: 858478
(Assignee)

Comment 27

5 years ago
Created attachment 737926 [details] [diff] [review]
v02 enable ARM optimization with libpng-1.5.15 and later

Revised patch to work with libpng-1.5.15 and later.  Untested.
Attachment #707092 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 873001
This may need revisions due to the ongoing moz.build conversion work.
(Assignee)

Comment 29

5 years ago
Right, and it's also possible that there is some bit rot due to checkin of libpng-1.5.16 tonight.
(Assignee)

Comment 30

5 years ago
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.
(Assignee)

Comment 31

5 years ago
Created attachment 759565 [details] [diff] [review]
v03 enable ARM optimization with libpng-1.5.16 and later

Revised patch to work with libpng-1.5.16 and moz.build
Attachment #737926 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
OS: Windows 8 → All
Hardware: x86 → All
(Assignee)

Comment 32

5 years ago
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
(Assignee)

Comment 36

5 years ago
Created attachment 759725 [details] [diff] [review]
v04 enable ARM optimization with libpng-1.5.16 and later

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...)
(Assignee)

Comment 41

5 years ago
Created attachment 759730 [details] [diff] [review]
v05 enable ARM optimization with libpng-1.5.16 and later
Attachment #759725 - Attachment is obsolete: true
(Assignee)

Comment 42

5 years ago
Created attachment 759733 [details] [diff] [review]
v06 enable ARM optimization with libpng-1.5.16 and later

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?
(Assignee)

Comment 44

5 years ago
Created attachment 759738 [details] [diff] [review]
v07 enable ARM optimization with libpng-1.5.16 and later

Account for MOZ_PNG prefix in function name, in mozpngconf.h
Attachment #759733 - Attachment is obsolete: true
(Assignee)

Comment 45

5 years ago
(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

Comment 46

5 years ago
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.
(Assignee)

Comment 47

5 years ago
Created attachment 759786 [details] [diff] [review]
v08 enable ARM optimization with libpng-1.5.16 and later

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'
(Assignee)

Comment 50

5 years ago
I don't know what that means.  Is it looking for filter_neon.asm perhaps?
Flags: needinfo?
(Assignee)

Comment 51

5 years ago
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)
(Assignee)

Comment 53

5 years ago
Created attachment 759847 [details] [diff] [review]
v10 enable ARM optimization with libpng-1.5.16 and later

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)
(Assignee)

Comment 55

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 886499
(Assignee)

Comment 58

5 years ago
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.
(Assignee)

Comment 59

5 years ago
Created attachment 778224 [details] [diff] [review]
v12 enable ARM optimization with libpng-1.5.17 and later

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
(Assignee)

Comment 60

5 years ago
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?
(Assignee)

Comment 62

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

Comment 63

5 years ago
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
(Assignee)

Comment 64

5 years ago
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.
(Assignee)

Comment 66

5 years ago
Created attachment 778475 [details] [diff] [review]
v13 enable ARM optimization with libpng-1.5.17 and later

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)
(Assignee)

Comment 67

5 years ago
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?
(Assignee)

Comment 69

5 years ago
(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.

Comment 70

5 years ago
(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)
(Assignee)

Comment 71

5 years ago
Created attachment 778491 [details] [diff] [review]
v14 enable ARM optimization with libpng-1.5.17 and later

Revised patch per comment #67 and comment #69
Attachment #778475 - Attachment is obsolete: true
(Assignee)

Comment 72

5 years ago
(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

Comment 73

5 years ago
(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

Comment 74

5 years ago
> 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.)
(Assignee)

Comment 75

5 years ago
Created attachment 778525 [details] [diff] [review]
v15 enable ARM optimization with libpng-1.5.17 and later

#  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)
(Assignee)

Updated

5 years ago
Flags: needinfo?(joe)
(Assignee)

Comment 77

5 years ago
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.
(Assignee)

Comment 78

5 years ago
Created attachment 778683 [details] [diff] [review]
v16 enable ARM optimization with libpng-1.5.17 and later

Alphabetized the DEFINES.
Attachment #778525 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
(Assignee)

Comment 80

5 years ago
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.
(Assignee)

Comment 81

5 years ago
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.
(Assignee)

Comment 82

5 years ago
Created attachment 778736 [details] [diff] [review]
v17 enable ARM optimization with libpng-1.5.17 and later

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)
(Assignee)

Comment 84

5 years ago
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.
(Assignee)

Comment 85

5 years ago
Created attachment 778769 [details] [diff] [review]
v18 enable ARM optimization with libpng-1.5.17 and later

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)
(Assignee)

Comment 87

5 years ago
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]
(Assignee)

Comment 88

5 years ago
Created attachment 778783 [details] [diff] [review]
v19 enable ARM optimization with libpng-1.5.17 and later

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)

Comment 89

5 years ago
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
(Assignee)

Comment 90

5 years ago
Created attachment 778869 [details] [diff] [review]
v20 enable ARM optimization with libpng-1.5.17 and later

Added link to Android cpu-features
Attachment #778783 - Attachment is obsolete: true
(Assignee)

Comment 91

5 years ago
ping: still needing a try run of the v20 patch
(Assignee)

Updated

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

Comment 93

5 years ago
It got a little further but still busted on the Android platforms.
(Assignee)

Comment 94

5 years ago
Created attachment 781270 [details] [diff] [review]
v20 Enable ARM optimization with libpng-1.5.17 and later

Removed renaming macros for NEON functions from mozlibpng.h
Attachment #778869 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
(Assignee)

Comment 95

5 years ago
Created attachment 782164 [details] [diff] [review]
v22 Enable ARM optimization with libpng-1.5.17 and later

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

Updated

5 years ago
Attachment #781270 - Attachment is obsolete: true
(Assignee)

Comment 97

5 years ago
Created attachment 783422 [details] [diff] [review]
v23 Enable ARM optimization with libpng-1.5.17 and later

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)
(Assignee)

Comment 98

5 years ago
Created attachment 783874 [details] [diff] [review]
v24 Enable ARM optimization with libpng-1.5.17 and later

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

Comment 99

5 years ago
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
(Assignee)

Comment 101

5 years ago
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)
(Assignee)

Updated

5 years ago
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).
(Assignee)

Comment 104

5 years ago
Created attachment 785202 [details] [diff] [review]
v25 Enable ARM optimization with libpng-1.5.17 and later

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

Comment 105

5 years ago
Created attachment 785403 [details] [diff] [review]
v26 Enable ARM optimization with libpng-1.5.17 and later

Restored NEON function renaming in mozpngconf.h; that was not the cause of problems.
Attachment #785202 - Attachment is obsolete: true
(Assignee)

Comment 106

5 years ago
Created attachment 785440 [details] [diff] [review]
v27 Enable ARM optimization with libpng-1.5.17 and later

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
(Assignee)

Comment 107

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

Updated

5 years ago
No longer depends on: 832487, 858578, 873001
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 109

5 years ago
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.
(Assignee)

Comment 110

5 years ago
Created attachment 785914 [details] [diff] [review]
v28 Enable ARM optimization with libpng-1.5.17 and later

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
(Assignee)

Comment 112

5 years ago
Created attachment 786348 [details] [diff] [review]
v29 Enable ARM optimization with libpng-1.5.17 and later

Fixed typo introduced in the v28 patch, in configure.in
Attachment #785914 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
(Assignee)

Comment 114

5 years ago
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?
(Assignee)

Comment 115

5 years ago
Created attachment 786451 [details] [diff] [review]
v30 Enable ARM optimization with libpng-1.5.17 and later

Disable ARM detection and use of ARM optimization on B2G for now.
Attachment #786348 - Attachment is obsolete: true
Flags: needinfo?
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 118

5 years ago
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.
(Assignee)

Updated

5 years ago
No longer blocks: 841734
Depends on: 841734
(Assignee)

Comment 119

5 years ago
Created attachment 811451 [details] [diff] [review]
v31 enable ARM_NEON optimization in libpng-1.6.6

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)
(Assignee)

Comment 121

5 years ago
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?
(Assignee)

Updated

5 years ago
Flags: needinfo?(gps)
(Assignee)

Comment 122

5 years ago
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
(Assignee)

Comment 123

5 years ago
Created attachment 812617 [details] [diff] [review]
v32 enable ARM_NEON optimization in libpng-1.6.6

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)
(Assignee)

Comment 125

5 years ago
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
(Assignee)

Comment 126

5 years ago
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.
(Assignee)

Comment 127

5 years ago
Created attachment 814204 [details] [diff] [review]
v33 enable-arm in libpng
Attachment #812617 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
(Assignee)

Comment 128

5 years ago
Created attachment 814688 [details] [diff] [review]
v34 enable-arm in libpng

Fixed typo (MOZ_PNG_ARM_NEON_CHECK should be MOZ_PNG_HAVE_ARM_NEON_CHECK) in moz.build
Attachment #814204 - Attachment is obsolete: true
(Assignee)

Comment 130

5 years ago
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)

Comment 132

5 years ago
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)
(Assignee)

Comment 133

5 years ago
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.
(Assignee)

Comment 134

5 years ago
Created attachment 816292 [details] [diff] [review]
v35 enable ARM in libpng

Removed extra #define of PNG_ARM_NEON_OPT from mozpngconf.h
Attachment #814688 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
(Assignee)

Comment 135

5 years ago
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.
(Assignee)

Comment 137

5 years ago
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.
(Assignee)

Comment 138

5 years ago
Created attachment 819521 [details] [diff] [review]
v36 enable ARM in libpng

Disable use of Android cpufeatures on B2G (MOZ_WIDGET_GONK) platforms.
Attachment #816292 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
(Assignee)

Comment 139

5 years ago
The v36 patch is bit-rotten due to change of CSRCS to SOURCES in moz.build.
Testing v37 locally now.
(Assignee)

Comment 140

5 years ago
Created attachment 822819 [details] [diff] [review]
v37 enable ARM in libpng

Fix bit rot due to recent change to moz.build (CSRCS -> SOURCES)
Attachment #819521 - Attachment is obsolete: true
(Assignee)

Comment 141

5 years ago
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)
(Assignee)

Comment 142

5 years ago
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
(Assignee)

Updated

5 years ago
See Also: → bug 920992
(Assignee)

Comment 143

5 years ago
Created attachment 824738 [details] [diff] [review]
v38-832390-enable-arm.diff

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)
(Assignee)

Comment 144

5 years ago
Created attachment 824806 [details] [diff] [review]
v38 enable ARM in libpng

Added ".align 2" to arm/filter_neon.S as suggested in bug #920992
Attachment #824738 - Attachment is obsolete: true
(Assignee)

Comment 146

5 years ago
Created attachment 824891 [details] [diff] [review]
v40 enable ARM in libpng

Removed extra "/cpufeatures" from a line in moz.build that caused ARM builds to fail.
Attachment #824806 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
(Assignee)

Comment 147

5 years ago
Created attachment 825086 [details] [diff] [review]
v41 enable ARM in libpng

Moved ARM support from media/libpng/moz.build to media/libpng/arm/moz.build
Attachment #824891 - Attachment is obsolete: true
(Assignee)

Comment 149

5 years ago
There's a stray single-quotation mark in arm/moz.build
(Assignee)

Comment 150

5 years ago
Created attachment 825333 [details] [diff] [review]
v42 enable ARM in libpng

Removed stray quotation mark from arm/moz.build
Attachment #825086 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
(Assignee)

Comment 152

5 years ago
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.
(Assignee)

Comment 154

5 years ago
Created attachment 825613 [details] [diff] [review]
v43 enable ARM in libpng

Added some debugging output to tell why libpng/arm/* was not built on the platform.
Attachment #825333 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
(Assignee)

Comment 156

5 years ago
Created attachment 826562 [details] [diff] [review]
v44 enable ARM in libpng

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)
(Assignee)

Comment 157

5 years ago
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)

Comment 159

5 years ago
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.
(Assignee)

Comment 161

5 years ago
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)

Comment 162

5 years ago
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)
(Assignee)

Comment 163

5 years ago
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)

Comment 164

5 years ago
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)
(Assignee)

Comment 165

5 years ago
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.
(Assignee)

Comment 166

5 years ago
Created attachment 829769 [details] [diff] [review]
v45 enable ARM in libpng

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)
(Assignee)

Comment 168

5 years ago
Created attachment 830456 [details] [diff] [review]
v46 enable ARM in libpng

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)
(Assignee)

Comment 170

5 years ago
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.
(Assignee)

Comment 171

5 years ago
Created attachment 830625 [details] [diff] [review]
v47 enable ARM in libpng

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)
(Assignee)

Comment 173

5 years ago
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.
(Assignee)

Comment 175

5 years ago
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.
(Assignee)

Comment 176

5 years ago
Created attachment 831006 [details] [diff] [review]
v48 enable ARM in libpng

Configure with a topdir-relative path to cpu-features.c, .h in LOCAL_INCLUDES
Attachment #830625 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
(Assignee)

Comment 178

5 years ago
Created attachment 831255 [details] [diff] [review]
v49 enable ARM in libpng

Fixed misplaced "/" in v48 patch that caused ARM support to be omitted.
Attachment #831006 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)

Comment 180

5 years ago
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.
(Assignee)

Comment 182

5 years ago
Created attachment 831659 [details] [diff] [review]
v50 enable ARM in libpng

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)
(Assignee)

Comment 184

5 years ago
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.
(Assignee)

Comment 185

5 years ago
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.
(Assignee)

Comment 187

5 years ago
Created attachment 831777 [details] [diff] [review]
v51 enable ARM in libpng

Use ${_topsrcdir} instead of $_topsrcdir in libpng ARM tests in configure.in
Attachment #831659 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
(Assignee)

Comment 188

5 years ago
(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).
(Assignee)

Comment 190

5 years ago
Created attachment 832459 [details] [diff] [review]
v52 enable ARM in libpng

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)
(Assignee)

Comment 192

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #832459 - Attachment is obsolete: true
(Assignee)

Comment 193

5 years ago
Created attachment 8334997 [details] [diff] [review]
v53 enable ARM in libpng

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)
(Assignee)

Comment 195

5 years ago
Created attachment 8335335 [details] [diff] [review]
v54 enable ARM in libpng

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)
(Assignee)

Comment 197

5 years ago
(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?
(Assignee)

Comment 199

5 years ago
(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?
(Assignee)

Comment 201

5 years ago
(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
(Assignee)

Comment 202

5 years ago
Created attachment 8335430 [details] [diff] [review]
v55 enable ARM in libpng

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)
(Assignee)

Comment 203

5 years ago
Created attachment 8335602 [details] [diff] [review]
v56 enable ARM in libpng

V55 patch was missing the update to arm_init.c
Attachment #8335430 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 938740
(Assignee)

Comment 205

5 years ago
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.
(Assignee)

Comment 207

4 years ago
Created attachment 8343170 [details] [diff] [review]
v57 enable ARM in libpng-1.6.7 and later

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)
(Assignee)

Comment 209

4 years ago
Created attachment 8343746 [details] [diff] [review]
v58 enable ARM in libpng-1.6.7 and later

Added two "new file" directives to the patch.
Attachment #8343170 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
(Assignee)

Comment 211

4 years ago
Try of the v58 patch shows green on all platforms.  Logs indicate that the optimized ARM code was used when appropriate.
(Assignee)

Updated

4 years ago
Attachment #8343746 - Flags: review?(jmuizelaar)
(Assignee)

Updated

4 years ago
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-
(Assignee)

Comment 214

4 years ago
Created attachment 8346071 [details] [diff] [review]
v59-832390-enable-arm.diff

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)
(Assignee)

Comment 215

4 years ago
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+
(Assignee)

Comment 216

4 years ago
Try run for v59 patch needed.
Flags: needinfo?(ryanvm)
(Assignee)

Comment 218

4 years ago
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)
(Assignee)

Comment 219

4 years ago
(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.

Comment 220

4 years ago
(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+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba55f07dbdc7
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(Assignee)

Updated

4 years ago
Blocks: 952505

Updated

4 years ago
Summary: Investigate possibility to enable arm neon for libpng → Enable arm neon for libpng
(Assignee)

Updated

2 years ago
Blocks: 1296946
(Assignee)

Updated

2 years ago
No longer blocks: 1296946
You need to log in before you can comment on or make changes to this bug.