Lift MEDIA_MMX_INTRINSICS_AVAILABLE definition to header and use it to condition media_unittests

Lift MEDIA_MMX_INTRINSICS_AVAILABLE definition to header and use it to condition media_unittests


BUG=173697, 166496
TEST=With other build errors fixed in privates (fix win64 gmock_mutant template functor redefinitions & ignore C4267 size_t to int truncations), win64 media_unittests builds without link error to (shared library) media::FilterYUVRows_MMX
R=scherkus@chromium.org


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181344

[wolenetz, 2013-02-05 23:24:38]

Please review.  Previous related CR is at
https://codereview.chromium.org/12082087

Thanks,
Matt

[scherkus (not reviewing), 2013-02-06 08:09:01]

lgtm w/ nits

try as I might to limit that #define to the .cc I guess I failed :'(

on a side note, those tests could be vastly improved / parameterized, but that's
for another day

https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_uni...
File media/base/yuv_convert_unittest.cc (right):

https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_uni...
media/base/yuv_convert_unittest.cc:778: TEST(YUVConvertTest,
FilterYUVRows_MMX_OutOfBounds) {
gtest supports disabling tests by prepending the test name with DISABLED_

this way the code is still compiled + linked, but is skipped by default

can you change to the following?

#if defined(MEDIA_MMX_INTRINSICS_AVAILABLE)
TEST(YUVConvertTest, DISABLED_FilterYUVRows_MMX_OutOfBounds) {
#else
TEST(YUVConvertTest, FilterYUVRows_MMX_OutOfBounds) {
#endif

https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_uni...
media/base/yuv_convert_unittest.cc:827: #if
defined(MEDIA_MMX_INTRINSICS_AVAILABLE)
ditto

[wolenetz, 2013-02-06 19:03:10]

https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_uni...
File media/base/yuv_convert_unittest.cc (right):

https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_uni...
media/base/yuv_convert_unittest.cc:778: TEST(YUVConvertTest,
FilterYUVRows_MMX_OutOfBounds) {
On 2013/02/06 08:09:01, scherkus wrote:
> gtest supports disabling tests by prepending the test name with DISABLED_
> 
> this way the code is still compiled + linked, but is skipped by default
> 
> can you change to the following?
> 
> #if defined(MEDIA_MMX_INTRINSICS_AVAILABLE)
> TEST(YUVConvertTest, DISABLED_FilterYUVRows_MMX_OutOfBounds) {
> #else
> TEST(YUVConvertTest, FilterYUVRows_MMX_OutOfBounds) {
> #endif

Sorry about being pedantic here.  Please help me understand your comment:
If I understand correctly, the logic would be reverse of your proposal: if MMX
intrinsics are available, then go ahead and test the method if the cpu also
supports mmx, otherwise disable the test.

> this way the code is still compiled + linked,
On win64, the disabled, but still compiled and linked test, would not enable
successful build because the routine that uses MMX intrinsics wouldn't be built
into media.dll, so linking would fail. On win64, the routine is excluded from
media.dll because it uses MMX intrinsics that VS2010 doesn't support
(compilation of media.dll would fail if included).

If the scope of the win64-disabled version of the test were changed to be
essentially a noop (or the current LOG(WARNING)), then using DISABLED_ would
work, I think.

https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_uni...
media/base/yuv_convert_unittest.cc:827: #if
defined(MEDIA_MMX_INTRINSICS_AVAILABLE)
On 2013/02/06 08:09:01, scherkus wrote:
> ditto

See reply for similar comment, above.

[scherkus (not reviewing), 2013-02-06 19:32:14]

Ah if a link error would happen just #ifdef out the whole tests. Don't
bother with the warning log -- it's just log spam that nobody will read :)
On Feb 6, 2013 11:03 AM, <wolenetz@chromium.org> wrote:

>
> https://codereview.chromium.**org/12213029/diff/5001/media/**
>
base/yuv_convert_unittest.cc<https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_unittest.cc>
> File media/base/yuv_convert_**unittest.cc (right):
>
> https://codereview.chromium.**org/12213029/diff/5001/media/**
>
base/yuv_convert_unittest.cc#**newcode778<https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_unittest.cc#newcode778>
> media/base/yuv_convert_**unittest.cc:778: TEST(YUVConvertTest,
> FilterYUVRows_MMX_OutOfBounds) {
> On 2013/02/06 08:09:01, scherkus wrote:
>
>> gtest supports disabling tests by prepending the test name with
>>
> DISABLED_
>
>  this way the code is still compiled + linked, but is skipped by
>>
> default
>
>  can you change to the following?
>>
>
>  #if defined(MEDIA_MMX_INTRINSICS_**AVAILABLE)
>> TEST(YUVConvertTest, DISABLED_FilterYUVRows_MMX_**OutOfBounds) {
>> #else
>> TEST(YUVConvertTest, FilterYUVRows_MMX_OutOfBounds) {
>> #endif
>>
>
> Sorry about being pedantic here.  Please help me understand your
> comment:
> If I understand correctly, the logic would be reverse of your proposal:
> if MMX intrinsics are available, then go ahead and test the method if
> the cpu also supports mmx, otherwise disable the test.
>
>  this way the code is still compiled + linked,
>>
> On win64, the disabled, but still compiled and linked test, would not
> enable successful build because the routine that uses MMX intrinsics
> wouldn't be built into media.dll, so linking would fail. On win64, the
> routine is excluded from media.dll because it uses MMX intrinsics that
> VS2010 doesn't support (compilation of media.dll would fail if
> included).
>
> If the scope of the win64-disabled version of the test were changed to
> be essentially a noop (or the current LOG(WARNING)), then using
> DISABLED_ would work, I think.
>
> https://codereview.chromium.**org/12213029/diff/5001/media/**
>
base/yuv_convert_unittest.cc#**newcode827<https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_unittest.cc#newcode827>
> media/base/yuv_convert_**unittest.cc:827: #if
> defined(MEDIA_MMX_INTRINSICS_**AVAILABLE)
> On 2013/02/06 08:09:01, scherkus wrote:
>
>> ditto
>>
>
> See reply for similar comment, above.
>
>
https://codereview.chromium.**org/12213029/<https://codereview.chromium.org/1...
>

[wolenetz, 2013-02-06 21:21:51]

On 2013/02/06 19:32:14, scherkus wrote:
> Ah if a link error would happen just #ifdef out the whole tests. Don't
> bother with the warning log -- it's just log spam that nobody will read :)
> On Feb 6, 2013 11:03 AM, <mailto:wolenetz@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/12213029/diff/5001/media/**
> >
>
base/yuv_convert_unittest.cc<https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_unittest.cc>
> > File media/base/yuv_convert_**unittest.cc (right):
> >
> > https://codereview.chromium.**org/12213029/diff/5001/media/**
> >
>
base/yuv_convert_unittest.cc#**newcode778<https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_unittest.cc#newcode778>
> > media/base/yuv_convert_**unittest.cc:778: TEST(YUVConvertTest,
> > FilterYUVRows_MMX_OutOfBounds) {
> > On 2013/02/06 08:09:01, scherkus wrote:
> >
> >> gtest supports disabling tests by prepending the test name with
> >>
> > DISABLED_
> >
> >  this way the code is still compiled + linked, but is skipped by
> >>
> > default
> >
> >  can you change to the following?
> >>
> >
> >  #if defined(MEDIA_MMX_INTRINSICS_**AVAILABLE)
> >> TEST(YUVConvertTest, DISABLED_FilterYUVRows_MMX_**OutOfBounds) {
> >> #else
> >> TEST(YUVConvertTest, FilterYUVRows_MMX_OutOfBounds) {
> >> #endif
> >>
> >
> > Sorry about being pedantic here.  Please help me understand your
> > comment:
> > If I understand correctly, the logic would be reverse of your proposal:
> > if MMX intrinsics are available, then go ahead and test the method if
> > the cpu also supports mmx, otherwise disable the test.
> >
> >  this way the code is still compiled + linked,
> >>
> > On win64, the disabled, but still compiled and linked test, would not
> > enable successful build because the routine that uses MMX intrinsics
> > wouldn't be built into media.dll, so linking would fail. On win64, the
> > routine is excluded from media.dll because it uses MMX intrinsics that
> > VS2010 doesn't support (compilation of media.dll would fail if
> > included).
> >
> > If the scope of the win64-disabled version of the test were changed to
> > be essentially a noop (or the current LOG(WARNING)), then using
> > DISABLED_ would work, I think.
> >
> > https://codereview.chromium.**org/12213029/diff/5001/media/**
> >
>
base/yuv_convert_unittest.cc#**newcode827<https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_unittest.cc#newcode827>
> > media/base/yuv_convert_**unittest.cc:827: #if
> > defined(MEDIA_MMX_INTRINSICS_**AVAILABLE)
> > On 2013/02/06 08:09:01, scherkus wrote:
> >
> >> ditto
> >>
> >
> > See reply for similar comment, above.
> >
> >
>
https://codereview.chromium.**org/12213029/%3Chttps://codereview.chromium.org...>
> >

Done in PS5.  Please review again :)

[scherkus (not reviewing), 2013-02-06 21:37:26]

lgtm!
On Feb 6, 2013 1:21 PM, <wolenetz@chromium.org> wrote:

> On 2013/02/06 19:32:14, scherkus wrote:
>
>> Ah if a link error would happen just #ifdef out the whole tests. Don't
>> bother with the warning log -- it's just log spam that nobody will read :)
>> On Feb 6, 2013 11:03 AM, <mailto:wolenetz@chromium.org> wrote:
>>
>
>  >
>> > https://codereview.chromium.****org/12213029/diff/5001/media/****
>> >
>>
>
> base/yuv_convert_unittest.cc<h**ttps://codereview.chromium.**
>
org/12213029/diff/5001/media/**base/yuv_convert_unittest.cc<https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_unittest.cc>
> >
>
>> > File media/base/yuv_convert_****unittest.cc (right):
>> >
>> > https://codereview.chromium.****org/12213029/diff/5001/media/****
>> >
>>
>
> base/yuv_convert_unittest.cc#****newcode778<https://**
> codereview.chromium.org/**12213029/diff/5001/media/base/**
>
yuv_convert_unittest.cc#**newcode778<https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_unittest.cc#newcode778>
> >
>
>> > media/base/yuv_convert_****unittest.cc:778: TEST(YUVConvertTest,
>> > FilterYUVRows_MMX_OutOfBounds) {
>> > On 2013/02/06 08:09:01, scherkus wrote:
>> >
>> >> gtest supports disabling tests by prepending the test name with
>> >>
>> > DISABLED_
>> >
>> >  this way the code is still compiled + linked, but is skipped by
>> >>
>> > default
>> >
>> >  can you change to the following?
>> >>
>> >
>> >  #if defined(MEDIA_MMX_INTRINSICS_****AVAILABLE)
>> >> TEST(YUVConvertTest, DISABLED_FilterYUVRows_MMX_****OutOfBounds) {
>> >> #else
>> >> TEST(YUVConvertTest, FilterYUVRows_MMX_OutOfBounds) {
>> >> #endif
>> >>
>> >
>> > Sorry about being pedantic here.  Please help me understand your
>> > comment:
>> > If I understand correctly, the logic would be reverse of your proposal:
>> > if MMX intrinsics are available, then go ahead and test the method if
>> > the cpu also supports mmx, otherwise disable the test.
>> >
>> >  this way the code is still compiled + linked,
>> >>
>> > On win64, the disabled, but still compiled and linked test, would not
>> > enable successful build because the routine that uses MMX intrinsics
>> > wouldn't be built into media.dll, so linking would fail. On win64, the
>> > routine is excluded from media.dll because it uses MMX intrinsics that
>> > VS2010 doesn't support (compilation of media.dll would fail if
>> > included).
>> >
>> > If the scope of the win64-disabled version of the test were changed to
>> > be essentially a noop (or the current LOG(WARNING)), then using
>> > DISABLED_ would work, I think.
>> >
>> > https://codereview.chromium.****org/12213029/diff/5001/media/****
>> >
>>
>
> base/yuv_convert_unittest.cc#****newcode827<https://**
> codereview.chromium.org/**12213029/diff/5001/media/base/**
>
yuv_convert_unittest.cc#**newcode827<https://codereview.chromium.org/12213029/diff/5001/media/base/yuv_convert_unittest.cc#newcode827>
> >
>
>> > media/base/yuv_convert_****unittest.cc:827: #if
>> > defined(MEDIA_MMX_INTRINSICS_****AVAILABLE)
>> > On 2013/02/06 08:09:01, scherkus wrote:
>> >
>> >> ditto
>> >>
>> >
>> > See reply for similar comment, above.
>> >
>> >
>>
>
> https://codereview.chromium.****org/12213029/%3Chttps://codere**
> view.chromium.org/12213029/ <http://codereview.chromium.org/12213029/>>
>
>> >
>>
>
> Done in PS5.  Please review again :)
>
>
https://codereview.chromium.**org/12213029/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-02-06 22:12:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/12213029/1008

[commit-bot: I haz the power, 2013-02-07 17:08:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wolenetz@chromium.org/12213029/1008

[commit-bot: I haz the power, 2013-02-07 19:09:40]

Change committed as 181344