Closed Bug 1364237 Opened 7 years ago Closed 7 years ago

libaom build followup

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: rillian, Assigned: rillian)

References

Details

Attachments

(2 files)

Bug 1364237 - aom: Remove obsolete 32-bit macOS build config.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1364237 - aom: Enable YASM for intel cpus.
59 bytes, text/x-review-board-request
froydnj
: review+
Details 
A couple of build issues with the av1 decoder library.
Assignee: nobody → giles
Comment on attachment 8866997 [details]
Bug 1364237 - aom: Remove obsolete 32-bit macOS build config.

https://reviewboard.mozilla.org/r/138590/#review142052
Attachment #8866997 - Flags: review?(nfroyd) → review+
Comment on attachment 8866998 [details]
Bug 1364237 - aom: Enable YASM for intel cpus.

https://reviewboard.mozilla.org/r/138592/#review142054

::: commit-message-aa035:6
(Diff revision 1)
> +the build won't compile .asm files, but instead try to link
> +them directly.

I think this could be re-written to be clearer, because we're not trying to link the assembly files directly into libxul unassembled!

::: commit-message-aa035:9
(Diff revision 1)
> +Restore them unconditionally for intel targets. We require
> +yasm to build for those targets, and want to fail if it
> +isn't available to block performance regressions.

I'm surprised it's not required for ARM as well, but maybe that's just the way people write ARM assembly.

::: media/libaom/moz.build:16
(Diff revision 1)
>  
>  # Linux, Mac and Win share file lists for x86* but not configurations.
>  if CONFIG['CPU_ARCH'] == 'x86_64':
>      EXPORTS.aom += files['X64_EXPORTS']
>      SOURCES += files['X64_SOURCES']
> +    USE_YASM = True

This is essentially going to make yasm required for these builds, correct?  I see that bootstrap installs it, and I see that various things depend on yasm sources, but I'm not clear on the required status of the tool.  Do we require it already (for VPX?)?
Attachment #8866998 - Flags: review?(nfroyd) → review+
Comment on attachment 8866998 [details]
Bug 1364237 - aom: Enable YASM for intel cpus.

https://reviewboard.mozilla.org/r/138592/#review142060

::: commit-message-aa035:6
(Diff revision 1)
> +the build won't compile .asm files, but instead try to link
> +them directly.

Ok. Perhaps I misunderstood. I see that `USE_YASM` just sets `AS=$YASM` in emitter.py, but the actual error message is
```
gcc: warning: /home/giles/firefox/third_party/aom/av1/encoder/x86/dct_sse2.asm: linker input file unused because linking not done
[...]
Exception: File not found: [foo]_sse2.o
```
Which doesn't look like the gnu assembler is failing to parse the yasm source, it looks like we didn't generate a build rule at all, which confused me.

::: commit-message-aa035:9
(Diff revision 1)
> +Restore them unconditionally for intel targets. We require
> +yasm to build for those targets, and want to fail if it
> +isn't available to block performance regressions.

I believe the arm assembly uses the gnu assembler instead of yasm. The actual source is in an older arm-toolchain assembly format, but there's a perl script we call to covert it at build time.

It looks like that conversion is actually optional. I've no idea what we do if the gnu assembler isn't available. Perhaps assume the old-style arm toolchain?

::: media/libaom/moz.build:16
(Diff revision 1)
>  
>  # Linux, Mac and Win share file lists for x86* but not configurations.
>  if CONFIG['CPU_ARCH'] == 'x86_64':
>      EXPORTS.aom += files['X64_EXPORTS']
>      SOURCES += files['X64_SOURCES']
> +    USE_YASM = True

Correct. We require yasm for VPX (except for --with-system-libvpx of course) but we print a nicer message about it:

https://dxr.mozilla.org/mozilla-central/source/old-configure.in#2992

I didn't want to duplicate all the machinery in old-configure.in, and the webm check will cover most cases in practice.
Pushed by rgiles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a5c7db293d58
aom: Remove obsolete 32-bit macOS build config. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/b629c2e4753e
aom: Enable YASM for intel cpus. r=froydnj
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: