Issue 1403893007: Adding third_party/openh264/src, build files and unittests

hbos_chromium

Description was changed from ========== third_party/OpenH264, gyp files and tests WIP CL, do not land ...

5 years, 2 months ago (2015-10-20 09:35:32 UTC) #1

hbos_chromium

Description was changed from ========== third_party/OpenH264, gyp files and tests WIP CL, do not land ...

5 years, 1 month ago (2015-10-29 14:21:39 UTC) #4

hbos_chromium

Description was changed from ========== Adding third_party/openh264/src, build files and unittests. Currently, only gyp build ...

5 years, 1 month ago (2015-10-29 14:26:37 UTC) #5

Description was changed from

==========
Adding third_party/openh264/src, build files and unittests.

Currently, only gyp build files have been created. TODO (before landing this
CL): Add GN build files as well! GN bots pass because openh264 is not built.

There is a CL in the WebRTC repo that adds encoder and decoder using OpenH264
(https://codereview.webrtc.org/1306813009/). It also has encoder/decoder test
code, but these are in WebRTC. It is blocked on this CL, in the Chromium repo,
for building OpenH264 from source. In order to test that the build file are
correct here in Chromium some encoder/decoder test code is required. To simplify
testing the WebRTC encoder/decoder are used, but since these have not landed yet
copies of them have temporarily been added to third_party/openh264/testing/.
When both CLs have landed a follow-up CL should remove the copies and rely on
the WebRTC encoder/decoder.

Note: In this CL (https://codereview.chromium.org/1394983003) a new unittest
binary was created for OpenH264. This was a bad move due to overhead, only
wanting the tests enabled if a flag was set, and it being easier and more
appropriate to reuse content_unittests for our OpenH264 tests. As such this CL
also removes those old test files.

Note: This CL should not land unless build_openh264 is 0 by default.

COMMIT=false
BUG=500605, 468365
==========

to

==========
Adding third_party/openh264/src, build files and unittests.

Currently, only gyp build files have been created. TODO (before landing this
CL): Add GN build files as well! GN bots pass because openh264 is not built.

Only building C++. Architecture-specific assembly code will be a different CL.

There is a CL in the WebRTC repo that adds encoder and decoder using OpenH264
(https://codereview.webrtc.org/1306813009/). It also has encoder/decoder test
code, but these are in WebRTC. It is blocked on this CL, in the Chromium repo,
for building OpenH264 from source. In order to test that the build file are
correct here in Chromium some encoder/decoder test code is required. To simplify
testing the WebRTC encoder/decoder are used, but since these have not landed yet
copies of them have temporarily been added to third_party/openh264/testing/.
When both CLs have landed a follow-up CL should remove the copies and rely on
the WebRTC encoder/decoder.

Note: In this CL (https://codereview.chromium.org/1394983003) a new unittest
binary was created for OpenH264. This was a bad move due to overhead, only
wanting the tests enabled if a flag was set, and it being easier and more
appropriate to reuse content_unittests for our OpenH264 tests. As such this CL
also removes those old test files.

Note: This CL should not land unless build_openh264 is 0 by default.

COMMIT=false
BUG=500605, 468365
==========

hbos_chromium

hbos@chromium.org changed reviewers: + torbjorng@chromium.org

5 years, 1 month ago (2015-10-29 14:28:25 UTC) #6

hbos_chromium

Do you have time to take a look torbjorng? I'll think of other reviewers as ...

5 years, 1 month ago (2015-10-29 14:28:26 UTC) #7

hbos_chromium

hbos@chromium.org changed reviewers: + phoglund@chromium.org

5 years, 1 month ago (2015-11-02 15:06:00 UTC) #8

hbos_chromium

And +phoglund as reviewer. It's not in a landable state yet (for example I need ...

5 years, 1 month ago (2015-11-02 15:06:01 UTC) #9

hbos_chromium

Description was changed from ========== Adding third_party/openh264/src, build files and unittests. Currently, only gyp build ...

5 years, 1 month ago (2015-11-03 15:00:49 UTC) #10

Description was changed from

==========
Adding third_party/openh264/src, build files and unittests.

Currently, only gyp build files have been created. TODO (before landing this
CL): Add GN build files as well! GN bots pass because openh264 is not built.

Only building C++. Architecture-specific assembly code will be a different CL.

There is a CL in the WebRTC repo that adds encoder and decoder using OpenH264
(https://codereview.webrtc.org/1306813009/). It also has encoder/decoder test
code, but these are in WebRTC. It is blocked on this CL, in the Chromium repo,
for building OpenH264 from source. In order to test that the build file are
correct here in Chromium some encoder/decoder test code is required. To simplify
testing the WebRTC encoder/decoder are used, but since these have not landed yet
copies of them have temporarily been added to third_party/openh264/testing/.
When both CLs have landed a follow-up CL should remove the copies and rely on
the WebRTC encoder/decoder.

Note: In this CL (https://codereview.chromium.org/1394983003) a new unittest
binary was created for OpenH264. This was a bad move due to overhead, only
wanting the tests enabled if a flag was set, and it being easier and more
appropriate to reuse content_unittests for our OpenH264 tests. As such this CL
also removes those old test files.

Note: This CL should not land unless build_openh264 is 0 by default.

COMMIT=false
BUG=500605, 468365
==========

to

==========
Adding third_party/openh264/src, build files and unittests.

Only building C++. Architecture-specific assembly code will be a different CL.

There is a CL in the WebRTC repo that adds encoder and decoder using OpenH264
(https://codereview.webrtc.org/1306813009/). It also has encoder/decoder test
code, but these are in WebRTC. It is blocked on this CL, in the Chromium repo,
for building OpenH264 from source. In order to test that the build file are
correct here in Chromium some encoder/decoder test code is required. To simplify
testing the WebRTC encoder/decoder are used, but since these have not landed yet
copies of them have temporarily been added to third_party/openh264/testing/.
When both CLs have landed a follow-up CL should remove the copies and rely on
the WebRTC encoder/decoder.

Note: In this CL (https://codereview.chromium.org/1394983003) a new unittest
binary was created for OpenH264. This was a bad move due to overhead, only
wanting the tests enabled if a flag was set, and it being easier and more
appropriate to reuse content_unittests for our OpenH264 tests. As such this CL
also removes those old test files.

Note: This CL should not land unless build_openh264 is 0/false by default.

COMMIT=false
BUG=500605, 468365
==========

torbjorng

Great job, this is naturally messy to get through on all platforms! I have only ...

5 years, 1 month ago (2015-11-05 13:18:25 UTC) #12

hbos_chromium

torbjorng can you take another look? Now ignoring individual warnings, not trying not to treat ...

5 years, 1 month ago (2015-11-10 14:36:47 UTC) #13

hbos_chromium

Hmm, I'll split this up into two CLs (one removing the old test stuff and ...

5 years, 1 month ago (2015-11-10 15:59:02 UTC) #14

hbos_chromium

Description was changed from ========== Adding third_party/openh264/src, build files and unittests. Only building C++. Architecture-specific ...

5 years, 1 month ago (2015-11-11 10:41:21 UTC) #15

Description was changed from

==========
Adding third_party/openh264/src, build files and unittests.

Only building C++. Architecture-specific assembly code will be a different CL.

There is a CL in the WebRTC repo that adds encoder and decoder using OpenH264
(https://codereview.webrtc.org/1306813009/). It also has encoder/decoder test
code, but these are in WebRTC. It is blocked on this CL, in the Chromium repo,
for building OpenH264 from source. In order to test that the build file are
correct here in Chromium some encoder/decoder test code is required. To simplify
testing the WebRTC encoder/decoder are used, but since these have not landed yet
copies of them have temporarily been added to third_party/openh264/testing/.
When both CLs have landed a follow-up CL should remove the copies and rely on
the WebRTC encoder/decoder.

Note: In this CL (https://codereview.chromium.org/1394983003) a new unittest
binary was created for OpenH264. This was a bad move due to overhead, only
wanting the tests enabled if a flag was set, and it being easier and more
appropriate to reuse content_unittests for our OpenH264 tests. As such this CL
also removes those old test files.

Note: This CL should not land unless build_openh264 is 0/false by default.

COMMIT=false
BUG=500605, 468365
==========

to

==========
Adding third_party/openh264/src, build files and unittests.

Only building C++. Architecture-specific assembly code will be a different CL.

--- This CL will be split up and closed for reviewability ---
CL #1: https://codereview.chromium.org/1438553002/

There is a CL in the WebRTC repo that adds encoder and decoder using OpenH264
(https://codereview.webrtc.org/1306813009/). It also has encoder/decoder test
code, but these are in WebRTC. It is blocked on this CL, in the Chromium repo,
for building OpenH264 from source. In order to test that the build file are
correct here in Chromium some encoder/decoder test code is required. To simplify
testing the WebRTC encoder/decoder are used, but since these have not landed yet
copies of them have temporarily been added to third_party/openh264/testing/.
When both CLs have landed a follow-up CL should remove the copies and rely on
the WebRTC encoder/decoder.

Note: In this CL (https://codereview.chromium.org/1394983003) a new unittest
binary was created for OpenH264. This was a bad move due to overhead, only
wanting the tests enabled if a flag was set, and it being easier and more
appropriate to reuse content_unittests for our OpenH264 tests. As such this CL
also removes those old test files.

Note: This CL should not land unless use_openh264 is 0/false by default.

COMMIT=false
BUG=500605, 468365
==========

torbjorng

LGTM assuming this is disabled by default (for reasons pointed out in the CL). Some ...

5 years, 1 month ago (2015-11-12 11:02:02 UTC) #16

hbos_chromium

Description was changed from ========== Adding third_party/openh264/src, build files and unittests. Only building C++. Architecture-specific ...

5 years, 1 month ago (2015-11-13 16:14:58 UTC) #17

Description was changed from

==========
Adding third_party/openh264/src, build files and unittests.

Only building C++. Architecture-specific assembly code will be a different CL.

--- This CL will be split up and closed for reviewability ---
CL #1: https://codereview.chromium.org/1438553002/

There is a CL in the WebRTC repo that adds encoder and decoder using OpenH264
(https://codereview.webrtc.org/1306813009/). It also has encoder/decoder test
code, but these are in WebRTC. It is blocked on this CL, in the Chromium repo,
for building OpenH264 from source. In order to test that the build file are
correct here in Chromium some encoder/decoder test code is required. To simplify
testing the WebRTC encoder/decoder are used, but since these have not landed yet
copies of them have temporarily been added to third_party/openh264/testing/.
When both CLs have landed a follow-up CL should remove the copies and rely on
the WebRTC encoder/decoder.

Note: In this CL (https://codereview.chromium.org/1394983003) a new unittest
binary was created for OpenH264. This was a bad move due to overhead, only
wanting the tests enabled if a flag was set, and it being easier and more
appropriate to reuse content_unittests for our OpenH264 tests. As such this CL
also removes those old test files.

Note: This CL should not land unless use_openh264 is 0/false by default.

COMMIT=false
BUG=500605, 468365
==========

to

==========
Adding third_party/openh264/src, build files and unittests.

Only building C++. Architecture-specific assembly code will be a different CL.

--- This CL will be split up and closed for reviewability ---
CL #1: https://codereview.chromium.org/1438553002/
CL #2: https://codereview.chromium.org/1446453004/

There is a CL in the WebRTC repo that adds encoder and decoder using OpenH264
(https://codereview.webrtc.org/1306813009/). It also has encoder/decoder test
code, but these are in WebRTC. It is blocked on this CL, in the Chromium repo,
for building OpenH264 from source. In order to test that the build file are
correct here in Chromium some encoder/decoder test code is required. To simplify
testing the WebRTC encoder/decoder are used, but since these have not landed yet
copies of them have temporarily been added to third_party/openh264/testing/.
When both CLs have landed a follow-up CL should remove the copies and rely on
the WebRTC encoder/decoder.

Note: In this CL (https://codereview.chromium.org/1394983003) a new unittest
binary was created for OpenH264. This was a bad move due to overhead, only
wanting the tests enabled if a flag was set, and it being easier and more
appropriate to reuse content_unittests for our OpenH264 tests. As such this CL
also removes those old test files.

Note: This CL should not land unless use_openh264 is 0/false by default.

COMMIT=false
BUG=500605, 468365
==========

hbos_chromium

5 years, 1 month ago (2015-11-16 16:56:29 UTC) #18

Message was sent while issue was closed.

<Responding to torbjorng's comments even though this CL has been re-uploaded,
further reviewing should take place there:
https://codereview.chromium.org/1446453004/>

"I did not check gyp/gn consistency carefully. Do we build the same thing, pass
the same flags, etc?"
- The gyp/gn files are slightly different, must be because different flags are
passed by default. For example, in gyp I have to list a lot of warnings to
ignore for MSVS that I don't have to list in GN.

As for inspecting the resulting compiler flags being produced I have not looked
at this.

https://codereview.chromium.org/1403893007/diff/980001/third_party/openh264/B...
File third_party/openh264/BUILD.gn (right):

https://codereview.chromium.org/1403893007/diff/980001/third_party/openh264/B...
third_party/openh264/BUILD.gn:18: "-fvisibility=default",
I should check if this (-fvisiblity=default) is really needed because I didn't
need it in the gyp file.

https://codereview.chromium.org/1403893007/diff/980001/third_party/openh264/B...
third_party/openh264/BUILD.gn:29: # Workaround relating to the VERSION_NUMBER
macro in version.h.
On 2015/11/12 11:02:02, torbjorng wrote:
> I am not too excited about these workarounds. If is funny that you get it
> defined 0 or 2 times. In the latter case, do you know if the source is
> OpenH264's version.h for both defs?

OpenH264's version.h has include guards, it redefining the macro should be
impossible. When it's defined twice it's because the first time it is us
defining it in the build file (resulting in -DVERSION_NUMBER=... compiler flag
or similar) and the second time is due to our version.h...

Hang on a minute, if I also define the include guards of version.h then the
macro will be defined without it being redefined on #include!

https://codereview.chromium.org/1403893007/diff/980001/third_party/openh264/B...
third_party/openh264/BUILD.gn:79: configs -= [
"//build/config/compiler:chromium_code" ]
On 2015/11/12 11:02:02, torbjorng wrote:
> I am not good at gn, but perhaps the "configs" manipulation could be inherited
> from "openh264_common" (here and for encoder and decoder)?

I don't think that's possible, openh264_common is a separate target that we
depend on here, and this is a different target with its own configurations.

https://codereview.chromium.org/1403893007/diff/980001/third_party/openh264/b...
File third_party/openh264/buildnotes.txt (right):

https://codereview.chromium.org/1403893007/diff/980001/third_party/openh264/b...
third_party/openh264/buildnotes.txt:1: # Notes, just me trying to understand the
.mk files with pseudocode...
On 2015/11/12 11:02:02, torbjorng wrote:
> This file should probably be removed.

I don't include this in the reuploaded version.

Issue 1403893007: Adding third_party/openh264/src, build files and unittests (Closed)