Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(36)

Issue 15151002: Streamline SIMD targets in media.gyp (Closed)

Created:
7 years, 7 months ago by DaleCurtis
Modified:
7 years, 7 months ago
CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, dcaiafa+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, feature-media-reviews_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, darin-cc_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org, Mark Mentovai
Visibility:
Public.

Description

Streamline SIMD targets in media.gyp Prevents duplicates of the media_sse and yuv_convert targets from ending up in both media and media_unittests during shared builds. - Removes the yuv_convert target since everyone who uses it already uses media. - Merges differ_block_sse2 and yuv_convert_simd_x86 into media_sse2. - Moves assembly into media_asm. - Moves incorrect mmx bundling from sse2 to new media_mmx target. - Introduces EXPORT macro to x86inc.asm - Introduces yasm_includes for non-.asm files in yasm_compile.gypi. - Fixes missing const on yuv constants table. BUG=none TEST=compiles, all unittests pass. TBR=fischman@chromium.org, kbr@chromium.org, sergeyu@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202230

Patch Set 1 : Exports. #

Patch Set 2 : Fix includes. #

Patch Set 3 : Fix exports. #

Total comments: 17

Patch Set 4 : Comments. #

Total comments: 2

Patch Set 5 : Rename include. #

Patch Set 6 : Add Win64 hack. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+625 lines, -597 lines) Patch
M media/base/simd/convert_rgb_to_yuv.h View 1 2 3 2 chunks +50 lines, -58 lines 0 comments Download
M media/base/simd/convert_rgb_to_yuv_ssse3.asm View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M media/base/simd/convert_rgb_to_yuv_ssse3.inc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M media/base/simd/convert_yuv_to_rgb.h View 1 2 3 2 chunks +153 lines, -194 lines 0 comments Download
M media/base/simd/convert_yuv_to_rgb_c.cc View 3 chunks +2 lines, -6 lines 0 comments Download
M media/base/simd/convert_yuv_to_rgb_mmx.inc View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M media/base/simd/convert_yuva_to_argb_mmx.inc View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M media/base/simd/empty_register_state_mmx.asm View 1 2 3 4 2 chunks +2 lines, -1 line 0 comments Download
M media/base/simd/filter_yuv.h View 1 2 3 1 chunk +21 lines, -14 lines 0 comments Download
M media/base/simd/linear_scale_yuv_to_rgb_mmx.inc View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download
M media/base/simd/linear_scale_yuv_to_rgb_mmx_x64.asm View 1 2 3 4 2 chunks +2 lines, -1 line 0 comments Download
A media/base/simd/media_export.asm View 1 2 3 4 1 chunk +48 lines, -0 lines 0 comments Download
M media/base/simd/scale_yuv_to_rgb_mmx.inc View 1 2 3 4 2 chunks +4 lines, -2 lines 0 comments Download
M media/base/simd/scale_yuv_to_rgb_sse2_x64.asm View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M media/base/simd/yuv_to_rgb_table.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M media/base/simd/yuv_to_rgb_table.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M media/base/yuv_convert.h View 3 chunks +89 lines, -88 lines 0 comments Download
M media/base/yuv_convert.cc View 1 2 3 4 5 2 chunks +64 lines, -8 lines 0 comments Download
M media/base/yuv_convert_unittest.cc View 2 chunks +4 lines, -0 lines 0 comments Download
M media/media.gyp View 1 2 3 4 40 chunks +161 lines, -195 lines 0 comments Download
M remoting/remoting.gyp View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M third_party/x86inc/x86inc.asm View 1 2 3 2 chunks +3 lines, -17 lines 0 comments Download
M third_party/yasm/yasm_compile.gypi View 1 4 chunks +6 lines, -2 lines 0 comments Download
M ui/surface/surface.gyp View 1 2 1 chunk +1 line, -1 line 0 comments Download
M webkit/media/webkit_media.gypi View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 21 (0 generated)
DaleCurtis
Not ready for review yet. I'm hitting a wall with making the YASM generated functions ...
7 years, 7 months ago (2013-05-14 01:15:55 UTC) #1
Ami GONE FROM CHROMIUM
On 2013/05/14 01:15:55, DaleCurtis wrote: > Not ready for review yet. I'm hitting a wall ...
7 years, 7 months ago (2013-05-14 02:01:09 UTC) #2
DaleCurtis
Bonus guess was on the money. Thanks! Running some more testing now, will update when ...
7 years, 7 months ago (2013-05-14 18:09:51 UTC) #3
DaleCurtis
Actually, I take it back. Without PRIVATE we end up with symbols higher than _ChromeMain ...
7 years, 7 months ago (2013-05-15 00:38:40 UTC) #4
DaleCurtis
Double actually, just linking media_asm doesn't work since the asm code needs a static table. ...
7 years, 7 months ago (2013-05-15 01:18:20 UTC) #5
Ami GONE FROM CHROMIUM
I'm confused. Don't we _want_ these symbols to be visible? Is the right thing to ...
7 years, 7 months ago (2013-05-15 03:52:15 UTC) #6
DaleCurtis
I don't think a "media.framework" is possible, we would need to add entries to the ...
7 years, 7 months ago (2013-05-15 17:20:03 UTC) #7
DaleCurtis
s/media.framework/media.order/
7 years, 7 months ago (2013-05-15 17:20:36 UTC) #8
Mark Mentovai
You don’t want those symbols to be exported from Chromium Framework in a non-component build ...
7 years, 7 months ago (2013-05-15 17:26:33 UTC) #9
DaleCurtis
Thanks for the insight, exporting at the framework level is the wrong choice then. Switching ...
7 years, 7 months ago (2013-05-15 18:19:53 UTC) #10
Ami GONE FROM CHROMIUM
Dale: taking a minor performance hit for PIC when building in shared_library mode seems fine ...
7 years, 7 months ago (2013-05-15 18:26:50 UTC) #11
DaleCurtis
After discussion with fischman@ we determined the problem with the non-ForTesting approach, yasm code had ...
7 years, 7 months ago (2013-05-18 01:06:15 UTC) #12
Sergey Ulanov
remoting.gyp - lgtm
7 years, 7 months ago (2013-05-18 01:38:55 UTC) #13
Ami GONE FROM CHROMIUM
That's a lot of crack you're ferreting out of the system. Nice. https://codereview.chromium.org/15151002/diff/45001/media/base/simd/convert_rgb_to_yuv.h File media/base/simd/convert_rgb_to_yuv.h ...
7 years, 7 months ago (2013-05-18 02:22:03 UTC) #14
Ken Russell (switch to Gerrit)
ui/surface/surface.gyp LGTM Did this end up not being pursued?
7 years, 7 months ago (2013-05-20 18:12:46 UTC) #15
DaleCurtis
I'm not sure what happened, the "closed" mark ended up being ticked by someone accidentally ...
7 years, 7 months ago (2013-05-21 18:42:46 UTC) #16
DaleCurtis
Comments addressed. PTAL. https://codereview.chromium.org/15151002/diff/45001/media/base/simd/convert_rgb_to_yuv.h File media/base/simd/convert_rgb_to_yuv.h (right): https://codereview.chromium.org/15151002/diff/45001/media/base/simd/convert_rgb_to_yuv.h#newcode12 media/base/simd/convert_rgb_to_yuv.h:12: On 2013/05/18 02:22:04, Ami Fischman wrote: ...
7 years, 7 months ago (2013-05-23 23:43:22 UTC) #17
Ami GONE FROM CHROMIUM
LGTM https://codereview.chromium.org/15151002/diff/57001/media/base/simd/media_export.inc File media/base/simd/media_export.inc (right): https://codereview.chromium.org/15151002/diff/57001/media/base/simd/media_export.inc#newcode8 media/base/simd/media_export.inc:8: %ifndef MEDIA_BASE_SIMD_MEDIA_EXPORT_INC_ Is .inc a standard extension for ...
7 years, 7 months ago (2013-05-24 02:22:37 UTC) #18
DaleCurtis
Thanks for the review! https://codereview.chromium.org/15151002/diff/57001/media/base/simd/media_export.inc File media/base/simd/media_export.inc (right): https://codereview.chromium.org/15151002/diff/57001/media/base/simd/media_export.inc#newcode8 media/base/simd/media_export.inc:8: %ifndef MEDIA_BASE_SIMD_MEDIA_EXPORT_INC_ On 2013/05/24 02:22:38, ...
7 years, 7 months ago (2013-05-24 18:15:33 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/15151002/74001
7 years, 7 months ago (2013-05-24 18:16:00 UTC) #20
DaleCurtis
7 years, 7 months ago (2013-05-24 23:49:09 UTC) #21
Message was sent while issue was closed.
Committed patchset #6 manually as r202230 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698