1369932 - libaom doesn't build for windows

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Ralph Giles (:rillian)]

Various versions of libaom fail to build under mvsc, usually issues with the simd intrinsics. This is a tracking bug for resolving these issues.

Comment 1

[Ralph Giles (:rillian)]

Updating to a newer snapshot in bug 1369950 resolved the build issue for win64. Win32 still fails with alignment constraint violations. https://bugs.chromium.org/p/aomedia/issues/detail?id=587

Comment 2

[Ralph Giles (:rillian)]

Attachment: Bug 1369932 - Backport win32 build fix for aom.

Backport patch by David Barker converting arguments to pointers.
The win32 ABI spills to the stack after the first three arguments,
but doesn't guarantee correct alignment for simd data there.

This is fixed upstream, but I don't want to bump our commit
just yet since it forces a re-encode of active demo streams.

Patch from https://bugs.chromium.org/p/aomedia/issues/detail?id=587

Review commit: https://reviewboard.mozilla.org/r/151966/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151966/

Comment 3

[Ralph Giles (:rillian)]

Attachment: Bug 1369932 - Enable av1 playback in nightly Windows builds.

Enable for Windows targets now that they're building.

Review commit: https://reviewboard.mozilla.org/r/151968/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151968/

Comment 4

[Ralph Giles (:rillian)]

Attachment: Bug 1369932 - Fix typo in aom build description.

The configuration object is spelled with capitals. We didn't
try to build this code on Windows previously, so short-circuit
evaluation saved us from hitting the mistake.

Review commit: https://reviewboard.mozilla.org/r/151978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151978/

Comment 5

[Ralph Giles (:rillian)]

Comment on attachment 8880640 [details]
Bug 1369932 - Enable av1 playback in nightly Windows builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151968/diff/1-2/

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8880644 [details]
Bug 1369932 - Fix typo in aom build description.

https://reviewboard.mozilla.org/r/151978/#review157152

Doh!

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8880640 [details]
Bug 1369932 - Enable av1 playback in nightly Windows builds.

https://reviewboard.mozilla.org/r/151968/#review157154

::: toolkit/moz.configure:417
(Diff revision 2)
>  
>  @depends('--enable-av1', target, milestone)
>  def av1(value, target, milestone):
>      enabled = bool(value)
>      if value.origin == 'default' and milestone.is_nightly:
> -        enabled = target.os not in ('Android', 'WINNT')
> +        enabled = target.os not in ('Android')

I think this is not going to do what you want, because `('Android')` is a string, not a single-element tuple.  So this is actually going to turn *off* AOM for all nightlies.

Maybe `target.os != 'Android'`?

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 8880639 [details]
Bug 1369932 - Backport win32 build fix for aom.

https://reviewboard.mozilla.org/r/151966/#review157156

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 8880640 [details]
Bug 1369932 - Enable av1 playback in nightly Windows builds.

https://reviewboard.mozilla.org/r/151968/#review157158

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8880640 [details]
Bug 1369932 - Enable av1 playback in nightly Windows builds.

https://reviewboard.mozilla.org/r/151968/#review157154

> I think this is not going to do what you want, because `('Android')` is a string, not a single-element tuple.  So this is actually going to turn *off* AOM for all nightlies.
> 
> Maybe `target.os != 'Android'`?

So I wanted `target.os in ('Android',)`? Your suggestion si more clear, Thanks.

I'm confused about turning off AOM everywhere though. Isn't `'Android' not in ('Android')` False but `'WINNT' not in ('Android')` True?

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 8880640 [details]
Bug 1369932 - Enable av1 playback in nightly Windows builds.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151968/diff/2-3/

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8880640 [details]
Bug 1369932 - Enable av1 playback in nightly Windows builds.

https://reviewboard.mozilla.org/r/151968/#review157252

Thank you!

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8880640 [details]
Bug 1369932 - Enable av1 playback in nightly Windows builds.

https://reviewboard.mozilla.org/r/151968/#review157154

> So I wanted `target.os in ('Android',)`? Your suggestion si more clear, Thanks.
> 
> I'm confused about turning off AOM everywhere though. Isn't `'Android' not in ('Android')` False but `'WINNT' not in ('Android')` True?

Indeed it is.  I had seen problems with people thinking that `(X)` in Python gives them a single-element tuple, when it does not, and assumed that case applied here.  But `'WINNT' not in ('Android')` or even `'WINNT' not in 'Android'` returns `True`.

Ah, the Python docs note that for string or unicode objects, `in` and `not in` perform substring tests, not membership:

https://docs.python.org/2.7/library/stdtypes.html#sequence-types-str-unicode-list-tuple-bytearray-buffer-xrange

So while the proposed code "works" in the sense that it gives the right answer, the actual semantics are probably not what you intended!  WDYT--changing it would be good?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/599d7bc3e566
Backport win32 build fix for aom. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/77dc3807eaa2
Fix typo in aom build description. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/744f286690d7
Enable av1 playback in nightly Windows builds. r=froydnj

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

I see some alerts from the build metrics related to build times, compiler warnings and installer size:
== Change summary for alert #7506 (as of June 23 2017 17:10 UTC) ==

Regressions:

  3%  build times summary windows2012-32 pgo taskcluster-c4.4xlarge     4,135.59 -> 4,277.11
  3%  build times summary windows2012-64 pgo taskcluster-c4.4xlarge     4,468.01 -> 4,592.59
  2%  compiler warnings summary windows2012-32 pgo                      138.08 -> 141.00
  2%  compiler warnings summary windows2012-64 pgo                      154.00 -> 157.00
  2%  compiler warnings summary windowsxp opt                           129.00 -> 131.00
  2%  compiler warnings summary windows2012-32 opt                      129.00 -> 131.00
  1%  compiler warnings summary windows2012-32-noopt debug              134.00 -> 136.00
  1%  compiler warnings summary windowsxp debug                         134.00 -> 136.00
  1%  compiler warnings summary windows2012-32 debug                    134.00 -> 136.00
  1%  compiler warnings summary windows2012-64 opt                      145.00 -> 147.00
  1%  compiler warnings summary windows8-64 opt                         145.08 -> 147.00
  1%  compiler warnings summary windows2012-64-noopt debug              151.00 -> 153.00
  1%  compiler warnings summary windows8-64 debug                       151.00 -> 153.00
  1%  compiler warnings summary windows2012-64 debug                    151.00 -> 153.00
  1%  installer size summary windows2012-32 pgo                         50,421,476.25 -> 51,069,052.67

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7506

there are no expectations to fix this, this is for information only- it would be nice to know if this is expected.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Thanks Joel.
Same as bug 1371609

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/599d7bc3e566
https://hg.mozilla.org/mozilla-central/rev/77dc3807eaa2
https://hg.mozilla.org/mozilla-central/rev/744f286690d7