Closed Bug 1369932 Opened 3 years ago Closed 3 years ago

libaom doesn't build for windows

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: rillian, Assigned: rillian)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

Various versions of libaom fail to build under mvsc, usually issues with the simd intrinsics. This is a tracking bug for resolving these issues.
Priority: -- → P3
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
Depends on: 1370975
Assignee: nobody → giles
Attachment #8880639 - Flags: review?(kinetik)
Attachment #8880644 - Flags: review?(nfroyd)
Attachment #8880640 - Flags: review?(nfroyd)
Attachment #8880640 - Flags: review?(kinetik)
Comment on attachment 8880644 [details]
Bug 1369932 - Fix typo in aom build description.

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

Doh!
Attachment #8880644 - Flags: review?(nfroyd) → review+
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'`?
Attachment #8880640 - Flags: review?(nfroyd) → review-
Comment on attachment 8880639 [details]
Bug 1369932 - Backport win32 build fix for aom.

https://reviewboard.mozilla.org/r/151966/#review157156
Attachment #8880639 - Flags: review?(kinetik) → review+
Comment on attachment 8880640 [details]
Bug 1369932 - Enable av1 playback in nightly Windows builds.

https://reviewboard.mozilla.org/r/151968/#review157158
Attachment #8880640 - Flags: review?(kinetik)
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 on attachment 8880640 [details]
Bug 1369932 - Enable av1 playback in nightly Windows builds.

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

Thank you!
Attachment #8880640 - Flags: review?(nfroyd) → review+
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?
Pushed by rgiles@mozilla.com:
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
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.
Thanks Joel.
Same as bug 1371609
See Also: → 1371609
Depends on: 1376174
Depends on: 1378518
Depends on: 1379039
You need to log in before you can comment on or make changes to this bug.