Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
238353008:
Add perf tests for simd yuv->rgb conversion functions. (Closed)
|
Created:
6 years, 8 months ago by rileya (GONE FROM CHROMIUM) Modified:
6 years, 8 months ago Reviewers:
scherkus (not reviewing) CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@m Visibility:
Public. |
DescriptionAdd perf tests for simd yuv->rgb conversion functions.
BUG=172898
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266490
Patch Set 1 #Patch Set 2 : #
Total comments: 12
Patch Set 3 : address comments #
Total comments: 2
Patch Set 4 : fix nits #Patch Set 5 : Move #ifdef up to fix android build. #
Messages
Total messages: 17 (0 generated)
Some simple perf tests for SIMD YUV conversion functions. This is very focused on the individual SIMD functions, which convert/scale/etc a single row of YUV data, rather than the overall YUV convert functions which use them to convert a whole image. The usage of the YUV conversion methods in media/ code is far more limited than it used to be, but since some other parts of the code (remoting/ and pepper/) use the media/ YUV conversion/scaling routines, we can't cull them too indiscriminately (going forward it may be nice to standardize on libyuv or something, but that's beyond the immediate scope of this work)... Next I'll remove the non-PIC specializations of the functions and ensure the perf changes aren't huge (earlier testing showed the nearest-neighbor scaling functions took a ~20% hit, but we always use bilinear scaling currently...). After that adding YUVJ support should be fairly straightforward.
Some simple perf tests for SIMD YUV conversion functions. This is very focused on the individual SIMD functions, which convert/scale/etc a single row of YUV data, rather than the overall YUV convert functions which use them to convert a whole image. The usage of the YUV conversion methods in media/ code is far more limited than it used to be, but since some other parts of the code (remoting/ and pepper/) use the media/ YUV conversion/scaling routines, we can't cull them too indiscriminately (going forward it may be nice to standardize on libyuv or something, but that's beyond the immediate scope of this work)... Next I'll remove the non-PIC specializations of the functions and ensure the perf changes aren't huge (earlier testing showed the nearest-neighbor scaling functions took a ~20% hit, but we always use bilinear scaling currently...). After that adding YUVJ support should be fairly straightforward.
https://codereview.chromium.org/238353008/diff/40001/media/base/yuv_convert_p... File media/base/yuv_convert_perftest.cc (right): https://codereview.chromium.org/238353008/diff/40001/media/base/yuv_convert_p... media/base/yuv_convert_perftest.cc:11: #include "media/base/djb2.h" this isn't used anywhere -- remove (might be worth trimming a bunch of these headers if you yanked them from another file...) https://codereview.chromium.org/238353008/diff/40001/media/base/yuv_convert_p... media/base/yuv_convert_perftest.cc:19: namespace media should go here https://codereview.chromium.org/238353008/diff/40001/media/base/yuv_convert_p... media/base/yuv_convert_perftest.cc:69: ReadData( nit: is it worth having ReadData() if there's only one caller? https://codereview.chromium.org/238353008/diff/40001/media/base/yuv_convert_p... media/base/yuv_convert_perftest.cc:75: }; DISALLOW etc.. https://codereview.chromium.org/238353008/diff/40001/media/base/yuv_convert_p... media/base/yuv_convert_perftest.cc:82: if (!cpu.has_mmx()) { considering our minimum supported arch is SSE2 I'd rather turn these into ASSERTs are there machines out there that really don't have mmx/sse[2]? https://codereview.chromium.org/238353008/diff/40001/media/base/yuv_convert_p... media/base/yuv_convert_perftest.cc:117: InitYV12Data(); this is called every time -- move into ctor?
https://codereview.chromium.org/238353008/diff/40001/media/base/yuv_convert_p... File media/base/yuv_convert_perftest.cc (right): https://codereview.chromium.org/238353008/diff/40001/media/base/yuv_convert_p... media/base/yuv_convert_perftest.cc:11: #include "media/base/djb2.h" On 2014/04/22 18:10:56, scherkus wrote: > this isn't used anywhere -- remove > > (might be worth trimming a bunch of these headers if you yanked them from > another file...) Whoops, yeah I grabbed this from yuv_convert_unittest.cc. Removed unnecessary ones. https://codereview.chromium.org/238353008/diff/40001/media/base/yuv_convert_p... media/base/yuv_convert_perftest.cc:19: On 2014/04/22 18:10:56, scherkus wrote: > namespace media should go here Done. https://codereview.chromium.org/238353008/diff/40001/media/base/yuv_convert_p... media/base/yuv_convert_perftest.cc:69: ReadData( On 2014/04/22 18:10:56, scherkus wrote: > nit: is it worth having ReadData() if there's only one caller? Removed. https://codereview.chromium.org/238353008/diff/40001/media/base/yuv_convert_p... media/base/yuv_convert_perftest.cc:75: }; On 2014/04/22 18:10:56, scherkus wrote: > DISALLOW etc.. Added DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/238353008/diff/40001/media/base/yuv_convert_p... media/base/yuv_convert_perftest.cc:82: if (!cpu.has_mmx()) { On 2014/04/22 18:10:56, scherkus wrote: > considering our minimum supported arch is SSE2 I'd rather turn these into > ASSERTs > > are there machines out there that really don't have mmx/sse[2]? Probably not many, but I was following the convention set by the yuv convert unit tests. Turned these into ASSERT_TRUEs. https://codereview.chromium.org/238353008/diff/40001/media/base/yuv_convert_p... media/base/yuv_convert_perftest.cc:117: InitYV12Data(); On 2014/04/22 18:10:56, scherkus wrote: > this is called every time -- move into ctor? Done.
lgtm w/ nits https://codereview.chromium.org/238353008/diff/100001/media/base/yuv_convert_... File media/base/yuv_convert_perftest.cc (right): https://codereview.chromium.org/238353008/diff/100001/media/base/yuv_convert_... media/base/yuv_convert_perftest.cc:44: void InitYV12Data() { might as well move this into the ctor / use initializer lists https://codereview.chromium.org/238353008/diff/100001/media/base/yuv_convert_... media/base/yuv_convert_perftest.cc:69: DISALLOW_COPY_AND_ASSIGN(YUVConvertPerfTest); this macro is only useful if it's behind a private:
The CQ bit was checked by rileya@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rileya@chromium.org/238353008/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_clang_dbg tryserver.chromium on win_chromium_rel
The CQ bit was checked by rileya@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rileya@chromium.org/238353008/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_x64_rel
The CQ bit was checked by rileya@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rileya@chromium.org/238353008/140001
Message was sent while issue was closed.
Change committed as 266490
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/251953003/ by machenbach@chromium.org. The reason for reverting is: Breaks win64 compile.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
