Bugzilla

Reporter

Comment 2

•

8 years ago

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> Does this issue belong in the Mozilla bug tracker? I figured this kind of
> issue would end up in https://github.com/cisco/openh264/issues

I threw it in our bug tracker and e-mailed our Point of Contact, since it blocks actual work they asked us to do. (and that our own team(s) want us to do).  Certainly the work will get tracked/fixed on their end.

Flags: needinfo?(bugspam.Callek)

Comment 3

•

8 years ago

Yes, we got the info for this bug. We're investigating it now. Thanks.

Comment 4

•

8 years ago

Which version of nasm are you using? 

Here is the the nasm requirement from https://github.com/cisco/openh264:
NASM needed to be installed for assembly code: workable version 2.10.06 or above, nasm can downloaded from http://www.nasm.us/ For Mac OSX 64-bit NASM needed to be below version 2.11.08 as nasm 2.11.08 will introduce error when using RIP-relative addresses in Mac OSX 64-bit.

Flags: needinfo?(bugspam.Callek)

Reporter

Comment 5

•

8 years ago

08:28:41     INFO -  Package nasm-2.07-7.el6.x86_64 already installed and latest version
08:28:41     INFO -  Package yasm-1.1.0-1.x86_64 already installed and latest version

(We use a stable mirror of package, so we don't introduce undue dependancies into Firefox -- this version of NASM was used to build the 1.5.x OpenH264 version)

We use yasm (not that exact version iirc) on OSX and Windows.

Flags: needinfo?(bugspam.Callek) → needinfo?(hankpeng)

Updated

•

8 years ago

Flags: needinfo?(hankpeng)

Comment 6

•

8 years ago

Justin, I am sorry to tell you that some new assembly code is added since openh264 v1.5.3. And now the required nasm version is changed from "2.07 or above" to "2.10.06 or above".

See the code change here:
https://github.com/cisco/openh264/compare/v1.5.3-Firefox39...v1.6-Firefox39#diff-c30c448042c1ceef6443d1bac5d616dfR1508

See the nasm upgrade requirement here:
https://github.com/cisco/openh264/compare/v1.5.3-Firefox39...v1.6-Firefox39#diff-04c6e90faac2675aa89e2176d2eec7d8L53

Flags: needinfo?(bugspam.Callek)

Comment 7

•

8 years ago

@Haibo, is there any other option instead of upgrading nasm to avoid this issue?

Flags: needinfo?(haibozhu)

Reporter

Comment 8

•

8 years ago

(In reply to Hank Peng from comment #7)
> @Haibo, is there any other option instead of upgrading nasm to avoid this
> issue?

To be clear, its not impossible for us to get a newer nasm for openh264's use only. (We can download a binary tarball and point PATH at that, rather than system NASM) -- its just more overhead in the script and will delay us more on our end.  (Especially if I have to do the same logic for Windows's Yasm)

Flags: needinfo?(bugspam.Callek)

Reporter

Comment 9

•

8 years ago

(In reply to Justin Wood (:Callek) from comment #8)
> (In reply to Hank Peng from comment #7)
> > @Haibo, is there any other option instead of upgrading nasm to avoid this
> > issue?
> 
> To be clear, its not impossible for us to get a newer nasm for openh264's
> use only. (We can download a binary tarball and point PATH at that, rather
> than system NASM) -- its just more overhead in the script and will delay us
> more on our end.  (Especially if I have to do the same logic for Windows's
> Yasm)

This "not impossible" turns out to be harder than first thought -- Our machines don't have a GLIBC 2.14 library around. We have a few options but none of them are the easiest options on our end.

Comment 10

•

8 years ago

> This "not impossible" turns out to be harder than first thought -- Our
> machines don't have a GLIBC 2.14 library around. We have a few options but
> none of them are the easiest options on our end.

Thanks for trying nasm upgrading. I will sync up with team to see if we could find another workaround option. Will let you know the result.

Comment 11

•

8 years ago

@Justin Wood, about AVX support and nasm version too old problem, locally we have two solutions: 
(1) make the gmp plugin independently, and the new libgmpopenh264.so will call libopenh264.so, which can be downloaded from openh264 CDN. 
(2) Disable the all AVX related code with an predefined macro and can open/close this macro from the make options.

Reporter

Comment 12

•

8 years ago

Maire,

Can you figure out what we 'want' re: c#11. I'm not sure implications on any of the options specified, however I'm up for whatever makes sense.

I still have a few options (though more time consuming) to get nasm working (I articulated to maire in an e-mail), but I'm not sure what the options in c#11 mean for us/our users, and I feel that if there is a trade off decision it should be up to Maire (or people she delegates to) to decide on. At least from my end.

Flags: needinfo?(mreavy)

Maire Reavy [:mreavy]

Comment 13

•

8 years ago

Ref: comment 11

Option (1) is really a non-starter.  It's a bunch of work, especially when we include testing, with very little return on that time investment.

@Haibo -- Regarding Option (2), are we sure that we need to disable all AVX related code?  Firefox uses AVX2 code in libyuv (as inline assembly on linux, win and mac).

Flags: needinfo?(mreavy)

Comment 14

•

8 years ago

(In reply to Maire Reavy [:mreavy] (Pls needinfo) from comment #13)
> @Haibo -- Regarding Option (2), are we sure that we need to disable all AVX
> related code?  Firefox uses AVX2 code in libyuv (as inline assembly on
> linux, win and mac).

@Maire, Option (2) comes from the fix (--disable-avx2) for the Bug 1214462 - Support ffmpeg on Windows . 

As said in https://github.com/mozilla/gecko-dev/blob/master/media/ffvpx/README_MOZILLA:

"AVX2 must be disabled on Linux due to the use of yasm 1.1 on the build bots.
Once yasm is upgraded to 1.2 or later, AVX2 code could be re-enabled.
Add --disable-avx2 to configure on those platforms."

We'd like to provide a similar compiling option for OpenH264. Before upgrading your build system, we are supposing you could use that option to disable the AVX2 assembly code.

Does it make sense?

Flags: needinfo?(mreavy)

Comment 15

•

8 years ago

@Maire, making all the AVX2 assembly code inline to get it built by compiler instead of assembler is another good option. I am afraid it may take longer time to make it work on all the supported platforms. We'll evaluate to see if it is easy to do so.

Maire Reavy [:mreavy]

Comment 16

•

8 years ago

Thanks, Hank.  (Ref: Comment 15) If it turns out there's any easy way to do so, that's great, but I suspect there isn't.  I'm ok with disabling AVX2 so long as we believe there isn't a major performance hit. I just wanted to understand why we'd need to disable it.

@Callek -- Justin, we'll likely do another release of OpenH264 (after this one) sometime in Q4.  Do you think you can have the builders updated by then?  Thanks!

Flags: needinfo?(mreavy) → needinfo?(bugspam.Callek)

Reporter

Comment 17

•

8 years ago

(In reply to Maire Reavy [:mreavy] (Pls needinfo) from comment #16)
> @Callek -- Justin, we'll likely do another release of OpenH264 (after this
> one) sometime in Q4.  Do you think you can have the builders updated by
> then?  Thanks!

I can plan for needing either a new nasm or building on taskcluster (with the newer toolchain) in time for a "sometime in Q4" timeline build, yes.

Flags: needinfo?(bugspam.Callek)

Comment 18

•

8 years ago

Firstly, we will try to add an option in make file which can disable the AVX related code to make the current build process on old system can pass.

Comment 19

•

8 years ago

As discussed with Haibo, the encoding/decoding performance loss caused by disabling AVX2 assembly code is minor. He would try to provide some data. We're now improving the build script to support the compiling option to disable AVX2. When inform you when it is ready. Thanks.

Comment 20

•

8 years ago

Hi, about the NASM support problem, update some informations:
(1) Can FF make the newer nasm from the source code and update the old nasm with it? It can solve the lib dependency problem and no performance loss, I think.
(2) We established a branch which can disable the AVX2 related code when make openh264. The source code can be got from: https://github.com/GuangweiWang/openh264/, branch enable-disable-AVX2. Make option is HAVE_AVX2=false. I have tested it locally with nasm 2.07. Can you help to test it on your environment. We will merge it into branch openh264v1.6 when your test passed and (1) not been selected.
B.T.W., about the performance loss when we disable AVX2, I have no detailed information about it. But the AVX2 disabled openh264 v1.6 is faster than the previous v1.5.

Comment 21

•

8 years ago

@Maire, is it workable for you to upgrade the nasm from source code? 
@Justin, please help verify if the build option HAVE_AVX2=false could solve the problem. 
Let us know which option you prefer.

Flags: needinfo?(mreavy)

Flags: needinfo?(bugspam.Callek)

Maire Reavy [:mreavy]

Comment 22

•

8 years ago

(In reply to Hank Peng from comment #21)
> @Maire, is it workable for you to upgrade the nasm from source code? 
> @Justin, please help verify if the build option HAVE_AVX2=false could solve
> the problem. 
> Let us know which option you prefer.

I think the first question is also for Justin.  Justin -- what would it take to upgrade the nasm from source?  I'm thinking this isn't something we could do in the v1.6 time frame. (I'd like us to get v1.6 tested and rolled out by the end of August.  This deadline is not a hard one, but this timeline is what I would like to achieve.)  Thanks.

Flags: needinfo?(mreavy)

Reporter

Comment 23

•

8 years ago

(In reply to Maire Reavy [:mreavy] (Pls needinfo) from comment #22)
> (In reply to Hank Peng from comment #21)
> > @Maire, is it workable for you to upgrade the nasm from source code? 
> > @Justin, please help verify if the build option HAVE_AVX2=false could solve
> > the problem. 
> > Let us know which option you prefer.
> 
> I think the first question is also for Justin.  Justin -- what would it take
> to upgrade the nasm from source?  I'm thinking this isn't something we could
> do in the v1.6 time frame. (I'd like us to get v1.6 tested and rolled out by
> the end of August.  This deadline is not a hard one, but this timeline is
> what I would like to achieve.)  Thanks.

Doing that would be a very tight timeline. So with your blessing, I'm going to persue option 2 (see if the branch works) and report back here.

If the fixes required to ship without an upgraded nasm end up being too invasive, then I'll revert to trying to build nasm from source, with the knowledge that if we slip its one of our only options to ship so worth the effort and timeline creep.

I'll touch back here within the next ~24 hours to detail results of that testing.

Comment 24

•

8 years ago

Thanks, Maire and Justin.

Reporter

Comment 25

•

8 years ago

Sooo, the code as provided on that branch doesn't seem to disable AVX2 for us here.

(I don't see any actual tests for AVX2 support that would toggle this flag in the codebase either, fwiw)

I'm happy to test more if needed.

07:04:34     INFO -  g++ -O3 -DNDEBUG -m32 -DX86_ASM -DX86_32_ASM -DHAVE_AVX2 -Wall -fno-strict-aliasing -fPIC -MMD -MP -DGENERATED_VERSION_HEADER -DHAVE_AVX2 -I./codec/api/svc -I./codec/common/inc -Icodec/common/inc  -I./codec/encoder/core/inc -I./codec/encoder/plus/inc -I./codec/processing/interface -c -o codec/encoder/plus/src/welsEncoderExt.o codec/encoder/plus/src/welsEncoderExt.cpp
07:04:34     INFO -  nasm -DX86_32 -DHAVE_AVX2 -f elf -I./codec/common/x86/   -o codec/encoder/core/x86/coeff.o codec/encoder/core/x86/coeff.asm
07:04:34     INFO -  nasm -DX86_32 -DHAVE_AVX2 -f elf -I./codec/common/x86/   -o codec/encoder/core/x86/dct.o codec/encoder/core/x86/dct.asm
07:04:34     INFO -  nasm -DX86_32 -DHAVE_AVX2 -f elf -I./codec/common/x86/   -o codec/encoder/core/x86/intra_pred.o codec/encoder/core/x86/intra_pred.asm
07:04:34     INFO -  nasm -DX86_32 -DHAVE_AVX2 -f elf -I./codec/common/x86/   -o codec/encoder/core/x86/matrix_transpose.o codec/encoder/core/x86/matrix_transpose.asm
07:04:34     INFO -  nasm -DX86_32 -DHAVE_AVX2 -f elf -I./codec/common/x86/   -o codec/encoder/core/x86/memzero.o codec/encoder/core/x86/memzero.asm
07:04:34     INFO -  nasm -DX86_32 -DHAVE_AVX2 -f elf -I./codec/common/x86/   -o codec/encoder/core/x86/quant.o codec/encoder/core/x86/quant.asm
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:392: error: parser: instruction expected
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:393: error: symbol `vbroadcasti128' redefined
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:393: error: parser: instruction expected
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:415: error: parser: instruction expected
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:421: error: symbol `vpbroadcastw' redefined
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:421: error: parser: instruction expected
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:440: error: symbol `vbroadcasti128' redefined
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:440: error: parser: instruction expected
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:441: error: symbol `vbroadcasti128' redefined
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:441: error: parser: instruction expected
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:468: error: symbol `vbroadcasti128' redefined
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:468: error: parser: instruction expected
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:469: error: symbol `vbroadcasti128' redefined
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:469: error: parser: instruction expected
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:477: error: parser: instruction expected
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:478: error: symbol `vperm2i128' redefined
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:478: error: parser: instruction expected
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:486: error: symbol `vperm2i128' redefined
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:486: error: parser: instruction expected
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:487: error: symbol `vperm2i128' redefined
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:487: error: parser: instruction expected
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:491: error: parser: instruction expected
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:492: error: symbol `vextracti128' redefined
07:04:34     INFO -  codec/encoder/core/x86/quant.asm:492: error: parser: instruction expected
07:04:34     INFO -  make: *** [codec/encoder/core/x86/quant.o] Error 1
07:04:34    ERROR - Return code: 2
07:04:34    FATAL - couldn't build plugin
07:04:34    FATAL - Running post_fatal callback...
07:04:34    FATAL - Exiting -1

Flags: needinfo?(bugspam.Callek) → needinfo?(hankpeng)

Reporter

Comment 26

•

8 years ago

(In reply to Justin Wood (:Callek) from comment #25)
> (I don't see any actual tests for AVX2 support that would toggle this flag
> in the codebase either, fwiw)

Err looks like its not defined for android, let me test that one real quick...

...and nope, its not defined for *arm* android, but x86 android hits the first block and hits the same error as previous comment

Comment 27

•

8 years ago

Justin, thanks for trying. I can see that the build option "HAVE_AVX2" is defined according to the build output. It is defined by default. In order to disable it, you have to add an option to the build command, like "make HAVE_AVX2=false". This option has to be specified manually. 

By the way, please make sure you're working on the branch "enable-disable-AVX2" of git repo " git@github.com:GuangweiWang/openh264.git". The change is here,  https://github.com/GuangweiWang/openh264/commits/enable-disable-AVX2. This change is not committed into the official git repo so far. We'd like to commit it only when we're sure this option works for you.

Flags: needinfo?(hankpeng)

Comment 28

•

8 years ago

Justin, would you mind making a try again? Thanks.

Flags: needinfo?(bugspam.Callek)

Reporter

Comment 29

•

8 years ago

(In reply to Hank Peng from comment #28)
> Justin, would you mind making a try again? Thanks.

Woo Hoo, indeed it does work for both Linux32 and Android-x86, Im assuming linux64 will have the same results.

Please merge this in and we can close this bug, and I can proceed with the official repo.

Flags: needinfo?(bugspam.Callek)

Comment 30

•

8 years ago

Hi, we have merged this commit into master, openh264v1.6 and v1.6-Firefox39 branches.

Comment 31

•

8 years ago

HI Justin, please continue to integrate the official v1.6-Firefox39 branch. Please let us know if there is any other issue. Thanks.

Flags: needinfo?(bugspam.Callek)

Reporter

Comment 32

•

8 years ago

We have built and passed back v1.6 now. There were no more issues here

Status: NEW → RESOLVED

Closed: 8 years ago

Flags: needinfo?(haibozhu)

Flags: needinfo?(bugspam.Callek)

Resolution: --- → FIXED