Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1693803002:
Make it possible to exclude device management code from rtc_media target. (Closed)
|
Created:
4 years, 10 months ago by kjellander_webrtc Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMake it possible to exclude device management code from rtc_media target.
Chromium doesn't use the device managment code in webrtc/media
so we need a way to turn it off in order to eliminate Chromium's
src/third_party/libjingle/libjingle.gyp
BUG=webrtc:4256
NOTRY=True
TESTED=Trybots + successfully compiled with
GYP_DEFINES=include_internal_device_management=0 webrtc/build/gyp_webrtc
ninja -C out/Debug rtc_media
Committed: https://crrev.com/7d9112cbc4a980c505572012dd9253e14f0d8ad4
Cr-Commit-Position: refs/heads/master@{#11816}
Patch Set 1 #Patch Set 2 : Move libudev include and remove redundant winmm.lib entry #
Total comments: 7
Patch Set 3 : Flattended conditions hierarchy, added Android and iOS and updated comment #
Messages
Total messages: 18 (8 generated)
Description was changed from ========== Make it possible to exclude device management code from rtc_media target. Chromium doesn't use the device managment code in webrtc/media so we need a way to turn it off in order to eliminate Chromium's src/third_party/libjingle/libjingle.gyp BUG=webrtc:4256 NOTRY=True ========== to ========== Make it possible to exclude device management code from rtc_media target. Chromium doesn't use the device managment code in webrtc/media so we need a way to turn it off in order to eliminate Chromium's src/third_party/libjingle/libjingle.gyp BUG=webrtc:4256 NOTRY=True TESTED=Trybots + successfully compiled with GYP_DEFINES=include_internal_device_management=0 webrtc/build/gyp_webrtc ninja -C out/Debug rtc_media ==========
kjellander@webrtc.org changed reviewers: + perkj@webrtc.org
lgtm
I wasn't quite done with this yet (didn't publish comments yet). I'll send another message when I'm done (sorry I shouldn't have put you as reviewer yet).
Patchset #2 (id:20001) has been deleted
PTAL regarding library questions. https://codereview.webrtc.org/1693803002/diff/40001/webrtc/media/media.gyp File webrtc/media/media.gyp (right): https://codereview.webrtc.org/1693803002/diff/40001/webrtc/media/media.gyp#ne... webrtc/media/media.gyp:195: 'libraries': [ It seems this is not needed since compilation or rtc_media_unittests passes with it removed. Is it safe to remove it in that case or could it be that it's only used by classes that are not referenced by the test binary, so potentially else may fail? I don't know which sources are referencing the library (or how to find out). https://codereview.webrtc.org/1693803002/diff/40001/webrtc/media/media.gyp#ne... webrtc/media/media.gyp:207: 'd3d9.lib', Same goes with these, rtc_media_unittests compiles with all of them removed. Does that mean they're safe to remove or is it better to just keep them here anyway?
kjellander@webrtc.org changed reviewers: + tommi@webrtc.org
+tommi as well. https://codereview.webrtc.org/1693803002/diff/40001/webrtc/media/media.gyp File webrtc/media/media.gyp (right): https://codereview.webrtc.org/1693803002/diff/40001/webrtc/media/media.gyp#ne... webrtc/media/media.gyp:82: 'engine/simulcast.cc', Should we split out simulcast.cc and friends into a 'libpeerconnection' static library so it becomes similar to https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... ?
Some more thoughts: what about some of the defines set in libjingle.gyp that I cannot find any references to in own code, do they need to be set in the GYP targets in WebRTC, or is it sufficient they're set in Chromium's libjingle.gyp while we transition? Examples: _CRT_SECURE_NO_WARNINGS at https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... _USE_32BIT_TIME_T NO_SOUND_SYSTEM USE_WEBRTC_DEV_BRANCH
PTAL since I changed a bit of things, and still have a question in my comments. I looked into wiring up all the defines in Chromium's third_party/libjingle/libjingle.gyp but that quickly grows into a complex CL so let's keep this one focused one thing. https://codereview.webrtc.org/1693803002/diff/40001/webrtc/media/media.gyp File webrtc/media/media.gyp (right): https://codereview.webrtc.org/1693803002/diff/40001/webrtc/media/media.gyp#ne... webrtc/media/media.gyp:82: 'engine/simulcast.cc', On 2016/02/15 16:45:45, kjellander (webrtc) wrote: > Should we split out simulcast.cc and friends into a 'libpeerconnection' static > library so it becomes similar to > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin... > ? I'll do this, but in a separate CL. https://codereview.webrtc.org/1693803002/diff/40001/webrtc/media/media.gyp#ne... webrtc/media/media.gyp:195: 'libraries': [ On 2016/02/15 16:38:40, kjellander (webrtc) wrote: > It seems this is not needed since compilation or rtc_media_unittests passes with > it removed. Is it safe to remove it in that case or could it be that it's only > used by classes that are not referenced by the test binary, so potentially else > may fail? I don't know which sources are referencing the library (or how to find > out). Anyone know the answer to this?
lgtm https://codereview.webrtc.org/1693803002/diff/40001/webrtc/media/media.gyp File webrtc/media/media.gyp (right): https://codereview.webrtc.org/1693803002/diff/40001/webrtc/media/media.gyp#ne... webrtc/media/media.gyp:195: 'libraries': [ On 2016/02/29 09:18:59, kjellander (webrtc) wrote: > On 2016/02/15 16:38:40, kjellander (webrtc) wrote: > > It seems this is not needed since compilation or rtc_media_unittests passes > with > > it removed. Is it safe to remove it in that case or could it be that it's only > > used by classes that are not referenced by the test binary, so potentially > else > > may fail? I don't know which sources are referencing the library (or how to > find > > out). > > Anyone know the answer to this? I don't. Could we try removing it in a separate CL and see if things stay green? https://codereview.webrtc.org/1693803002/diff/40001/webrtc/media/media.gyp#ne... webrtc/media/media.gyp:207: 'd3d9.lib', On 2016/02/15 16:38:40, kjellander (webrtc) wrote: > Same goes with these, rtc_media_unittests compiles with all of them removed. > Does that mean they're safe to remove or is it better to just keep them here > anyway? Could be that there are other binaries that depend on it. However, if things compile, it might be OK to remove. My guess would be that some integration tests on Windows might break due to missing IIDs.
The CQ bit was checked by kjellander@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/1693803002/#ps60001 (title: "Flattended conditions hierarchy, added Android and iOS and updated comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1693803002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1693803002/60001
Message was sent while issue was closed.
Description was changed from ========== Make it possible to exclude device management code from rtc_media target. Chromium doesn't use the device managment code in webrtc/media so we need a way to turn it off in order to eliminate Chromium's src/third_party/libjingle/libjingle.gyp BUG=webrtc:4256 NOTRY=True TESTED=Trybots + successfully compiled with GYP_DEFINES=include_internal_device_management=0 webrtc/build/gyp_webrtc ninja -C out/Debug rtc_media ========== to ========== Make it possible to exclude device management code from rtc_media target. Chromium doesn't use the device managment code in webrtc/media so we need a way to turn it off in order to eliminate Chromium's src/third_party/libjingle/libjingle.gyp BUG=webrtc:4256 NOTRY=True TESTED=Trybots + successfully compiled with GYP_DEFINES=include_internal_device_management=0 webrtc/build/gyp_webrtc ninja -C out/Debug rtc_media ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Make it possible to exclude device management code from rtc_media target. Chromium doesn't use the device managment code in webrtc/media so we need a way to turn it off in order to eliminate Chromium's src/third_party/libjingle/libjingle.gyp BUG=webrtc:4256 NOTRY=True TESTED=Trybots + successfully compiled with GYP_DEFINES=include_internal_device_management=0 webrtc/build/gyp_webrtc ninja -C out/Debug rtc_media ========== to ========== Make it possible to exclude device management code from rtc_media target. Chromium doesn't use the device managment code in webrtc/media so we need a way to turn it off in order to eliminate Chromium's src/third_party/libjingle/libjingle.gyp BUG=webrtc:4256 NOTRY=True TESTED=Trybots + successfully compiled with GYP_DEFINES=include_internal_device_management=0 webrtc/build/gyp_webrtc ninja -C out/Debug rtc_media Committed: https://crrev.com/7d9112cbc4a980c505572012dd9253e14f0d8ad4 Cr-Commit-Position: refs/heads/master@{#11816} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7d9112cbc4a980c505572012dd9253e14f0d8ad4 Cr-Commit-Position: refs/heads/master@{#11816} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
