Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1446453004:
Adding third_party/openh264 build files for encoding (Closed)
|
Created:
5 years, 1 month ago by hbos_chromium Modified:
5 years ago CC:
chromium-reviews, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org, jschuh Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding third_party/openh264 build files for encoding (but not decoding).
Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s).
(This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up into pieces and removed decoding and tests).
BUG=500605, 468365
Committed: https://crrev.com/17a7b4a2959f7b65f764fffcdb7b39e276f040b7
Cr-Commit-Position: refs/heads/master@{#363443}
Patch Set 1 : #Patch Set 2 : Moved BUILD.gn deps to 'both_gn_and_gyp', added corresponding change to gn_migration.gypi #
Total comments: 11
Patch Set 3 : Rebase with master #
Total comments: 10
Patch Set 4 : Addressed comments #Patch Set 5 : Addressed torbjorng's comments #
Total comments: 15
Patch Set 6 : Addressed comments (and temporarily made use_openh264 unconditionally true for trybots) #Patch Set 7 : Moved openh264_unittests.cc from content_unittests to media_unittests #Patch Set 8 : Moved tests to a new test binary, openh264_unittests #Patch Set 9 : use_openh264 = conditional on chrome branding again #
Total comments: 25
Patch Set 10 : Addressed brettw comments, (use_openh264 = true again for trybots) #
Total comments: 4
Patch Set 11 : Addressed comments #Patch Set 12 : Rebase with master (and targets in correct alphabetical order after removing "openh264_" prefix) #Patch Set 13 : Removed openh264/testing/ #Patch Set 14 : use_openh264 off by default. TODO to make it conditional on chrome branding after review is done #Patch Set 15 : Rebase with master #Patch Set 16 : Removed decoder from build files #Patch Set 17 : Rebase /w master and corrected comment about build flag #
Messages
Total messages: 71 (26 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Adding third_party/openh264 build files and unittests <Insert description> (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). COMMIT=false BUG=500605, 468365 ========== to ========== Adding third_party/openh264 build files and unittests Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). 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. (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). *** Will not land this without switching the default value of use_openh264 to 0 and false in GYP and GN, hence COMMIT=false *** COMMIT=false BUG=500605, 468365 ==========
Description was changed from ========== Adding third_party/openh264 build files and unittests Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). 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. (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). *** Will not land this without switching the default value of use_openh264 to 0 and false in GYP and GN, hence COMMIT=false *** COMMIT=false BUG=500605, 468365 ========== to ========== Adding third_party/openh264 build files and unittests Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). 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 files 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. (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). *** Will not land this without switching the default value of use_openh264 to 0 and false in GYP and GN, hence COMMIT=false *** COMMIT=false BUG=500605, 468365 ==========
hbos@chromium.org changed reviewers: + dpranke@chromium.org, phoglund@chromium.org, tommi@chromium.org, torbjorng@chromium.org
Please take a look pdranke, torbjorng, phoglund, tommi. dpranke: Added to review the build files, GYP and GN. phoglund, torbjorng: Added to review the build files and as general reviewers of the whole thing. tommi: Added as the content/ owner for "webrtc files" (is this appropriate?). I've added a unittest inside content_unittests, openh264_unittests.cc. I don't expect anyone to review h264_encoder/decoder_impl since these are essentially temporary copies of the yet-to-land ones in https://codereview.webrtc.org/1306813009/.
https://codereview.chromium.org/1446453004/diff/40001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1446453004/diff/40001/build/common.gypi#newco... build/common.gypi:1251: 'use_openh264%': 1, Only, it's 1. Is this temporary just to run on the trybots? If so, please write so. https://codereview.chromium.org/1446453004/diff/40001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/1446453004/diff/40001/build/config/features.g... build/config/features.gni:61: use_openh264 = true Same here.
https://codereview.chromium.org/1446453004/diff/40001/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/1446453004/diff/40001/build/common.gypi#newco... build/common.gypi:1251: 'use_openh264%': 1, On 2015/11/16 14:51:41, phoglund_chrome wrote: > Only, it's 1. Is this temporary just to run on the trybots? If so, please write > so. Oh, right, you wrote that in the CL desc. Never mind. https://codereview.chromium.org/1446453004/diff/40001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/1446453004/diff/40001/build/config/features.g... build/config/features.gni:61: use_openh264 = true On 2015/11/16 14:51:41, phoglund_chrome wrote: > Same here. Acknowledged.
I think this looks all right, but please remove all commented-out code you have in there (or find a way to keep it in). https://codereview.chromium.org/1446453004/diff/40001/third_party/openh264/te... File third_party/openh264/testing/h264_decoder_impl.cc (right): https://codereview.chromium.org/1446453004/diff/40001/third_party/openh264/te... third_party/openh264/testing/h264_decoder_impl.cc:64: // LOG(LS_ERROR) << "Failed to create OpenH264 decoder"; Nit: remove https://codereview.chromium.org/1446453004/diff/40001/third_party/openh264/te... third_party/openh264/testing/h264_decoder_impl.cc:89: // LOG(LS_ERROR) << "Failed to initialize OpenH264 decoder"; Nit: remove; apply throughout
https://codereview.chromium.org/1446453004/diff/40001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/1446453004/diff/40001/build/config/features.g... build/config/features.gni:61: use_openh264 = true On 2015/11/16 14:52:21, phoglund_chrome wrote: > On 2015/11/16 14:51:41, phoglund_chrome wrote: > > Same here. > > Acknowledged. "The default value should be false", not 0. Also, would we want these to default to true in an official build? If so, we should make that be the case rather than require anyone who wants an official build to have to specify additional flags.
https://codereview.chromium.org/1446453004/diff/40001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/1446453004/diff/40001/build/config/features.g... build/config/features.gni:61: use_openh264 = true One more comment ... we shouldn't be adding new settings to features.gni unless they tend to affect targets globally (which this one doesn't). Can you move this to its own standalone *.gni file and then include it as needed?
lgtm Only minor comments. I did a rather spotty review of the test code, but read the build files a bit more carefully. https://codereview.chromium.org/1446453004/diff/60001/content/browser/media/o... File content/browser/media/openh264_unittests.cc (right): https://codereview.chromium.org/1446453004/diff/60001/content/browser/media/o... content/browser/media/openh264_unittests.cc:23: int shade = 0; Note that shade here will go from 0 to 225, since the accumulation is truncated and the loop does 10 instead of 11 iteration. Perhaps use i*255/10 instead and use i <= kNumSampleFrames (with corresponding sample_frames_ allocation adjustments). https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... File third_party/openh264/testing/h264_decoder_impl.h (right): https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... third_party/openh264/testing/h264_decoder_impl.h:55: const CodecSpecificInfo* codec_specific_info = nullptr, Default parameters are discouraged. Furthermore, this one is ignored. https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... File third_party/openh264/testing/h264_encoder_impl.h (right): https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... third_party/openh264/testing/h264_encoder_impl.h:56: int32_t RegisterEncodeCompleteCallback( Nit: Unneeded linebreak. :-) https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... File third_party/openh264/testing/i420_utils.cc (right): https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... third_party/openh264/testing/i420_utils.cc:47: memset(video_frame->buffer(webrtc::kYPlane), y, Slightly ignorant comment: Should we really copy allocated_size, not actual size? https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... File third_party/openh264/testing/openh264_testing.gyp (right): https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... third_party/openh264/testing/openh264_testing.gyp:21: 'sources': [ Please canonicalize file order (alphabetically as in gn file). No big deal, but gyp/gn similarity helps maintenance.
Description was changed from ========== Adding third_party/openh264 build files and unittests Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). 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 files 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. (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). *** Will not land this without switching the default value of use_openh264 to 0 and false in GYP and GN, hence COMMIT=false *** COMMIT=false BUG=500605, 468365 ========== to ========== Adding third_party/openh264 build files and unittests Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). 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 files 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. (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). COMMIT=false BUG=500605, 468365 ==========
Please take another look, dpranke and phoglund. https://codereview.chromium.org/1446453004/diff/40001/build/config/features.gni File build/config/features.gni (right): https://codereview.chromium.org/1446453004/diff/40001/build/config/features.g... build/config/features.gni:61: use_openh264 = true On 2015/11/17 23:49:14, Dirk Pranke wrote: > One more comment ... we shouldn't be adding new settings to features.gni unless > they tend to affect targets globally (which this one doesn't). Can you move this > to its own standalone *.gni file and then include it as needed? Done. Did the corresponding thing for gyp. + openh264_args.gni: defaults use_openh264 to is_chrome_branded. + openh264_args.gypi: defaults use_openh264 to 1 if branding=="Chrome", else 0. In both cases you can override the default value (GYP_DEFINES or gn args), regardless of if you're building chrome or chromium. https://codereview.chromium.org/1446453004/diff/40001/third_party/openh264/te... File third_party/openh264/testing/h264_decoder_impl.cc (right): https://codereview.chromium.org/1446453004/diff/40001/third_party/openh264/te... third_party/openh264/testing/h264_decoder_impl.cc:64: // LOG(LS_ERROR) << "Failed to create OpenH264 decoder"; On 2015/11/16 14:55:36, phoglund_chrome wrote: > Nit: remove Done. https://codereview.chromium.org/1446453004/diff/40001/third_party/openh264/te... third_party/openh264/testing/h264_decoder_impl.cc:89: // LOG(LS_ERROR) << "Failed to initialize OpenH264 decoder"; On 2015/11/16 14:55:36, phoglund_chrome wrote: > Nit: remove; apply throughout Done. https://codereview.chromium.org/1446453004/diff/60001/content/browser/media/o... File content/browser/media/openh264_unittests.cc (right): https://codereview.chromium.org/1446453004/diff/60001/content/browser/media/o... content/browser/media/openh264_unittests.cc:23: int shade = 0; On 2015/11/18 14:41:09, torbjorng wrote: > Note that shade here will go from 0 to 225, since the accumulation is truncated > and the loop does 10 instead of 11 iteration. Perhaps use i*255/10 instead and > use i <= kNumSampleFrames (with corresponding sample_frames_ allocation > adjustments). Done. https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... File third_party/openh264/testing/h264_decoder_impl.h (right): https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... third_party/openh264/testing/h264_decoder_impl.h:55: const CodecSpecificInfo* codec_specific_info = nullptr, On 2015/11/18 14:41:09, torbjorng wrote: > Default parameters are discouraged. Furthermore, this one is ignored. I've seen them used the VideoDecoder parent class and some of its child classes. I think it's OK in this case since these parameters are not used, so forcing you to give values makes little sense. https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... File third_party/openh264/testing/h264_encoder_impl.h (right): https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... third_party/openh264/testing/h264_encoder_impl.h:56: int32_t RegisterEncodeCompleteCallback( On 2015/11/18 14:41:09, torbjorng wrote: > Nit: Unneeded linebreak. :-) (Actually it is needed. From "int" to ";" it's exactly 80 chars but there's also 2 spaces before "int") :-) https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... File third_party/openh264/testing/i420_utils.cc (right): https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... third_party/openh264/testing/i420_utils.cc:47: memset(video_frame->buffer(webrtc::kYPlane), y, On 2015/11/18 14:41:09, torbjorng wrote: > Slightly ignorant comment: Should we really copy allocated_size, not actual > size? I must admit basing this code off of https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin.... Looking at allocated_size(), buffer() and VideoFrameBuffer::data() though it looks like this is the correct size to use. https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... File third_party/openh264/testing/openh264_testing.gyp (right): https://codereview.chromium.org/1446453004/diff/60001/third_party/openh264/te... third_party/openh264/testing/openh264_testing.gyp:21: 'sources': [ On 2015/11/18 14:41:09, torbjorng wrote: > Please canonicalize file order (alphabetically as in gn file). No big deal, but > gyp/gn similarity helps maintenance. Done.
lgtm
the content looks roughly okay for the build files, but there are stylistic issues with how you've written the GN files (that should be simple to fix). I'm also confused by the fact that you're putting a test for this in //content/browser . Did some //content/ owner tell you to do that; you'll need review approval from them either way. https://codereview.chromium.org/1446453004/diff/100001/content/browser/media/... File content/browser/media/openh264_unittests.cc (right): https://codereview.chromium.org/1446453004/diff/100001/content/browser/media/... content/browser/media/openh264_unittests.cc:5: #include "testing/gtest/include/gtest/gtest.h" does this actually have anything to do w/ content? I'm a bit confused as to why this is being included in content_unittests. https://codereview.chromium.org/1446453004/diff/100001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/100001/content/test/BUILD.gn#... content/test/BUILD.gn:780: deps += [ "//third_party/openh264/testing:openh264_testing" ] see comment in openh264_unittests; I don't understand why this is part of content_unittests and not a separate binary? https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/B... File third_party/openh264/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/B... third_party/openh264/BUILD.gn:52: static_library("openh264_common") { why is this (and the others) a static_library and not a source_set? https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/B... third_party/openh264/BUILD.gn:71: sources = openh264_common_sources Per https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g... , you should list sources first, deps last, etc. https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/B... third_party/openh264/BUILD.gn:96: "src/codec/processing/src/vaacalc", wow, you really need to list every directory individually rather than include files relative to the top of the repo? :( https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/o... File third_party/openh264/openh264.gni (right): https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/o... third_party/openh264/openh264.gni:1: # Copyright 2015 The Chromium Authors. All rights reserved. Generally if we want to share lists of sources between GYP and GN we'll define them in a *.gypi file (like you've done) and then read the lists in using exec_script("//build/gypi_to_gn.py") (see //content/browser/browser.gni for an example). If you're not going to import the lists using gypi_to_gn, it would be much more orthodox to list the sources directly in the targets in BUILD.gn. Thoughts? Do you have a strong leaning to doing it the way you've done? https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/t... File third_party/openh264/testing/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/t... third_party/openh264/testing/BUILD.gn:8: static_library("openh264_testing") { same comments about source_set vs. static_library and the ordering of the variables in the target.
https://codereview.chromium.org/1446453004/diff/100001/content/browser/media/... File content/browser/media/openh264_unittests.cc (right): https://codereview.chromium.org/1446453004/diff/100001/content/browser/media/... content/browser/media/openh264_unittests.cc:5: #include "testing/gtest/include/gtest/gtest.h" On 2015/11/19 02:46:56, Dirk Pranke wrote: > does this actually have anything to do w/ content? I'm a bit confused as to why > this is being included in content_unittests. Most of the WebRTC code is in content/ and executes in the renderer process, so I thought it made sense. I'm not sure where it would make more sense, maybe media_unittests?
On 2015/11/19 12:39:24, phoglund_chrome wrote: > https://codereview.chromium.org/1446453004/diff/100001/content/browser/media/... > File content/browser/media/openh264_unittests.cc (right): > > https://codereview.chromium.org/1446453004/diff/100001/content/browser/media/... > content/browser/media/openh264_unittests.cc:5: #include > "testing/gtest/include/gtest/gtest.h" > On 2015/11/19 02:46:56, Dirk Pranke wrote: > > does this actually have anything to do w/ content? I'm a bit confused as to > why > > this is being included in content_unittests. > > Most of the WebRTC code is in content/ and executes in the renderer process, so > I thought it made sense. I'm not sure where it would make more sense, maybe > media_unittests? Yes media_unittests is more appropriate. At this level it doesn't have anything to do with content/ and the closest connection point I can think of, is media/video or maybe media/filters.
https://codereview.chromium.org/1446453004/diff/100001/content/browser/media/... File content/browser/media/openh264_unittests.cc (right): https://codereview.chromium.org/1446453004/diff/100001/content/browser/media/... content/browser/media/openh264_unittests.cc:5: #include "testing/gtest/include/gtest/gtest.h" On 2015/11/19 12:39:24, phoglund_chrome wrote: > On 2015/11/19 02:46:56, Dirk Pranke wrote: > > does this actually have anything to do w/ content? I'm a bit confused as to > why > > this is being included in content_unittests. > > Most of the WebRTC code is in content/ and executes in the renderer process, so > I thought it made sense. I'm not sure where it would make more sense, maybe > media_unittests? content_unittests has several WebRTC unittests (so the dependencies/includes are what I need them to be) and it even has a few audio and video tests. I moved it from content/browser/media/ to content/renderer/media/, but still part of content_unittests (Patch Set 6). Based on the comments I'll take a look at media_unittests now instead. https://codereview.chromium.org/1446453004/diff/100001/content/test/BUILD.gn File content/test/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/100001/content/test/BUILD.gn#... content/test/BUILD.gn:780: deps += [ "//third_party/openh264/testing:openh264_testing" ] On 2015/11/19 02:46:56, Dirk Pranke wrote: > see comment in openh264_unittests; I don't understand why this is part of > content_unittests and not a separate binary? There's overhead in creating new test binaries, I've been told that you probably don't want to do that for a single test class and might get pushback. Also, we only want the unittests to be included if you build with use_openh264, meaning producing and for the trybots running the binary would be conditional or it would have to produce a binary that doesn't have any tests if !use_openh264. I created and landed a CL that created a new binary specifically for openh264 unittests but we reverted it in favor of content_unittests. I'll take a look at media_unittests as an alternative. https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/B... File third_party/openh264/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/B... third_party/openh264/BUILD.gn:52: static_library("openh264_common") { On 2015/11/19 02:46:56, Dirk Pranke wrote: > why is this (and the others) a static_library and not a source_set? source_set makes more sense, changing. https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/B... third_party/openh264/BUILD.gn:71: sources = openh264_common_sources On 2015/11/19 02:46:56, Dirk Pranke wrote: > Per > https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/style_g... > , you should list sources first, deps last, etc. Oh! Thanks for the link, updating. https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/B... third_party/openh264/BUILD.gn:96: "src/codec/processing/src/vaacalc", On 2015/11/19 02:46:56, Dirk Pranke wrote: > wow, you really need to list every directory individually rather than include > files relative to the top of the repo? :( Yeah it's silly, they include by filename only. There was even a file with the same name in two different directories so you need to have different include_dirs per target or else you might end up including the wrong file :) https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/o... File third_party/openh264/openh264.gni (right): https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/o... third_party/openh264/openh264.gni:1: # Copyright 2015 The Chromium Authors. All rights reserved. On 2015/11/19 02:46:56, Dirk Pranke wrote: > Generally if we want to share lists of sources between GYP and GN we'll define > them in a *.gypi file (like you've done) and then read the lists in using > exec_script("//build/gypi_to_gn.py") (see //content/browser/browser.gni for an > example). > > If you're not going to import the lists using gypi_to_gn, it would be much more > orthodox to list the sources directly in the targets in BUILD.gn. > > Thoughts? Do you have a strong leaning to doing it the way you've done? Good idea. A shared .gypi file makes sense, then you don't have two identical-except-for-syntax files to maintain. I can do the same with the include directory lists. https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/t... File third_party/openh264/testing/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/100001/third_party/openh264/t... third_party/openh264/testing/BUILD.gn:8: static_library("openh264_testing") { On 2015/11/19 02:46:56, Dirk Pranke wrote: > same comments about source_set vs. static_library and the ordering of the > variables in the target. Done.
Description was changed from ========== Adding third_party/openh264 build files and unittests Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). 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 files 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. (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). COMMIT=false BUG=500605, 468365 ========== to ========== Adding third_party/openh264 build files and unittests Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). 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 files 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. (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). --- use_openh264 must not be unconditionally true when landing, hence the COMMIT=false --- COMMIT=false BUG=500605, 468365 ==========
On second thought, are we sure the test should be in media/? media/ does not properly depend on WebRTC. It compiled anyway on linux and I uploaded it after updating DEPS, but it does not compile on windows (WEBRTC_WIN macro not being defined causing trouble) and android (linker errors relating to logging). I spent an hour or two trying to make it properly depend on WebRTC to get it to compile before talking to grunell@ and he questioned trying to get media/ to depend on WebRTC and getting owners to agree on that. I could either... 1) Continue with media_unittests, making it properly depend on WebRTC. 2) Move it back to content_unittests (content/renderer/media/) where other WebRTC tests exist. 3) Find some other binary where we can run WebRTC unittests inside of Chromium. 4) Create a new test binary for OpenH264 testing, even though it only runs a unittest and only does so conditionally if a flag is set. Thoughts, reviewers? (Patch Set 6 does 2).)
On 2015/11/19 16:46:04, hbos_chromium wrote: > On second thought, are we sure the test should be in media/? > > media/ does not properly depend on WebRTC. It compiled anyway on linux and I > uploaded it after updating DEPS, but it does not compile on windows (WEBRTC_WIN > macro not being defined causing trouble) and android (linker errors relating to > logging). I spent an hour or two trying to make it properly depend on WebRTC to > get it to compile before talking to grunell@ and he questioned trying to get > media/ to depend on WebRTC and getting owners to agree on that. > > I could either... > 1) Continue with media_unittests, making it properly depend on WebRTC. > 2) Move it back to content_unittests (content/renderer/media/) where other > WebRTC tests exist. > 3) Find some other binary where we can run WebRTC unittests inside of Chromium. > 4) Create a new test binary for OpenH264 testing, even though it only runs a > unittest and only does so conditionally if a flag is set. > > Thoughts, reviewers? (Patch Set 6 does 2).) I would either do 1), 2), or 4). Logically, I would prefer 4) because it looks like there are no actual dependencies on either media or content. The main reason we push back on new test binaries is when they have a lot of dependencies and would take forever to link (e.g., creating a new test binary for just some files in //chrome), but I don't think that's the case here. Adding a new binary to the bots is a bit annoying, but much better than it used to be (can probably be done completely w/ changes to src-side files at this point), and I don't think the conditionality matters much since we're probably not going to be flipping this flag back and forth on the bots. However, if you really don't want to create a new binary, just get buy-in from the appropriate owner and I'm fine w/ whatever.
Patchset #8 (id:160001) has been deleted
What about having these tests in the webrtc repo since they use webrtc classes and it seems that the purpose is to ensure webrtc compatibility. Then, we do an integration test in Chromium that uses openh264 via webrtc. Does that make sense? On Thu, Nov 19, 2015 at 5:50 PM <dpranke@chromium.org> wrote: > On 2015/11/19 16:46:04, hbos_chromium wrote: > > On second thought, are we sure the test should be in media/? > > > media/ does not properly depend on WebRTC. It compiled anyway on linux > > and I > > uploaded it after updating DEPS, but it does not compile on windows > (WEBRTC_WIN > > macro not being defined causing trouble) and android (linker errors > > relating > to > > logging). I spent an hour or two trying to make it properly depend on > > WebRTC > to > > get it to compile before talking to grunell@ and he questioned trying to > > get > > media/ to depend on WebRTC and getting owners to agree on that. > > > I could either... > > 1) Continue with media_unittests, making it properly depend on WebRTC. > > 2) Move it back to content_unittests (content/renderer/media/) where > other > > WebRTC tests exist. > > 3) Find some other binary where we can run WebRTC unittests inside of > Chromium. > > 4) Create a new test binary for OpenH264 testing, even though it only > > runs a > > unittest and only does so conditionally if a flag is set. > > > Thoughts, reviewers? (Patch Set 6 does 2).) > > I would either do 1), 2), or 4). > > Logically, I would prefer 4) because it looks like there are no actual > dependencies on > either media or content. > > The main reason we push back on new test binaries is when > they have a lot of dependencies and would take forever to link (e.g., > creating a > > new test binary for just some files in //chrome), but I don't think that's > the > case > here. > > Adding a new binary to the bots is a bit annoying, but much better than it > used to be (can probably be done completely w/ changes to src-side files at > this > > point), and I don't think the conditionality matters much since we're > probably > not > going to be flipping this flag back and forth on the bots. > > However, if you really don't want to create a new binary, just get buy-in > from > the appropriate owner and I'm fine w/ whatever. > > https://codereview.chromium.org/1446453004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/11/20 11:33:08, tommi wrote: > What about having these tests in the webrtc repo since they use webrtc > classes and it seems that the purpose is to ensure webrtc compatibility. > Then, we do an integration test in Chromium that uses openh264 via webrtc. > Does that make sense? TL;DR: Yes we should have webrtc encoder/decoder tests in webrtc and yes we should have integration tests in chromium. But I want to have these enc+dec tests in chromium for now to make sure the build files are correct (on all platforms) *before* landing the CL. Assuming third_party/openh264 is not broken webrtc should be able to just pull that in and test its own encoder/decoder implementations, and I agree this is where most of the test code naturally belongs. (The webrtc encoder/decoder CL provides both the implementation and test code.) But in order to make sure my build files are correct on all platforms/trybots I need tests in the chromium repo as well or else I have to wait for waterfall/rolling to find out if I broke third_party/openh264 or not. Now I could test the OpenH264 encoder/decoder functions directly, without a dependency on the webrtc repo, but I choose to re-use the webrtc encoder/decoder implementations due to less code repetition, cleaner test code and, arguably, since there is a dependency between webrtc and the build files in chromium anyway I might as well test those implementations. But it does introduce a funny dependency (which is why this CL attempts to land temporary copies of the webrtc encoder/decoder that will be replaced by the ones in the webrtc repo as soon as that CL can land). A proper integration test in chromium would achieve the same result: openh264 encoding/decoding being tested *in the chromium repo*. But I need to land the encoder/decoder before trying to integrate it with chrome. openh264_unittests is enough for now: making sure enc+dec produces roughly the same result as the input frames. Thoughts?
Please take another look dpranke. It sounds like creating a new test binary is not as big of a deal as I thought, so I did that.
lgtm w/ comments. https://codereview.chromium.org/1446453004/diff/200001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/200001/BUILD.gn#newcode96 BUILD.gn:96: "//third_party/openh264/testing:openh264_unittests", Are you sure you want to add this unconditionally, rather than just if use_openh264 is true? In the non-use_openh264 case, it'll be an empty binary, admittedly, but that still adds a bit of overhead. It's fine either way, just double-checking. https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... File third_party/openh264/testing/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... third_party/openh264/testing/BUILD.gn:9: test("openh264_unittests") { If you want this to be run on the chromium bots, it should probably depend on //base/test:run_all_unittests as well as gtest. This adds some scaffolding that the interface to the bots needs (reporting test failures in a particular format, support swarming, etc.). You will also need to actually add it to the list of tests to run on each bot, which are more-or-less in //testing/buildbot/*.json. Let me know if you need help with that. If you don't want to run it on the bots (just build it), gtest-only is fine.
Landing as-is then, making bots run the test as a follow-up CL. https://codereview.chromium.org/1446453004/diff/200001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/200001/BUILD.gn#newcode96 BUILD.gn:96: "//third_party/openh264/testing:openh264_unittests", On 2015/11/21 00:48:51, Dirk Pranke wrote: > Are you sure you want to add this unconditionally, rather than just if > use_openh264 is true? > > In the non-use_openh264 case, it'll be an empty binary, admittedly, but that > still adds a bit of overhead. > > It's fine either way, just double-checking. If we can make trybots conditionally run the binary then we don't need to produce it unconditionally. For now I'll produce a binary either way to avoid the risk of trybots running an old binary that shouldn't run. https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... File third_party/openh264/testing/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... third_party/openh264/testing/BUILD.gn:9: test("openh264_unittests") { On 2015/11/21 00:48:51, Dirk Pranke wrote: > If you want this to be run on the chromium bots, it should probably depend on > //base/test:run_all_unittests as well as gtest. > > This adds some scaffolding that the interface to the bots needs (reporting test > failures in a particular format, support swarming, etc.). > > You will also need to actually add it to the list of tests to run on each bot, > which are more-or-less in //testing/buildbot/*.json. Let me know if you need > help with that. > > If you don't want to run it on the bots (just build it), gtest-only is fine. OK, good to know. I'll do the bot stuff in a follow-up CL.
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torbjorng@chromium.org, phoglund@chromium.org Link to the patchset: https://codereview.chromium.org/1446453004/#ps200001 (title: "use_openh264 = conditional on chrome branding again")
The CQ bit was unchecked by commit-bot@chromium.org
COMMIT=false detected. CQ is abandoning the patch.
Description was changed from ========== Adding third_party/openh264 build files and unittests Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). 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 files 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. (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). --- use_openh264 must not be unconditionally true when landing, hence the COMMIT=false --- COMMIT=false BUG=500605, 468365 ========== to ========== Adding third_party/openh264 build files and unittests. Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). The unittest is a simple test to make sure encoding and decoding produces roughly the original frame. This is a new test binary, openh264_unittests. Configuring the chromium bots to run it will be done in a follow-up 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 files 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. (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). BUG=500605, 468365 ==========
The CQ bit was checked by hbos@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1446453004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1446453004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
hbos@chromium.org changed reviewers: + brettw@chromium.org
I need an owner review for .gn. brettw, can you take a look at that file specifically? It's a one line change to allow third_party/openh264/BUILD.gn to use exec_script. I only need this for the sake of gypi_to_gn.py (reading in list of source files that are shared between gyp and gn build files). I also fail presubmit due to DEPS include_rules adding +openh264 and +webrtc. tommi, I think the presubmit is being weird again. Why would I need lgtm for +openh264 when I am the owner of openh264? I also need lgtm for +webrtc, but third_party/webrtc only has per-file OWNERS of build files (and I include "webrtc/video_frame.h"), wouldn't that mean it looks at third_party/ OWNERS?
On 2015/11/23 09:50:29, hbos_chromium wrote: > I need an owner review for .gn. brettw, can you take a look at that file > specifically? > It's a one line change to allow third_party/openh264/BUILD.gn to use > exec_script. I only need this for the sake of gypi_to_gn.py (reading in list of > source files that are shared between gyp and gn build files). > > I also fail presubmit due to DEPS include_rules adding +openh264 and +webrtc. > > tommi, I think the presubmit is being weird again. Why would I need lgtm for > +openh264 when I am the owner of openh264? I also need lgtm for +webrtc, but > third_party/webrtc only has per-file OWNERS of build files (and I include > "webrtc/video_frame.h"), wouldn't that mean it looks at third_party/ OWNERS? Yes, looks like bug crbug.com/545323 again.
On 2015/11/23 12:16:42, tommi wrote: > On 2015/11/23 09:50:29, hbos_chromium wrote: > > I need an owner review for .gn. brettw, can you take a look at that file > > specifically? > > It's a one line change to allow third_party/openh264/BUILD.gn to use > > exec_script. I only need this for the sake of gypi_to_gn.py (reading in list > of > > source files that are shared between gyp and gn build files). > > > > I also fail presubmit due to DEPS include_rules adding +openh264 and +webrtc. > > > > tommi, I think the presubmit is being weird again. Why would I need lgtm for > > +openh264 when I am the owner of openh264? I also need lgtm for +webrtc, but > > third_party/webrtc only has per-file OWNERS of build files (and I include > > "webrtc/video_frame.h"), wouldn't that mean it looks at third_party/ OWNERS? > > Yes, looks like bug crbug.com/545323 again. Then I'll land with NOPRESUBMIT=true when I have enough lgtms.
I would TBR brett for the .gn change. I'm not sure the DEPS change is right; normally we specify paths as subdirectories of src/ , e.g. "third_party/webrtc", (in which case I would think the presubmit owners check would work), but perhaps that won't work in this case? (right now I think the presubmit check is looking for owner approval for //webrtc and //openh264 , neither of which exists). https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/DEPS File third_party/openh264/DEPS (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/D... third_party/openh264/DEPS:4: '+webrtc', what directories are you trying to include here (i.e., what are the full paths)?
I'm planning on looking at this. Before landing anything else, I want to be sure we also follow up with my request on https://codereview.chromium.org/1394983003/ where it looks like no third_party owner reviewed the initial landing. I want to be sure all the necessary security and licensing reviews happened.
https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/B... File third_party/openh264/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/B... third_party/openh264/BUILD.gn:29: defines += [ "ANDROID_NDK" ] Can you comment why this is necessary? https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/B... third_party/openh264/BUILD.gn:40: defines += [ "VERSION_NUMBER=\"openh264 v.?.?\"" ] This seems pretty fishy. If your include directories are so wrong that you're getting stuff pulled in in the wrong order, you're going to have more problems down the road and wallpapering over it with all of this hacky build code won't help. Did you ever see the problem on a GN bot? Your includes look correct, your custom directories will be included before any other system ones. In GYP you're doing the reverse which I suspect is what's causing your issue. I think you want 'include_dirs+': [ ... ] as discussed in https://gyp.gsrc.io/docs/InputFormatReference.md https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/B... third_party/openh264/BUILD.gn:56: source_set("openh264_common") { These names should not duplicate the directory name, so just use "common", "processing", "encoder", and "decoder" https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... File third_party/openh264/testing/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... third_party/openh264/testing/BUILD.gn:23: "//testing/gtest:gtest", This should just be "//testing/gtest" https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... third_party/openh264/testing/BUILD.gn:26: "//third_party/webrtc/common_video:common_video", This should just be ""//third_party/webrtc/common_video" https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... third_party/openh264/testing/BUILD.gn:37: "//testing/gtest:gtest", Same as above.
Description was changed from ========== Adding third_party/openh264 build files and unittests. Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). The unittest is a simple test to make sure encoding and decoding produces roughly the original frame. This is a new test binary, openh264_unittests. Configuring the chromium bots to run it will be done in a follow-up 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 files 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. (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). BUG=500605, 468365 ========== to ========== Adding third_party/openh264 build files and unittests. Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). The unittest is a simple test to make sure encoding and decoding produces roughly the original frame. This is a new test binary, openh264_unittests. Configuring the chromium bots to run it will be done in a follow-up 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 files 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. (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). COMMIT=false BUG=500605, 468365 ==========
https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/B... File third_party/openh264/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/B... third_party/openh264/BUILD.gn:29: defines += [ "ANDROID_NDK" ] On 2015/11/24 00:05:19, brettw wrote: > Can you comment why this is necessary? Done. https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/B... third_party/openh264/BUILD.gn:40: defines += [ "VERSION_NUMBER=\"openh264 v.?.?\"" ] On 2015/11/24 00:05:19, brettw wrote: > This seems pretty fishy. If your include directories are so wrong that you're > getting stuff pulled in in the wrong order, you're going to have more problems > down the road and wallpapering over it with all of this hacky build code won't > help. > > Did you ever see the problem on a GN bot? Your includes look correct, your > custom directories will be included before any other system ones. In GYP you're > doing the reverse which I suspect is what's causing your issue. I think you want > 'include_dirs+': [ ... ] as discussed in > https://gyp.gsrc.io/docs/InputFormatReference.md Can't remember if I had this problem on GN or not. And I didn't know about +! I'll see if it can be avoided by running another patch set on the bots. https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/B... third_party/openh264/BUILD.gn:56: source_set("openh264_common") { On 2015/11/24 00:05:19, brettw wrote: > These names should not duplicate the directory name, so just use "common", > "processing", "encoder", and "decoder" Are you sure? They're the names used when you ninja. "ninja -C out/gnbuild common" for example is a lot more generic and prone to name clash than "openh264_common". Unless you're saying I'm exposing these somehow that I shouldn't and you shouldn't be able to build "common" without specifying a full path? https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/DEPS File third_party/openh264/DEPS (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/D... third_party/openh264/DEPS:4: '+webrtc', On 2015/11/23 23:07:07, Dirk Pranke wrote: > what directories are you trying to include here (i.e., what are the full paths)? third_party/openh264 testing third_party/webrtc webrtc headers do #include "webrtc/...", not #include "third_party/webrtc". If I change the DEPS file to +third_party/webrtc then presubmit fails. I did the same with third_party/openh264/testing files, #include "openh264/..." instead of #include "third_party/webrtc/..." (from the third party files' "perspective" they're not "third party" to themselves I'm thinking). https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... File third_party/openh264/testing/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... third_party/openh264/testing/BUILD.gn:23: "//testing/gtest:gtest", On 2015/11/24 00:05:19, brettw wrote: > This should just be "//testing/gtest" Done. https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... third_party/openh264/testing/BUILD.gn:26: "//third_party/webrtc/common_video:common_video", On 2015/11/24 00:05:19, brettw wrote: > This should just be ""//third_party/webrtc/common_video" OK done, but :openh264_encoder and :openh264_decoder is fine? When you don't include the specific target name does it automatically look for a target with the same name as the folder? https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... third_party/openh264/testing/BUILD.gn:37: "//testing/gtest:gtest", On 2015/11/24 00:05:19, brettw wrote: > Same as above. Done.
PTAL brettw. (Will switch use_openh264 back to being conditional on chrome branding before landing) So the reviews and security reviews are in progress. Assuming I get all the sufficient lgtms, are you OK with me landing this as to not block other work (even though the review is still in progress)? It's all behind a flag anyway. https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/B... File third_party/openh264/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/B... third_party/openh264/BUILD.gn:40: defines += [ "VERSION_NUMBER=\"openh264 v.?.?\"" ] On 2015/11/25 16:17:02, hbos_chromium wrote: > On 2015/11/24 00:05:19, brettw wrote: > > This seems pretty fishy. If your include directories are so wrong that you're > > getting stuff pulled in in the wrong order, you're going to have more problems > > down the road and wallpapering over it with all of this hacky build code won't > > help. > > > > Did you ever see the problem on a GN bot? Your includes look correct, your > > custom directories will be included before any other system ones. In GYP > you're > > doing the reverse which I suspect is what's causing your issue. I think you > want > > 'include_dirs+': [ ... ] as discussed in > > https://gyp.gsrc.io/docs/InputFormatReference.md > > Can't remember if I had this problem on GN or not. And I didn't know about +! > I'll see if it can be avoided by running another patch set on the bots. Oh wow, thanks a million, I got rid of it with your tip!
With the changes below made and the in-progress reviews completed, this LGTM https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/B... File third_party/openh264/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/B... third_party/openh264/BUILD.gn:56: source_set("openh264_common") { On 2015/11/25 16:17:02, hbos_chromium wrote: > On 2015/11/24 00:05:19, brettw wrote: > > These names should not duplicate the directory name, so just use "common", > > "processing", "encoder", and "decoder" > > Are you sure? They're the names used when you ninja. "ninja -C out/gnbuild > common" Yes. GN style is to not duplicate names like this. Source sets and static libraries can have colliding short names (there are 127 targets with the name "test_support" for example). In GN you can build a colliding name by supplying its label minus the "//" ninja -C out/foo third_party/openh264:common The GYP build requires globally unique names for everything so keep your current names there. https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... File third_party/openh264/testing/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... third_party/openh264/testing/BUILD.gn:26: "//third_party/webrtc/common_video:common_video", On 2015/11/25 16:17:02, hbos_chromium wrote: > On 2015/11/24 00:05:19, brettw wrote: > > This should just be ""//third_party/webrtc/common_video" > > OK done, but :openh264_encoder and :openh264_decoder is fine? When you don't > include the specific target name does it automatically look for a target with > the same name as the folder? Yes, if you omit the colon the last component is implicitly duplicated. https://codereview.chromium.org/1446453004/diff/220001/third_party/openh264/B... File third_party/openh264/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/220001/third_party/openh264/B... third_party/openh264/BUILD.gn:7: if (use_openh264) { As-is, this is unnecessary because this file will only be loaded when people refer to targets in it, and when this isn't defined, there are no targets. You can remove this and replace it with: assert(use_openh264) to catch some common mistakes. https://codereview.chromium.org/1446453004/diff/220001/third_party/openh264/o... File third_party/openh264/openh264_args.gni (right): https://codereview.chromium.org/1446453004/diff/220001/third_party/openh264/o... third_party/openh264/openh264_args.gni:13: use_openh264 = true # is_chrome_branded I think you meant to delete the "true #" If you want to keep this as-is, remove the import at the top and comment with a TODO or something if it should be changed in the future.
https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/DEPS File third_party/openh264/DEPS (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/D... third_party/openh264/DEPS:4: '+webrtc', On 2015/11/25 16:17:02, hbos_chromium wrote: > On 2015/11/23 23:07:07, Dirk Pranke wrote: > > what directories are you trying to include here (i.e., what are the full > paths)? > > third_party/openh264 > testing > third_party/webrtc > > webrtc headers do #include "webrtc/...", not #include "third_party/webrtc". > If I change the DEPS file to +third_party/webrtc then presubmit fails. > > I did the same with third_party/openh264/testing files, #include "openh264/..." > instead of #include "third_party/webrtc/..." (from the third party files' > "perspective" they're not "third party" to themselves I'm thinking). I thought the presubmit was failing because it was 'webrtc' instead of 'third_party/webrtc'. I'm not sure why it would fail if you made that change, or why it's passing now, unless brett's l-g-t-m made the check not care about non-existent directories. As long as the CQ is happy, I'm fine w/ whatever works in this regard.
https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/B... File third_party/openh264/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/B... third_party/openh264/BUILD.gn:56: source_set("openh264_common") { On 2015/11/25 19:26:34, brettw wrote: > On 2015/11/25 16:17:02, hbos_chromium wrote: > > On 2015/11/24 00:05:19, brettw wrote: > > > These names should not duplicate the directory name, so just use "common", > > > "processing", "encoder", and "decoder" > > > > Are you sure? They're the names used when you ninja. "ninja -C out/gnbuild > > common" > > Yes. GN style is to not duplicate names like this. Source sets and static > libraries can have colliding short names (there are 127 targets with the name > "test_support" for example). In GN you can build a colliding name by supplying > its label minus the "//" > ninja -C out/foo third_party/openh264:common > > The GYP build requires globally unique names for everything so keep your current > names there. Done. https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/DEPS File third_party/openh264/DEPS (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/D... third_party/openh264/DEPS:4: '+webrtc', On 2015/11/25 20:58:58, Dirk Pranke wrote: > On 2015/11/25 16:17:02, hbos_chromium wrote: > > On 2015/11/23 23:07:07, Dirk Pranke wrote: > > > what directories are you trying to include here (i.e., what are the full > > paths)? > > > > third_party/openh264 > > testing > > third_party/webrtc > > > > webrtc headers do #include "webrtc/...", not #include "third_party/webrtc". > > If I change the DEPS file to +third_party/webrtc then presubmit fails. > > > > I did the same with third_party/openh264/testing files, #include > "openh264/..." > > instead of #include "third_party/webrtc/..." (from the third party files' > > "perspective" they're not "third party" to themselves I'm thinking). > > I thought the presubmit was failing because it was 'webrtc' instead of > 'third_party/webrtc'. I'm not sure why it would fail if you made that change, > or why it's passing now, unless brett's l-g-t-m made the check not > care about non-existent directories. > > As long as the CQ is happy, I'm fine w/ whatever works in this regard. > Yeah not sure, but I think the presubmit looks at the #include strings which starts with "webrtc/" so that's what you have to put in include_rules, whereas the OWNERS check interprets this as src/webrtc and not src/third_party/webrtc/, so then there are no valid owners to get lgtm from. I'll have to land this with NOPRESUBMIT=true I think... https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... File third_party/openh264/testing/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/200001/third_party/openh264/t... third_party/openh264/testing/BUILD.gn:26: "//third_party/webrtc/common_video:common_video", On 2015/11/25 19:26:34, brettw wrote: > On 2015/11/25 16:17:02, hbos_chromium wrote: > > On 2015/11/24 00:05:19, brettw wrote: > > > This should just be ""//third_party/webrtc/common_video" > > > > OK done, but :openh264_encoder and :openh264_decoder is fine? When you don't > > include the specific target name does it automatically look for a target with > > the same name as the folder? > > Yes, if you omit the colon the last component is implicitly duplicated. Acknowledged. https://codereview.chromium.org/1446453004/diff/220001/third_party/openh264/B... File third_party/openh264/BUILD.gn (right): https://codereview.chromium.org/1446453004/diff/220001/third_party/openh264/B... third_party/openh264/BUILD.gn:7: if (use_openh264) { On 2015/11/25 19:26:34, brettw wrote: > As-is, this is unnecessary because this file will only be loaded when people > refer to targets in it, and when this isn't defined, there are no targets. > > You can remove this and replace it with: > assert(use_openh264) > to catch some common mistakes. Done. https://codereview.chromium.org/1446453004/diff/220001/third_party/openh264/o... File third_party/openh264/openh264_args.gni (right): https://codereview.chromium.org/1446453004/diff/220001/third_party/openh264/o... third_party/openh264/openh264_args.gni:13: use_openh264 = true # is_chrome_branded On 2015/11/25 19:26:34, brettw wrote: > I think you meant to delete the "true #" > > If you want to keep this as-is, remove the import at the top and comment with a > TODO or something if it should be changed in the future. Nah this is just to force the non-chrome chromium bots to compile it when I upload a patch set, I'll change it back to is_chrome_branded before landing (same with gyp file). I've added "COMMIT=false" to the CL description to make it impossible to accidentally land it before then.
Description was changed from ========== Adding third_party/openh264 build files and unittests. Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). The unittest is a simple test to make sure encoding and decoding produces roughly the original frame. This is a new test binary, openh264_unittests. Configuring the chromium bots to run it will be done in a follow-up 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 files 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. (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). COMMIT=false BUG=500605, 468365 ========== to ========== Adding third_party/openh264 build files and unittests. Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). COMMIT=false BUG=500605, 468365 ==========
Description was changed from ========== Adding third_party/openh264 build files and unittests. Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). COMMIT=false BUG=500605, 468365 ========== to ========== Adding third_party/openh264 build files. Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). COMMIT=false BUG=500605, 468365 ==========
brettw, dpranke: Are you OK with me landing this while the third party/security review is still in progress, or do I have to wait for that to finish? (Behind a flag and not linked as part of chrome yet.) This would unblock some parallel work. Lastly, I removed the test code. Q: Why? A: The test code that I had added was primarily meant to test the build files. It was not to test the library code, rather I was afraid that if I had done something subtly wrong with the build files like not define a macro or use the wrong assembly code on the wrong architecture, the code *might* compile successfully and then crash at runtime. After discussions with my colleagues I'm convinced that such mistakes in the build files would likely simply result in the compilation being unsuccessful and not need runtime testing. Since the webrtc encoder/decoder implementations will be tested in the webrtc repo anyway, this additional unittest in the chromium repo would be superfluous.
On 2015/11/27 10:46:10, hbos_chromium wrote: > brettw, dpranke: Are you OK with me landing this while the third party/security > review is still in progress, or do I have to wait for that to finish? (Behind a > flag and not linked as part of chrome yet.) This would unblock some parallel > work. I'm not actually sure if we need to wait or not. I can understand the desire to get the CL landed even if it's not used by chrome or enabled yet. Let me ping the security folks tomorrow if we haven't heard an answer before then.
I'm OK landing this and then iterating and finishing the security reviews, but I wouldn't want to speak for the security folks here. Best to just ask, we should bug them if they're not responding.
On 2015/11/30 17:53:16, brettw wrote: > I'm OK landing this and then iterating and finishing the security reviews, but I > wouldn't want to speak for the security folks here. Best to just ask, we should > bug them if they're not responding. I asked jschuh@ (now CC'ed) and he said that we should *not* land before the security folks are engaged. I believe you have already sent a note to security@,so hopefully you will have heard from them by this afternoon, but if you haven't, let me know and I'll bug them again.
PTAL. I have got eng and security review approval to proceed for now with openh264 for encoding but not for decoding. I removed the decoder from the CL. If nobody objects I'll land this on monday.
On 2015/12/04 10:14:12, hbos_chromium wrote: > PTAL. > > I have got eng and security review approval to proceed for now with openh264 for > encoding but not for decoding. > I removed the decoder from the CL. If nobody objects I'll land this on monday. (Note: This will not be linked into chrome until we've discussed binary size etc, but it's OK for me to start landing CLs.)
Sounds good. Still LGTM then.
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, phoglund@chromium.org, torbjorng@chromium.org Link to the patchset: https://codereview.chromium.org/1446453004/#ps340001 (title: "Removed decoder from build files")
The CQ bit was unchecked by commit-bot@chromium.org
COMMIT=false detected. CQ is abandoning the patch.
Description was changed from ========== Adding third_party/openh264 build files. Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up in two pieces and landed the first part). COMMIT=false BUG=500605, 468365 ========== to ========== Adding third_party/openh264 build files for encoding (but not decoding). Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up into pieces and removed decoding). BUG=500605, 468365 ==========
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, phoglund@chromium.org, brettw@chromium.org, torbjorng@chromium.org Link to the patchset: https://codereview.chromium.org/1446453004/#ps360001 (title: "Rebase /w master and corrected comment about build flag")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1446453004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1446453004/360001
Description was changed from ========== Adding third_party/openh264 build files for encoding (but not decoding). Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up into pieces and removed decoding). BUG=500605, 468365 ========== to ========== Adding third_party/openh264 build files for encoding (but not decoding). Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up into pieces and removed decoding and tests). BUG=500605, 468365 ==========
Message was sent while issue was closed.
Description was changed from ========== Adding third_party/openh264 build files for encoding (but not decoding). Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up into pieces and removed decoding and tests). BUG=500605, 468365 ========== to ========== Adding third_party/openh264 build files for encoding (but not decoding). Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up into pieces and removed decoding and tests). BUG=500605, 468365 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Adding third_party/openh264 build files for encoding (but not decoding). Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up into pieces and removed decoding and tests). BUG=500605, 468365 ========== to ========== Adding third_party/openh264 build files for encoding (but not decoding). Adds GYP and GN build files. Only building C++ code. Architecture-specific assembly code will be added in follow-up CL(s). (This is a reupload of https://codereview.chromium.org/1403893007/ after having split it up into pieces and removed decoding and tests). BUG=500605, 468365 Committed: https://crrev.com/17a7b4a2959f7b65f764fffcdb7b39e276f040b7 Cr-Commit-Position: refs/heads/master@{#363443} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/17a7b4a2959f7b65f764fffcdb7b39e276f040b7 Cr-Commit-Position: refs/heads/master@{#363443} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
